[Intel-xe] [PATCH v2 0/6] Add HWMON support for DGFX

[Guenter Roeck]

On 7/2/23 08:57, Dixit, Ashutosh wrote:
> On Sat, 01 Jul 2023 20:02:51 -0700, Guenter Roeck wrote:
>>
>> On 7/1/23 18:31, Dixit, Ashutosh wrote:
>>> On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
>>>>
>>>
>>> Hi Badal,
>>>
>>>> This series adds the hwmon support on xe driver for DGFX
>>>
>>> Needs some discussion but I have a general comment on this series
>>> first. The implementation here follow what was done for i915. But how
>>> "hwmon attributes are defined" I think we should look at how this was done
>>> in other drm drivers, namely amdgpu and radeon. Look here (search for
>>> "hwmon_attributes"):
>>>
>>> drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
>>> drivers/gpu/drm/radeon/radeon_pm.c
>>>
>>> Here the hwmon attribute definition is very similar to how general sysfs
>>> attributes are defined (they will just appear in hwmon directories) and
>>> does not carry baggage of the hwmon infrastructure (what i915 has). So my
>>> preference is to shift to this amd/radeon way for xe.
>>>
>>
>> You mean your preference is to use a deprecated hardware monitoring
>> registration function and to explicitly violate the following statement
>> from Documentation/hwmon/hwmon-kernel-api.rst ?
>>
>>    All other hardware monitoring device registration functions are deprecated
>>    and must not be used in new drivers.
> 
> I missed that, but since we also have this in ddaefa209c4a ("hwmon: Make
> chip parameter for with_info API mandatory"), yes that is what it would
> boil down to.
> 

The chip parameter covers all standard hwmon sysfs attributes. A hwmon driver
without standard sysfs attributes is not a hwmon driver. It abuses the hwmon
subsystem and its API/ABI. If I catch such a driver, I'll NACK it. If I find
one in the kernel, I will do my best to get it removed.

>> That is quite interesting. Please elaborate and explain your rationale.
> 
> Basically, like those other drm drivers, the chip parameter is of no use to
> us (or at least we'd be totally fine not using it), hence the desire to
> skip it.
> 
> But we are still required to use what we don't need? Do you care about
> drivers outside drivers/hwmon?
> 

I would suggest to read the hwmon API more closely to understand it. Your claim
that "the chip parameter is of no use to us" is simply wrong, as should be obvious
when you read this submission. Actually, if you would convert the other
drm drivers to use it, it would reduce the size of the hwmon specific code
in those drivers, typically by 20-40%. Given that, I must admit that I am quite
baffled by your claim. Maybe you could explain that in more detail.

Of course, I care about use of the hardware monitoring subsystem
outside drivers/hwmon. Unlike other maintainers, I let people register drivers
from outside that directory, but that doesn't mean that I don't care.

FWIW, you are close to convincing me to add a warning message to the kernel
to tell users of deprecated hwmon APIs that the API is deprecated.
Alternatively, I may stop permitting new hwmon drivers outside drivers/hwmon.

Guenter

> Thanks.
> --
> Ashutosh