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

[Lucas De Marchi]

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

>>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.

Lucas De Marchi