On Wed, 21 Sep 2022 05:44:35 -0700, Andi Shyti wrote:
>
> > +void i915_hwmon_register(struct drm_i915_private *i915)
> > +{
> > + struct device *dev = i915->drm.dev;
> > + struct i915_hwmon *hwmon;
> > + struct device *hwmon_dev;
> > + struct hwm_drvdata *ddat;
> > +
> > + /* hwmon is available only for dGfx */
> > + if (!IS_DGFX(i915))
> > + return;
> > +
> > + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
>
> why don't we use devm_kzalloc?
>
> > + if (!hwmon)
> > + return;
> > +
> > + i915->hwmon = hwmon;
> > + mutex_init(&hwmon->hwmon_lock);
> > + ddat = &hwmon->ddat;
> > +
> > + ddat->hwmon = hwmon;
> > + ddat->uncore = &i915->uncore;
> > + snprintf(ddat->name, sizeof(ddat->name), "i915");
> > +
> > + hwm_get_preregistration_info(i915);
> > +
> > + /* hwmon_dev points to device hwmon<i> */
> > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> > + ddat,
> > + &hwm_chip_info,
> > + NULL);
> > + if (IS_ERR(hwmon_dev)) {
> > + mutex_destroy(&hwmon->hwmon_lock);
>
> there is not such a big need to destroy the mutex. Destroying
> mutexes is more useful when you actually are creating/destroying
> and there is some debug need. I don't think that's the case.
>
> With the devm_kzalloc this would be just a return.
If we are using devm_kzalloc we might as well replace all the
hwmon_device_register_with_info's (in Patch 1 and 7) with
devm_hwmon_device_register_with_info and then i915_hwmon_unregister is just
this:
void i915_hwmon_unregister(struct drm_i915_private *i915)
{
fetch_and_zero(&i915->hwmon);
}
Even the above statement is probably not needed but might as well retain it
for sanity. So this is a simple change.
Thanks.
--
Ashutosh
