| Message ID | 20211222020114.3524736-5-liu.yun@linux.dev (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/5] hwmon: (corsair-cpro) Use devres function | expand |
On 12/21/21 6:01 PM, Jackie Liu wrote: > From: Jackie Liu <liuyun01@kylinos.cn> > > Use devm_hwmon_device_register_with_info() and remove hwmon_dev > from drivetemp_data struct as it is not needed anymore. > > Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> > --- > drivers/hwmon/drivetemp.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c > index 1eb37106a220..1a5a62288c0a 100644 > --- a/drivers/hwmon/drivetemp.c > +++ b/drivers/hwmon/drivetemp.c > @@ -113,7 +113,6 @@ struct drivetemp_data { > struct mutex lock; /* protect data buffer accesses */ > struct scsi_device *sdev; /* SCSI device */ > struct device *dev; /* instantiating device */ > - struct device *hwdev; /* hardware monitoring device */ > u8 smartdata[ATA_SECT_SIZE]; /* local buffer */ > int (*get_temp)(struct drivetemp_data *st, u32 attr, long *val); > bool have_temp_lowest; /* lowest temp in SCT status */ > @@ -555,6 +554,7 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf) > { > struct scsi_device *sdev = to_scsi_device(dev->parent); > struct drivetemp_data *st; > + struct device *hwmon_dev; > int err; > > st = kzalloc(sizeof(*st), GFP_KERNEL); > @@ -570,13 +570,13 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf) > goto abort; > } > > - st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp", > - st, &drivetemp_chip_info, > - NULL); > - if (IS_ERR(st->hwdev)) { > - err = PTR_ERR(st->hwdev); > + hwmon_dev = > + devm_hwmon_device_register_with_info(dev->parent, "drivetemp", > + st, &drivetemp_chip_info, > + NULL); > + err = PTR_ERR_OR_ZERO(hwmon_dev); > + if (err) > goto abort; > - } > > list_add(&st->list, &drivetemp_devlist); > return 0; > @@ -593,7 +593,6 @@ static void drivetemp_remove(struct device *dev, struct class_interface *intf) > list_for_each_entry_safe(st, tmp, &drivetemp_devlist, list) { > if (st->dev == dev) { > list_del(&st->list); > - hwmon_device_unregister(st->hwdev); > kfree(st); > break; > } > With this change in place, hwmon devices are removed _after_ struct drivetemp_data is released, which means that there is a race condition where hwmon access is still possible and uses a released data structure. Besides, it is not well defined if the lifetime of the hwmon device matches the lifetime of the parent device. I don't think you know what you are doing, sorry, and I am not even going to look into the other patches of this series. Guenter
Thanks for pointing out, I think you are right. -- Jackie Liu 在 2021/12/22 上午11:03, Guenter Roeck 写道: > On 12/21/21 6:01 PM, Jackie Liu wrote: >> From: Jackie Liu <liuyun01@kylinos.cn> >> >> Use devm_hwmon_device_register_with_info() and remove hwmon_dev >> from drivetemp_data struct as it is not needed anymore. >> >> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >> --- >> drivers/hwmon/drivetemp.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c >> index 1eb37106a220..1a5a62288c0a 100644 >> --- a/drivers/hwmon/drivetemp.c >> +++ b/drivers/hwmon/drivetemp.c >> @@ -113,7 +113,6 @@ struct drivetemp_data { >> struct mutex lock; /* protect data buffer accesses */ >> struct scsi_device *sdev; /* SCSI device */ >> struct device *dev; /* instantiating device */ >> - struct device *hwdev; /* hardware monitoring device */ >> u8 smartdata[ATA_SECT_SIZE]; /* local buffer */ >> int (*get_temp)(struct drivetemp_data *st, u32 attr, long *val); >> bool have_temp_lowest; /* lowest temp in SCT status */ >> @@ -555,6 +554,7 @@ static int drivetemp_add(struct device *dev, >> struct class_interface *intf) >> { >> struct scsi_device *sdev = to_scsi_device(dev->parent); >> struct drivetemp_data *st; >> + struct device *hwmon_dev; >> int err; >> st = kzalloc(sizeof(*st), GFP_KERNEL); >> @@ -570,13 +570,13 @@ static int drivetemp_add(struct device *dev, >> struct class_interface *intf) >> goto abort; >> } >> - st->hwdev = hwmon_device_register_with_info(dev->parent, >> "drivetemp", >> - st, &drivetemp_chip_info, >> - NULL); >> - if (IS_ERR(st->hwdev)) { >> - err = PTR_ERR(st->hwdev); >> + hwmon_dev = >> + devm_hwmon_device_register_with_info(dev->parent, "drivetemp", >> + st, &drivetemp_chip_info, >> + NULL); >> + err = PTR_ERR_OR_ZERO(hwmon_dev); >> + if (err) >> goto abort; >> - } >> list_add(&st->list, &drivetemp_devlist); >> return 0; >> @@ -593,7 +593,6 @@ static void drivetemp_remove(struct device *dev, >> struct class_interface *intf) >> list_for_each_entry_safe(st, tmp, &drivetemp_devlist, list) { >> if (st->dev == dev) { >> list_del(&st->list); >> - hwmon_device_unregister(st->hwdev); >> kfree(st); >> break; >> } >> > > With this change in place, hwmon devices are removed _after_ struct > drivetemp_data > is released, which means that there is a race condition where hwmon > access is still > possible and uses a released data structure. Besides, it is not well > defined if > the lifetime of the hwmon device matches the lifetime of the parent > device. I don't > think you know what you are doing, sorry, and I am not even going to > look into the > other patches of this series. > > Guenter
diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c index 1eb37106a220..1a5a62288c0a 100644 --- a/drivers/hwmon/drivetemp.c +++ b/drivers/hwmon/drivetemp.c @@ -113,7 +113,6 @@ struct drivetemp_data { struct mutex lock; /* protect data buffer accesses */ struct scsi_device *sdev; /* SCSI device */ struct device *dev; /* instantiating device */ - struct device *hwdev; /* hardware monitoring device */ u8 smartdata[ATA_SECT_SIZE]; /* local buffer */ int (*get_temp)(struct drivetemp_data *st, u32 attr, long *val); bool have_temp_lowest; /* lowest temp in SCT status */ @@ -555,6 +554,7 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf) { struct scsi_device *sdev = to_scsi_device(dev->parent); struct drivetemp_data *st; + struct device *hwmon_dev; int err; st = kzalloc(sizeof(*st), GFP_KERNEL); @@ -570,13 +570,13 @@ static int drivetemp_add(struct device *dev, struct class_interface *intf) goto abort; } - st->hwdev = hwmon_device_register_with_info(dev->parent, "drivetemp", - st, &drivetemp_chip_info, - NULL); - if (IS_ERR(st->hwdev)) { - err = PTR_ERR(st->hwdev); + hwmon_dev = + devm_hwmon_device_register_with_info(dev->parent, "drivetemp", + st, &drivetemp_chip_info, + NULL); + err = PTR_ERR_OR_ZERO(hwmon_dev); + if (err) goto abort; - } list_add(&st->list, &drivetemp_devlist); return 0; @@ -593,7 +593,6 @@ static void drivetemp_remove(struct device *dev, struct class_interface *intf) list_for_each_entry_safe(st, tmp, &drivetemp_devlist, list) { if (st->dev == dev) { list_del(&st->list); - hwmon_device_unregister(st->hwdev); kfree(st); break; }