Re: [PATCH v2] drm/i915/hwmon: Get rid of devm

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

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