Re: [PATCH -next 19/19] hwmon: (nzxt-smart2) Use devm_hid_hw_start_and_open in nzxt_smart2_hid_probe()

[Li Zetao <lizetao1@huawei.com>]

Hi,

在 2024/9/4 22:14, Guenter Roeck 写道:
> On 9/4/24 05:36, Li Zetao wrote:
>> Currently, the nzxt-smart2 module needs to maintain hid resources
>> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 
> For all patches:
> 
> s/Consider using/Use/
ok
> 
>> that hid resources are consistent with the device life cycle, and release
>> hid resources before device is released. At the same time, it can avoid
>> the goto-release encoding, drop the out_hw_close and out_hw_stop
>> lables, and directly return the error code when an error occurs.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   drivers/hwmon/nzxt-smart2.c | 22 +++-------------------
>>   1 file changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
>> index df6fa72a6b59..b5721a42c0d3 100644
>> --- a/drivers/hwmon/nzxt-smart2.c
>> +++ b/drivers/hwmon/nzxt-smart2.c
>> @@ -750,14 +750,10 @@ static int nzxt_smart2_hid_probe(struct 
>> hid_device *hdev,
>>       if (ret)
>>           return ret;
>> -    ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>> +    ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
>>       if (ret)
>>           return ret;
>> -    ret = hid_hw_open(hdev);
>> -    if (ret)
>> -        goto out_hw_stop;
>> -
>>       hid_device_io_start(hdev);
>>       init_device(drvdata, UPDATE_INTERVAL_DEFAULT_MS);
>> @@ -765,19 +761,10 @@ static int nzxt_smart2_hid_probe(struct 
>> hid_device *hdev,
>>       drvdata->hwmon =
>>           hwmon_device_register_with_info(&hdev->dev, "nzxtsmart2", 
>> drvdata,
>>                           &nzxt_smart2_chip_info, NULL);
>> -    if (IS_ERR(drvdata->hwmon)) {
>> -        ret = PTR_ERR(drvdata->hwmon);
>> -        goto out_hw_close;
>> -    }
>> +    if (IS_ERR(drvdata->hwmon))
>> +        return PTR_ERR(drvdata->hwmon);
>>       return 0;
> 
>      return PTR_ERR_OR_ZERO(drvdata->hwmon);
> 
> Also, this can be optimized further.
> 
>      struct device *hwmon;    // and drop from struct drvdata
>      ...
>      hwmon = devm_hwmon_device_register_with_info(&hdev->dev, 
> "nzxtsmart2", drvdata,
>                               &nzxt_smart2_chip_info, NULL);
> 
>      return PTR_ERR_OR_ZERO(hwmon);
> 
> and drop the remove function entirely.
Benjamin mentioned that there are unsafe scenarios in 
hid_hw_close_and_stop, and it is necessary to ensure that the driver can 
not use manual kzalloc/kfree. So, to be extra safe, I would delete 
.remove in v2.
> 
> Thanks,
> Guenter
> 
>> -
>> -out_hw_close:
>> -    hid_hw_close(hdev);
>> -
>> -out_hw_stop:
>> -    hid_hw_stop(hdev);
>> -    return ret;
>>   }
>>   static void nzxt_smart2_hid_remove(struct hid_device *hdev)
>> @@ -785,9 +772,6 @@ static void nzxt_smart2_hid_remove(struct 
>> hid_device *hdev)
>>       struct drvdata *drvdata = hid_get_drvdata(hdev);
>>       hwmon_device_unregister(drvdata->hwmon);
>> -
>> -    hid_hw_close(hdev);
>> -    hid_hw_stop(hdev);
>>   }
>>   static const struct hid_device_id nzxt_smart2_hid_id_table[] = {
>