Re: [PATCH 1/2] drm/xe/hwmon: Protect hwmon rw attributes with hwmon_lock

["Gupta, Anshuman" <anshuman.gupta@xxxxxxxxx>]

On 10/6/2023 10:32 PM, Badal Nilawar wrote:
> Take hwmon_lock while accessing hwmon rw attributes. For readonly
> attributes its not required to take lock as reads are protected
> by sysfs layer and therefore sequential.
>
> Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> Signed-off-by: Badal Nilawar <badal.nilawar@xxxxxxxxx>
> ---
>   drivers/gpu/drm/xe/xe_hwmon.c | 143 +++++++++++++++++-----------------
>   1 file changed, 71 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 9ac05994a967..391f9a8dd9d7 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -47,7 +47,7 @@ struct xe_hwmon_energy_info {
>   struct xe_hwmon {
>   	struct device *hwmon_dev;
>   	struct xe_gt *gt;
> -	struct mutex hwmon_lock; /* rmw operations*/
> +	struct mutex hwmon_lock;	/* For rw attributes */
>   	int scl_shift_power;
>   	int scl_shift_energy;
>   	struct xe_hwmon_energy_info ei;	/*  Energy info for energy1_input */
> @@ -95,47 +95,45 @@ static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
>   	return reg.raw;
>   }
>   
> -static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> -				enum xe_hwmon_reg_operation operation, u32 *value,
> -				u32 clr, u32 set)
> +static void xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> +				 enum xe_hwmon_reg_operation operation, u32 *value,
> +				 u32 clr, u32 set)
>   {
>   	struct xe_reg reg;
>   
>   	reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
>   
>   	if (!reg.raw)
> -		return -EOPNOTSUPP;
> +		return;
Don't we need to report this as warning?
What is possiblity of code path landing here.
>   
>   	switch (operation) {
>   	case REG_READ:
>   		*value = xe_mmio_read32(hwmon->gt, reg);
> -		return 0;
> +		break;
>   	case REG_WRITE:
>   		xe_mmio_write32(hwmon->gt, reg, *value);
> -		return 0;
> +		break;
>   	case REG_RMW:
>   		*value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
> -		return 0;
> +		break;
>   	default:
>   		drm_warn(&gt_to_xe(hwmon->gt)->drm, "Invalid xe hwmon reg operation: %d\n",
>   			 operation);
> -		return -EOPNOTSUPP;
> +		break;
>   	}
>   }
>   
> -static int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon,
> -				       enum xe_hwmon_reg hwmon_reg, u64 *value)
> +static void xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon,
> +					enum xe_hwmon_reg hwmon_reg, u64 *value)Isn't it possible to pass len of void * value to xe_hwmon_process_reg()
so we can wrap this fucntion in xe_hwmon_process_reg ?

>   {
>   	struct xe_reg reg;
>   
>   	reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
>   
>   	if (!reg.raw)
> -		return -EOPNOTSUPP;
> +		return;
>   
>   	*value = xe_mmio_read64_2x32(hwmon->gt, reg);
> -
> -	return 0;
>   }
>   
>   #define PL1_DISABLE 0
> @@ -146,16 +144,18 @@ static int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon,
>    * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
>    * clamped values when read.
>    */
> -static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
> +static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
>   {
>   	u32 reg_val;
>   	u64 reg_val64, min, max;
>   
> +	mutex_lock(&hwmon->hwmon_lock);
> +
>   	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val, 0, 0);
>   	/* Check if PL1 limit is disabled */
>   	if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>   		*value = PL1_DISABLE;
> -		return 0;
> +		goto unlock;
>   	}
>   
>   	reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
> @@ -169,14 +169,17 @@ static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
>   
>   	if (min && max)
>   		*value = clamp_t(u64, *value, min, max);
> -
> -	return 0;
> +unlock:
> +	mutex_unlock(&hwmon->hwmon_lock);
>   }
>   
>   static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
>   {
> +	int ret = 0;
>   	u32 reg_val;
>   
> +	mutex_lock(&hwmon->hwmon_lock);
> +
>   	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>   	if (value == PL1_DISABLE) {
>   		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
> @@ -184,8 +187,10 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
>   		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val,
>   				     PKG_PWR_LIM_1_EN, 0);
>   
> -		if (reg_val & PKG_PWR_LIM_1_EN)
> -			return -EOPNOTSUPP;
> +		if (reg_val & PKG_PWR_LIM_1_EN) {
> +			ret = -EOPNOTSUPP;
> +			goto unlock;
> +		}
>   	}
>   
>   	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> @@ -194,19 +199,18 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
>   
>   	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
>   			     PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
> -
> -	return 0;
> +unlock:
> +	mutex_unlock(&hwmon->hwmon_lock);
> +	return ret;
>   }
>   
> -static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
> +static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
>   {
>   	u32 reg_val;
>   
>   	xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ, &reg_val, 0, 0);
>   	reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
>   	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> -
> -	return 0;
>   }
>   
>   /*
> @@ -235,10 +239,6 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy)
>   	struct xe_hwmon_energy_info *ei = &hwmon->ei;
>   	u32 reg_val;
>   
> -	xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> -
> -	mutex_lock(&hwmon->hwmon_lock);
> -
>   	xe_hwmon_process_reg(hwmon, REG_PKG_ENERGY_STATUS, REG_READ,
>   			     &reg_val, 0, 0);
>   
> @@ -251,10 +251,6 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy)
>   
>   	*energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
>   				  hwmon->scl_shift_energy);
> -
> -	mutex_unlock(&hwmon->hwmon_lock);
> -
> -	xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>   }
>   
>   static const struct hwmon_channel_info *hwmon_info[] = {
> @@ -284,6 +280,38 @@ static int xe_hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
>   			      uval);
>   }
>   
> +static int xe_hwmon_power_curr_crit_read(struct xe_hwmon *hwmon, long *value, u32 scale_factor)
> +{
> +	int ret;
> +	u32 uval;
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
> +	if (ret)
> +		goto unlock;
> +
> +	*value = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
> +				 scale_factor, POWER_SETUP_I1_SHIFT);
> +unlock:
> +	mutex_unlock(&hwmon->hwmon_lock);
> +	return ret;
> +}
> +
> +static int xe_hwmon_power_curr_crit_write(struct xe_hwmon *hwmon, long value, u32 scale_factor)
> +{
> +	int ret;
> +	u32 uval;
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	uval = DIV_ROUND_CLOSEST_ULL(value << POWER_SETUP_I1_SHIFT, scale_factor);
> +	ret = xe_hwmon_pcode_write_i1(hwmon->gt, uval);
> +
> +	mutex_unlock(&hwmon->hwmon_lock);
> +	return ret;
> +}
> +
>   static int xe_hwmon_get_voltage(struct xe_hwmon *hwmon, long *value)
>   {
>   	u32 reg_val;
> @@ -317,23 +345,15 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
>   static int
>   xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
>   {
> -	int ret;
> -	u32 uval;
> -
>   	switch (attr) {
>   	case hwmon_power_max:
> -		return xe_hwmon_power_max_read(hwmon, val);
> +		xe_hwmon_power_max_read(hwmon, val);
> +		return 0;
>   	case hwmon_power_rated_max:
> -		return xe_hwmon_power_rated_max_read(hwmon, val);
> -	case hwmon_power_crit:
> -		ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
> -		if (ret)
> -			return ret;
> -		if (!(uval & POWER_SETUP_I1_WATTS))
> -			return -ENODEV;
> -		*val = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
> -				       SF_POWER, POWER_SETUP_I1_SHIFT);
> +		xe_hwmon_power_rated_max_read(hwmon, val);
>   		return 0;
> +	case hwmon_power_crit:
> +		return xe_hwmon_power_curr_crit_read(hwmon, val, SF_POWER);
>   	default:
>   		return -EOPNOTSUPP;
>   	}
> @@ -342,14 +362,11 @@ xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
>   static int
>   xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long val)
>   {
> -	u32 uval;
> -
>   	switch (attr) {
>   	case hwmon_power_max:
>   		return xe_hwmon_power_max_write(hwmon, val);
>   	case hwmon_power_crit:
> -		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
> -		return xe_hwmon_pcode_write_i1(hwmon->gt, uval);
> +		return xe_hwmon_power_curr_crit_write(hwmon, val, SF_POWER);
>   	default:
>   		return -EOPNOTSUPP;
>   	}
> @@ -372,19 +389,9 @@ xe_hwmon_curr_is_visible(const struct xe_hwmon *hwmon, u32 attr)
>   static int
>   xe_hwmon_curr_read(struct xe_hwmon *hwmon, u32 attr, long *val)
>   {
> -	int ret;
> -	u32 uval;
> -
>   	switch (attr) {
>   	case hwmon_curr_crit:
> -		ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
> -		if (ret)
> -			return ret;
> -		if (uval & POWER_SETUP_I1_WATTS)
> -			return -ENODEV;
> -		*val = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
> -				       SF_CURR, POWER_SETUP_I1_SHIFT);
> -		return 0;
> +		return xe_hwmon_power_curr_crit_read(hwmon, val, SF_CURR);
>   	default:
>   		return -EOPNOTSUPP;
>   	}
> @@ -393,12 +400,9 @@ xe_hwmon_curr_read(struct xe_hwmon *hwmon, u32 attr, long *val)
>   static int
>   xe_hwmon_curr_write(struct xe_hwmon *hwmon, u32 attr, long val)
>   {
> -	u32 uval;
> -
>   	switch (attr) {
>   	case hwmon_curr_crit:
> -		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_CURR);
> -		return xe_hwmon_pcode_write_i1(hwmon->gt, uval);
> +		return xe_hwmon_power_curr_crit_write(hwmon, val, SF_CURR);
>   	default:
>   		return -EOPNOTSUPP;
>   	}
> @@ -420,8 +424,6 @@ xe_hwmon_in_read(struct xe_hwmon *hwmon, u32 attr, long *val)
>   {
>   	int ret;
>   
> -	xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> -
>   	switch (attr) {
>   	case hwmon_in_input:
>   		ret = xe_hwmon_get_voltage(hwmon, val);
> @@ -430,8 +432,6 @@ xe_hwmon_in_read(struct xe_hwmon *hwmon, u32 attr, long *val)
>   		ret = -EOPNOTSUPP;
>   	}
>   
> -	xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> -
>   	return ret;
>   }
>   
> @@ -565,14 +565,13 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
>   	struct xe_hwmon *hwmon = xe->hwmon;
>   	long energy;
>   	u32 val_sku_unit = 0;
> -	int ret;
>   
> -	ret = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
>   	/*
>   	 * The contents of register PKG_POWER_SKU_UNIT do not change,
>   	 * so read it once and store the shift values.
>   	 */
> -	if (!ret) {
> +	if (xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT)) {
> +		xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
>   		hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
>   		hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
>   	}