| Message ID | 20211222020114.3524736-1-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 ccp_device struct as it is not needed anymore. > > Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> > --- > drivers/hwmon/corsair-cpro.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c > index fa6aa4fc8b52..f476367ba6cf 100644 > --- a/drivers/hwmon/corsair-cpro.c > +++ b/drivers/hwmon/corsair-cpro.c > @@ -76,7 +76,6 @@ > > struct ccp_device { > struct hid_device *hdev; > - struct device *hwmon_dev; > struct completion wait_input_report; > struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */ > u8 *buffer; > @@ -486,6 +485,7 @@ static int get_temp_cnct(struct ccp_device *ccp) > static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > struct ccp_device *ccp; > + struct device *hwmon_dev; > int ret; > > ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL); > @@ -523,12 +523,12 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) > ret = get_fan_cnct(ccp); > if (ret) > goto out_hw_close; > - ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro", > - ccp, &ccp_chip_info, 0); > - if (IS_ERR(ccp->hwmon_dev)) { > - ret = PTR_ERR(ccp->hwmon_dev); > + hwmon_dev = > + devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", > + ccp, &ccp_chip_info, 0); > + ret = PTR_ERR_OR_ZERO(hwmon_dev); > + if (ret) > goto out_hw_close; > - } > > return 0; > > @@ -541,9 +541,6 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) > > static void ccp_remove(struct hid_device *hdev) > { > - struct ccp_device *ccp = hid_get_drvdata(hdev); > - > - hwmon_device_unregister(ccp->hwmon_dev); > hid_hw_close(hdev); > hid_hw_stop(hdev); The point is that the above two functions need to be called _after_ the hwmon device was removed. This patch changes the order and removes the hwmon device after the hid functions have been removed. If you think this is valid you'll need to explain in detail why removal order does not matter. Otherwise this patch deserves a NACK. Guenter
在 2021/12/22 上午10:58, 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 ccp_device struct as it is not needed anymore. >> >> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >> --- >> drivers/hwmon/corsair-cpro.c | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c >> index fa6aa4fc8b52..f476367ba6cf 100644 >> --- a/drivers/hwmon/corsair-cpro.c >> +++ b/drivers/hwmon/corsair-cpro.c >> @@ -76,7 +76,6 @@ >> struct ccp_device { >> struct hid_device *hdev; >> - struct device *hwmon_dev; >> struct completion wait_input_report; >> struct mutex mutex; /* whenever buffer is used, lock before >> send_usb_cmd */ >> u8 *buffer; >> @@ -486,6 +485,7 @@ static int get_temp_cnct(struct ccp_device *ccp) >> static int ccp_probe(struct hid_device *hdev, const struct >> hid_device_id *id) >> { >> struct ccp_device *ccp; >> + struct device *hwmon_dev; >> int ret; >> ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL); >> @@ -523,12 +523,12 @@ static int ccp_probe(struct hid_device *hdev, >> const struct hid_device_id *id) >> ret = get_fan_cnct(ccp); >> if (ret) >> goto out_hw_close; >> - ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, >> "corsaircpro", >> - ccp, &ccp_chip_info, 0); >> - if (IS_ERR(ccp->hwmon_dev)) { >> - ret = PTR_ERR(ccp->hwmon_dev); >> + hwmon_dev = >> + devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", >> + ccp, &ccp_chip_info, 0); >> + ret = PTR_ERR_OR_ZERO(hwmon_dev); >> + if (ret) >> goto out_hw_close; >> - } >> return 0; >> @@ -541,9 +541,6 @@ static int ccp_probe(struct hid_device *hdev, >> const struct hid_device_id *id) >> static void ccp_remove(struct hid_device *hdev) >> { >> - struct ccp_device *ccp = hid_get_drvdata(hdev); >> - >> - hwmon_device_unregister(ccp->hwmon_dev); >> hid_hw_close(hdev); >> hid_hw_stop(hdev); > > The point is that the above two functions need to be called _after_ the > hwmon device > was removed. This patch changes the order and removes the hwmon device > after the hid > functions have been removed. > > If you think this is valid you'll need to explain in detail why removal > order > does not matter. Otherwise this patch deserves a NACK. > > Guenter > > Hi Guenter After adjusting the order here, there will be a small window for sysfs to continue to provide services. However, because hid has been disconnected, the read and write interfaces will not get the actual data and returned by timeout. IMO this is not a big issue, but it's okay to not change it. -- BR, Jackie Liu
On 12/21/21 10:13 PM, Jackie Liu wrote: > > > 在 2021/12/22 上午10:58, 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 ccp_device struct as it is not needed anymore. >>> >>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >>> --- >>> drivers/hwmon/corsair-cpro.c | 15 ++++++--------- >>> 1 file changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c >>> index fa6aa4fc8b52..f476367ba6cf 100644 >>> --- a/drivers/hwmon/corsair-cpro.c >>> +++ b/drivers/hwmon/corsair-cpro.c >>> @@ -76,7 +76,6 @@ >>> struct ccp_device { >>> struct hid_device *hdev; >>> - struct device *hwmon_dev; >>> struct completion wait_input_report; >>> struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */ >>> u8 *buffer; >>> @@ -486,6 +485,7 @@ static int get_temp_cnct(struct ccp_device *ccp) >>> static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) >>> { >>> struct ccp_device *ccp; >>> + struct device *hwmon_dev; >>> int ret; >>> ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL); >>> @@ -523,12 +523,12 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) >>> ret = get_fan_cnct(ccp); >>> if (ret) >>> goto out_hw_close; >>> - ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro", >>> - ccp, &ccp_chip_info, 0); >>> - if (IS_ERR(ccp->hwmon_dev)) { >>> - ret = PTR_ERR(ccp->hwmon_dev); >>> + hwmon_dev = >>> + devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", >>> + ccp, &ccp_chip_info, 0); >>> + ret = PTR_ERR_OR_ZERO(hwmon_dev); >>> + if (ret) >>> goto out_hw_close; >>> - } >>> return 0; >>> @@ -541,9 +541,6 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) >>> static void ccp_remove(struct hid_device *hdev) >>> { >>> - struct ccp_device *ccp = hid_get_drvdata(hdev); >>> - >>> - hwmon_device_unregister(ccp->hwmon_dev); >>> hid_hw_close(hdev); >>> hid_hw_stop(hdev); >> >> The point is that the above two functions need to be called _after_ the hwmon device >> was removed. This patch changes the order and removes the hwmon device after the hid >> functions have been removed. >> >> If you think this is valid you'll need to explain in detail why removal order >> does not matter. Otherwise this patch deserves a NACK. >> >> Guenter >> >> > > Hi Guenter > > After adjusting the order here, there will be a small window for sysfs to continue to provide services. However, because hid has been > disconnected, the read and write interfaces will not get the actual data and returned by timeout. IMO this is not a big issue, but it's okay to not change it. > That is not how kernel development works. I just hope you don't introduce such "not a big issue" problems in other areas of the kernel. Guenter
diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c index fa6aa4fc8b52..f476367ba6cf 100644 --- a/drivers/hwmon/corsair-cpro.c +++ b/drivers/hwmon/corsair-cpro.c @@ -76,7 +76,6 @@ struct ccp_device { struct hid_device *hdev; - struct device *hwmon_dev; struct completion wait_input_report; struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */ u8 *buffer; @@ -486,6 +485,7 @@ static int get_temp_cnct(struct ccp_device *ccp) static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) { struct ccp_device *ccp; + struct device *hwmon_dev; int ret; ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL); @@ -523,12 +523,12 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) ret = get_fan_cnct(ccp); if (ret) goto out_hw_close; - ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro", - ccp, &ccp_chip_info, 0); - if (IS_ERR(ccp->hwmon_dev)) { - ret = PTR_ERR(ccp->hwmon_dev); + hwmon_dev = + devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro", + ccp, &ccp_chip_info, 0); + ret = PTR_ERR_OR_ZERO(hwmon_dev); + if (ret) goto out_hw_close; - } return 0; @@ -541,9 +541,6 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id) static void ccp_remove(struct hid_device *hdev) { - struct ccp_device *ccp = hid_get_drvdata(hdev); - - hwmon_device_unregister(ccp->hwmon_dev); hid_hw_close(hdev); hid_hw_stop(hdev); }