[PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[Jackie Liu]

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);
 }
-- 
2.25.1

[PATCH 2/5] hwmon: (nzxt-kraken2) Use devres function

[Jackie Liu]

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

[PATCH 3/5] hwmon: (corsair-psu) Use devres function

[Jackie Liu]

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

[PATCH 4/5] hwmon: (d5next) Use devres function

[Jackie Liu]

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

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

[Jackie Liu]

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

Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[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

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

[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

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

[Jackie Liu]

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

Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[Jackie Liu]

在 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

Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[Guenter Roeck]

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