Re: [Intel-gfx] [PATCH v2] drm/i915/hwmon: Use 0 to designate disabled PL1 power limit

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 31 Mar 2023 03:23:33 -0700, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> > @@ -385,8 +395,22 @@ static int
> >   hwm_power_max_write(struct hwm_drvdata *ddat, long val)
> >   {
> >	struct i915_hwmon *hwmon = ddat->hwmon;
> > +	intel_wakeref_t wakeref;
> >	u32 nval;
> >   +	if (val == PL1_DISABLE) {
> > +		/* Disable PL1 limit */
> > +		hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> > +						    PKG_PWR_LIM_1_EN, 0);
> > +
> > +		/* Verify, because PL1 limit cannot be disabled on all platforms */
>
> I think there is a race right here, since above grabbed and released the
> hwmon_lock, anyone can modify it at this point before the verification
> below. Not sure if any consequences worse than a wrong -EPERM are possible
> though.
>
> Also, is EPERM correct for something hardware does not support? We usually
> say ENODEV for such things, IIRC at least.

Changed to -ENODEV in v3.

> Anyway, race looks easily solvable by holding the existing mutex and a
> single rpm ref for the whole rmw-r cycle.

Fixed in v3, thanks for catching these.

Ashutosh

> > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > +			nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit);
> > +		if (nval & PKG_PWR_LIM_1_EN)
> > +			return -EPERM;
> > +		return 0;
> > +	}
> > +
> >	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> >	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
> >	nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval);