[PATCH v3] drm/xe/hwmon: Update xe hwmon with couple of fixes

[Nilawar, Badal]

Hi Karthik,

On 03-04-2024 18:43, Lucas De Marchi wrote:
> On Wed, Apr 03, 2024 at 06:22:24PM +0530, Poosa, Karthik wrote:
>>
>> On 03-04-2024 00:48, Lucas De Marchi wrote:
>>> On Tue, Apr 02, 2024 at 11:39:56PM +0530, Karthik Poosa wrote:
>>>> Address potential overflows in result of multiplication of two lower
>>>> precision (u32) operands before widening it to higher precision (u64).
>>>>
>>>> Initialize variables which were being used uninitialized.
>>>>
>>>> v2:
>>>> - Rebase.
>>>> - In xe_hwmon_process_reg, set argument 'value' to 0 on failure.
>>>>   With this change caller function need not initialize value to 0.
>>>>
>>>> v3:
>>>> - Update commit msg as per review comments (Rodrigo, Lucas).
>>>> - Update xe_hwmon_get_reg. Return bool from xe_hwmon_get_reg
>>>>   and output xe_reg. This is to have abstracted usage of xe_reg. 
>>>> (Lucas)
>>>>
>>>> Fixes: 4446fcf220ce ("drm/xe/hwmon: Expose power1_max_interval")
>>>> Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
>>>> Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_hwmon.c | 60 ++++++++++++++++++++---------------
>>>> 1 file changed, 35 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c 
>>>> b/drivers/gpu/drm/xe/xe_hwmon.c
>>>> index 7e8caac838e0..8c805ace592c 100644
>>>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>>>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>>>> @@ -79,46 +79,48 @@ struct xe_hwmon {
>>>>     struct xe_hwmon_energy_info ei[CHANNEL_MAX];
>>>> };
>>>>
>>>> -static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum 
>>>> xe_hwmon_reg hwmon_reg, int channel)
>>>> +static bool xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum 
>>>> xe_hwmon_reg hwmon_reg,
>>>> +                 int channel, struct xe_reg *reg)
>>>
>>> patch 1 could add xe_reg_is_valid(), just like we have for i915. This
>>> will create the missing abstraction
>>>
>> Should this be added in xe_regs.h or xe_hwmon.c ?
> 
> drivers/gpu/drm/xe/regs/xe_reg_defs.h
Function will look like
bool xe_reg_is_valid(struct xe_reg reg)
{	
	if(!reg.raw)
		return false;
	else
	   	return true;
}

> 
>>> patch 2 could change this function, but rather than returning bool,
>>> return struct xe_reg (yes, by value). The caller will then do a
>>> if (!xe_reg_is_valid(reg)) return. This will unbreak the abstraction.
>>>
>> There would be a call to xe_reg_is_valid() after every xe_hwmon_get_reg.
>>
>> Can't this be as part of xe_hwmon_get_reg itself ?
> 
> I don't follow ... xe_hwmon_get_reg() returns the invalid reg, the
> caller checks it. It's similar to what you did by checking the bool
> return, but instead of having the return value + the pointer,
> we are just using the return value with the added abstraction for
> invalid registers.

Where ever needed, especially reg valid check needed in visible 
functions to decide whether to create sysfs entry or not, use.

%s/if(!xe_hwmon_get_reg(reg)/if(xe_reg_is_valid(xe_hwmon_get_reg(reg)))/


Regards,
Badal
> 
> Lucas De Marchi