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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 15 Apr 2024, "Dixit, Ashutosh" <ashutosh.dixit@intel.com> wrote:
> 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.

IMO it's more obvious to set i915->hwmon to NULL separately.

BR,
Jani.

>
>>
>> > +	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

-- 
Jani Nikula, Intel