On Mon, 15 Apr 2024 16:35:02 -0700, Armin Wolf wrote:
>
Hi Armin,
> Am 16.04.24 um 00:36 schrieb Ashutosh Dixit:
> > @@ -818,10 +818,10 @@ void i915_hwmon_register(struct drm_i915_private
> > *i915)
> > hwm_get_preregistration_info(i915);
> >
> > /* hwmon_dev points to device hwmon<i> */
> > - hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> > - ddat,
> > - &hwm_chip_info,
> > - hwm_groups);
> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> > + ddat,
> > + &hwm_chip_info,
> > + hwm_groups);
> > if (IS_ERR(hwmon_dev)) {
> > i915->hwmon = NULL;
>
> you need to free hwmon here, since it is not managed by devres anymore.
Thanks a lot for catching this, I had missed it in v2, it's fixed in v3. I
am actually reusing i915_hwmon_unregister() for error unwinding in v3.
>
> > return;
> > @@ -838,10 +838,10 @@ void i915_hwmon_register(struct drm_i915_private
> > *i915)
> > if (!hwm_gt_is_visible(ddat_gt, hwmon_energy,
> > hwmon_energy_input, 0))
> > continue;
> >
> > - hwmon_dev = devm_hwmon_device_register_with_info(dev,
> > ddat_gt->name,
> > - ddat_gt,
> > -
> > &hwm_gt_chip_info,
> > - NULL);
> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat_gt->name,
> > + ddat_gt,
> > + &hwm_gt_chip_info,
> > + NULL);
> > if (!IS_ERR(hwmon_dev))
> > ddat_gt->hwmon_dev = hwmon_dev;
> > }
> > @@ -849,5 +849,26 @@ void i915_hwmon_register(struct drm_i915_private *i915)
> >
> > void i915_hwmon_unregister(struct drm_i915_private *i915)
> > {
> > - fetch_and_zero(&i915->hwmon);
> > + struct i915_hwmon *hwmon = fetch_and_zero(&i915->hwmon);
>
> Why is fetch_and_zero() necessary here?
As mentioned, in v3 i915_hwmon_unregister() itself is used for error
unwinding so we need to prevent multiple device_unregister's etc. That is
the purpose of setting i915->hwmon to NULL. But even earlier, though it is
not obvious, i915_hwmon_unregister() is called multiple times. So e.g. it
will be called at device unbind as well as module unload. So once again we
prevent multiple device_unregister's by setting and checking for NULL
i915->hwmon.
>
> > + struct hwm_drvdata *ddat = &hwmon->ddat;
> > + struct intel_gt *gt;
> > + int i;
> > +
> > + if (!hwmon)
> > + return;
> > +
> > + for_each_gt(gt, i915, i) {
> > + struct hwm_drvdata *ddat_gt = hwmon->ddat_gt + i;
> > +
> > + if (ddat_gt->hwmon_dev) {
> > + hwmon_device_unregister(ddat_gt->hwmon_dev);
> > + ddat_gt->hwmon_dev = NULL;
> > + }
> > + }
> > +
> > + if (ddat->hwmon_dev)
> > + hwmon_device_unregister(ddat->hwmon_dev);
> > +
> > + mutex_destroy(&hwmon->hwmon_lock);
> > + kfree(hwmon);
> > }
Thanks.
--
Ashutosh
