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);
