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