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