* [PATCH 2/5] hwmon: (nzxt-kraken2) Use devres function
2021-12-22 2:01 [PATCH 1/5] hwmon: (corsair-cpro) Use devres function Jackie Liu
@ 2021-12-22 2:01 ` Jackie Liu
2021-12-22 2:01 ` [PATCH 3/5] hwmon: (corsair-psu) " Jackie Liu
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Jackie Liu @ 2021-12-22 2:01 UTC (permalink / raw)
To: linux; +Cc: jdelvare, linux-hwmon, liu.yun
From: Jackie Liu <liuyun01@kylinos.cn>
Use devm_hwmon_device_register_with_info() and remove hwmon_dev
from kraken2_priv_data struct as it is not needed anymore.
Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
---
drivers/hwmon/nzxt-kraken2.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
index 89f7ea4f42d4..cd644ca12fb4 100644
--- a/drivers/hwmon/nzxt-kraken2.c
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -29,7 +29,6 @@ static const char *const kraken2_fan_label[] = {
struct kraken2_priv_data {
struct hid_device *hid_dev;
- struct device *hwmon_dev;
s32 temp_input[1];
u16 fan_input[2];
unsigned long updated; /* jiffies */
@@ -133,6 +132,7 @@ static int kraken2_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
struct kraken2_priv_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -170,11 +170,12 @@ static int kraken2_probe(struct hid_device *hdev,
goto fail_and_close;
}
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
- priv, &kraken2_chip_info,
- NULL);
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
+ hwmon_dev =
+ devm_hwmon_device_register_with_info(&hdev->dev, "kraken2",
+ priv, &kraken2_chip_info,
+ NULL);
+ ret = PTR_ERR_OR_ZERO(hwmon_dev);
+ if (ret) {
hid_err(hdev, "hwmon registration failed with %d\n", ret);
goto fail_and_close;
}
@@ -190,10 +191,6 @@ static int kraken2_probe(struct hid_device *hdev,
static void kraken2_remove(struct hid_device *hdev)
{
- struct kraken2_priv_data *priv = hid_get_drvdata(hdev);
-
- hwmon_device_unregister(priv->hwmon_dev);
-
hid_hw_close(hdev);
hid_hw_stop(hdev);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/5] hwmon: (corsair-psu) Use devres function
2021-12-22 2:01 [PATCH 1/5] hwmon: (corsair-cpro) Use devres function Jackie Liu
2021-12-22 2:01 ` [PATCH 2/5] hwmon: (nzxt-kraken2) " Jackie Liu
@ 2021-12-22 2:01 ` Jackie Liu
2021-12-22 2:01 ` [PATCH 4/5] hwmon: (d5next) " Jackie Liu
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Jackie Liu @ 2021-12-22 2:01 UTC (permalink / raw)
To: linux; +Cc: jdelvare, linux-hwmon, liu.yun
From: Jackie Liu <liuyun01@kylinos.cn>
Use devm_hwmon_device_register_with_info() and remove hwmon_dev
from corsairpsu_data struct as it is not needed anymore.
Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
---
drivers/hwmon/corsair-psu.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index 14389fd7afb8..1a40cde246aa 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -115,7 +115,6 @@ static const char *const label_amps[] = {
struct corsairpsu_data {
struct hid_device *hdev;
- struct device *hwmon_dev;
struct dentry *debugfs;
struct completion wait_completion;
struct mutex lock; /* for locking access to cmd_buffer */
@@ -684,6 +683,7 @@ static void corsairpsu_debugfs_init(struct corsairpsu_data *priv)
static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct corsairpsu_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(struct corsairpsu_data), GFP_KERNEL);
@@ -728,13 +728,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
corsairpsu_get_criticals(priv);
corsairpsu_check_cmd_support(priv);
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
- &corsairpsu_chip_info, NULL);
-
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
+ hwmon_dev =
+ devm_hwmon_device_register_with_info(&hdev->dev, "corsairpsu",
+ priv, &corsairpsu_chip_info,
+ NULL);
+ ret = PTR_ERR_OR_ZERO(hwmon_dev);
+ if (ret)
goto fail_and_close;
- }
corsairpsu_debugfs_init(priv);
@@ -752,7 +752,6 @@ static void corsairpsu_remove(struct hid_device *hdev)
struct corsairpsu_data *priv = hid_get_drvdata(hdev);
debugfs_remove_recursive(priv->debugfs);
- hwmon_device_unregister(priv->hwmon_dev);
hid_hw_close(hdev);
hid_hw_stop(hdev);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/5] hwmon: (d5next) Use devres function
2021-12-22 2:01 [PATCH 1/5] hwmon: (corsair-cpro) Use devres function Jackie Liu
2021-12-22 2:01 ` [PATCH 2/5] hwmon: (nzxt-kraken2) " Jackie Liu
2021-12-22 2:01 ` [PATCH 3/5] hwmon: (corsair-psu) " Jackie Liu
@ 2021-12-22 2:01 ` Jackie Liu
2021-12-22 2:01 ` [PATCH 5/5] hwmon: (drivetemp) " Jackie Liu
2021-12-22 2:58 ` [PATCH 1/5] hwmon: (corsair-cpro) " Guenter Roeck
4 siblings, 0 replies; 10+ messages in thread
From: Jackie Liu @ 2021-12-22 2:01 UTC (permalink / raw)
To: linux; +Cc: jdelvare, linux-hwmon, liu.yun
From: Jackie Liu <liuyun01@kylinos.cn>
Use devm_hwmon_device_register_with_info() and remove hwmon_dev
from d5next_data struct as it is not needed anymore.
Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
---
drivers/hwmon/aquacomputer_d5next.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index fb9341a53051..4f6f11169a53 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -84,7 +84,6 @@ static const char *const label_current[] = {
struct d5next_data {
struct hid_device *hdev;
- struct device *hwmon_dev;
struct dentry *debugfs;
s32 temp_input;
u16 speed_input[2];
@@ -275,6 +274,7 @@ static void d5next_debugfs_init(struct d5next_data *priv)
static int d5next_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct d5next_data *priv;
+ struct device *hwmon_dev;
int ret;
priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -298,13 +298,12 @@ static int d5next_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (ret)
goto fail_and_stop;
- priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "d5next", priv,
- &d5next_chip_info, NULL);
-
- if (IS_ERR(priv->hwmon_dev)) {
- ret = PTR_ERR(priv->hwmon_dev);
+ hwmon_dev =
+ devm_hwmon_device_register_with_info(&hdev->dev, "d5next", priv,
+ &d5next_chip_info, NULL);
+ ret = PTR_ERR_OR_ZERO(hwmon_dev);
+ if (ret)
goto fail_and_close;
- }
d5next_debugfs_init(priv);
@@ -322,7 +321,6 @@ static void d5next_remove(struct hid_device *hdev)
struct d5next_data *priv = hid_get_drvdata(hdev);
debugfs_remove_recursive(priv->debugfs);
- hwmon_device_unregister(priv->hwmon_dev);
hid_hw_close(hdev);
hid_hw_stop(hdev);
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5/5] hwmon: (drivetemp) Use devres function
2021-12-22 2:01 [PATCH 1/5] hwmon: (corsair-cpro) Use devres function Jackie Liu
` (2 preceding siblings ...)
2021-12-22 2:01 ` [PATCH 4/5] hwmon: (d5next) " Jackie Liu
@ 2021-12-22 2:01 ` Jackie Liu
2021-12-22 3:03 ` Guenter Roeck
2021-12-22 2:58 ` [PATCH 1/5] hwmon: (corsair-cpro) " Guenter Roeck
4 siblings, 1 reply; 10+ messages in thread
From: Jackie Liu @ 2021-12-22 2:01 UTC (permalink / raw)
To: linux; +Cc: jdelvare, linux-hwmon, liu.yun
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 5/5] hwmon: (drivetemp) Use devres function
2021-12-22 2:01 ` [PATCH 5/5] hwmon: (drivetemp) " Jackie Liu
@ 2021-12-22 3:03 ` Guenter Roeck
2021-12-22 5:43 ` Jackie Liu
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2021-12-22 3:03 UTC (permalink / raw)
To: Jackie Liu; +Cc: jdelvare, linux-hwmon
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 5/5] hwmon: (drivetemp) Use devres function
2021-12-22 3:03 ` Guenter Roeck
@ 2021-12-22 5:43 ` Jackie Liu
0 siblings, 0 replies; 10+ messages in thread
From: Jackie Liu @ 2021-12-22 5:43 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function
2021-12-22 2:01 [PATCH 1/5] hwmon: (corsair-cpro) Use devres function Jackie Liu
` (3 preceding siblings ...)
2021-12-22 2:01 ` [PATCH 5/5] hwmon: (drivetemp) " Jackie Liu
@ 2021-12-22 2:58 ` Guenter Roeck
2021-12-22 6:13 ` Jackie Liu
4 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2021-12-22 2:58 UTC (permalink / raw)
To: Jackie Liu; +Cc: jdelvare, linux-hwmon
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function
2021-12-22 2:58 ` [PATCH 1/5] hwmon: (corsair-cpro) " Guenter Roeck
@ 2021-12-22 6:13 ` Jackie Liu
2021-12-22 16:10 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Jackie Liu @ 2021-12-22 6:13 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon
在 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function
2021-12-22 6:13 ` Jackie Liu
@ 2021-12-22 16:10 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2021-12-22 16:10 UTC (permalink / raw)
To: Jackie Liu; +Cc: jdelvare, linux-hwmon
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
^ permalink raw reply [flat|nested] 10+ messages in thread