Re: [PATCH 5/5] hwmon: (drivetemp) Use devres function

[Jackie Liu <liu.yun@linux.dev>]

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