Re: [PATCH v7 3/4] drm/xe/hwmon: Update xe_hwmon_process_reg

["Nilawar, Badal" <badal.nilawar@intel.com>]

Hi Karthik,

On 24-04-2024 17:10, Lucas De Marchi wrote:
> On Wed, Apr 24, 2024 at 01:46:45PM GMT, Nilawar, Badal wrote:
>>> I's my personal feeling. I don't like the pattern xe_hwmon.c is using
>>> since it's very easy to introduce buggy code. If someone wants to give a
>>> r-b and merge this particular patch, fine. But I do think this pattern
>>> should be changed.
>> Idea of xe_hwmon_process_reg to abstract the register accesses. In 
>> i915 we maintain register addresses inside the hwmon structure. During 
>> reviews that idea was not much likeable. So we came up with 
>> xe_hwmon_get_reg, xe_hwmon_process_reg.
> 
> xe_hwmon_get_reg() is totally fine. It is xe_hwmon_process_reg() that
> abstracts what it shouldn't IMO. As shown in the snippet, the only thing
> it does is to make it easier to introduce hidden bugs.
To xe_hwmon_process_reg effectively we should validate all the regs 
hwmon attribute is using. For power1_max attribute we should have 
validate PACKAGE_POWER_SKU as well. But PACKAGE_POWER_SKU is legacy 
register and on some platforms it may not be valid but 
REG_PKG_RAPL_LIMIT will be valid. In that case it doesn't make sense to 
block power1_max attribute. So please feel free to drop 
xe_hwmon_process_reg.

Regards,
Badal
> 
> Lucas De Marchi