[PATCH -next 00/19] HID: convert to devm_hid_hw_start_and_open()

[PATCH -next 00/19] HID: convert to devm_hid_hw_start_and_open()

[Li Zetao]

Hi, all

This patchset is dedicated to using the life cycle approach to manage
hid resources. By keeping hid resources consistent with the life cycle
of the device, we ensure that resources are available during the life
cycle and the hid resources can be released before device release.

Going one step further, since the module does not need to recycle hid
resources by itself, the goto-release resource release coding can be
avoided. It also reduces the risk of resources not being released.

Li Zetao (19):
  HID: core: Use devm_add_action_or_reset helper to manage hid resources
  HID: cp2112: Use devm_hid_hw_start_and_open in cp2112_probe()
  HID: ft260: Use devm_hid_hw_start_and_open in ft260_probe()
  HID: mcp2200: Use devm_hid_hw_start_and_open in mcp2200_probe()
  HID: mcp2221: Use devm_hid_hw_start_and_open in mcp2221_probe()
  HID: nintendo: Use devm_hid_hw_start_and_open in nintendo_hid_probe()
  HID: shield: Use devm_hid_hw_start_and_open in shield_probe()
  HID: hid-picolcd: Use devm_hid_hw_start_and_open in picolcd_probe()
  HID: playstation: Use devm_hid_hw_start_and_open in ps_probe()
  HID: hid-steam: Use devm_hid_hw_start_and_open in steam_probe()
  HID: wiimote: Use devm_hid_hw_start_and_open in wiimote_hid_probe()
  hwmon: (aquacomputer_d5next) Use devm_hid_hw_start_and_open in
    aqc_probe()
  hwmon: Use devm_hid_hw_start_and_open in rog_ryujin_probe()
  hwmon: (corsair-cpro) Use devm_hid_hw_start_and_open in ccp_probe()
  hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in
    corsairpsu_probe()
  hwmon: (gigabyte_waterforce) Use devm_hid_hw_start_and_open in
    waterforce_probe()
  hwmon: (nzxt-kraken2) Use devm_hid_hw_start_and_open in
    kraken2_probe()
  hwmon: (nzxt-kraken3) Use devm_hid_hw_start_and_open in
    kraken3_probe()
  hwmon: (nzxt-smart2) Use devm_hid_hw_start_and_open in
    nzxt_smart2_hid_probe()

 drivers/hid/hid-core.c              | 40 +++++++++++++++++++++++++++++
 drivers/hid/hid-cp2112.c            | 26 +++----------------
 drivers/hid/hid-ft260.c             | 32 +++++------------------
 drivers/hid/hid-mcp2200.c           | 22 ++--------------
 drivers/hid/hid-mcp2221.c           | 26 ++-----------------
 drivers/hid/hid-nintendo.c          | 23 +++--------------
 drivers/hid/hid-nvidia-shield.c     | 20 +++------------
 drivers/hid/hid-picolcd_core.c      | 22 +++-------------
 drivers/hid/hid-playstation.c       | 27 +++----------------
 drivers/hid/hid-steam.c             | 18 ++-----------
 drivers/hid/hid-wiimote-core.c      | 20 +++------------
 drivers/hwmon/aquacomputer_d5next.c | 39 +++++++---------------------
 drivers/hwmon/asus_rog_ryujin.c     | 29 ++++-----------------
 drivers/hwmon/corsair-cpro.c        | 24 ++++-------------
 drivers/hwmon/corsair-psu.c         | 24 ++++-------------
 drivers/hwmon/gigabyte_waterforce.c | 29 ++++-----------------
 drivers/hwmon/nzxt-kraken2.c        | 23 +++--------------
 drivers/hwmon/nzxt-kraken3.c        | 34 +++++-------------------
 drivers/hwmon/nzxt-smart2.c         | 22 +++-------------
 include/linux/hid.h                 |  2 ++
 20 files changed, 118 insertions(+), 384 deletions(-)

-- 
2.34.1

[PATCH -next 01/19] HID: core: Use devm_add_action_or_reset helper to manage hid resources

[Li Zetao]

By adding a custom action to the device, it can bind the hid resource
to the hid_device life cycle. The framework automatically close and stop
the hid resources before hid_device is released, and the users do not
need to pay attention to the timing of hid resource release.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h    |  2 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 30de92d0bf0f..71143c0a4a02 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev)
 }
 EXPORT_SYMBOL_GPL(hid_hw_close);
 
+static void hid_hw_close_and_stop(void *data)
+{
+	struct hid_device *hdev = data;
+
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+/**
+ * devm_hid_hw_start_and_open - manage hid resources through custom action
+ *
+ * @hdev: hid device
+ * @connect_mask: which outputs to connect, see HID_CONNECT_*
+ *
+ * Bind the hid resource to the hid_device life cycle and register
+ * an action to release the hid resource. The users do not need to
+ * pay attention to the release of hid.
+ */
+
+int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
+{
+	int ret;
+
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "hw start failed with %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev, "hw open failed with %d\n", ret);
+		hid_hw_stop(hdev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
+}
+EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
+
 /**
  * hid_hw_request - send report request to device
  *
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 121d5b8bc867..0ce217aa5f62 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
 void hid_hw_stop(struct hid_device *hdev);
 int __must_check hid_hw_open(struct hid_device *hdev);
 void hid_hw_close(struct hid_device *hdev);
+int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
+					    unsigned int connect_mask);
 void hid_hw_request(struct hid_device *hdev,
 		    struct hid_report *report, enum hid_class_request reqtype);
 int __hid_hw_raw_request(struct hid_device *hdev,
-- 
2.34.1

Re: [PATCH -next 01/19] HID: core: Use devm_add_action_or_reset helper to manage hid resources

[Benjamin Tissoires]

On Sep 04 2024, Li Zetao wrote:
> By adding a custom action to the device, it can bind the hid resource
> to the hid_device life cycle. The framework automatically close and stop
> the hid resources before hid_device is released, and the users do not
> need to pay attention to the timing of hid resource release.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/hid.h    |  2 ++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 30de92d0bf0f..71143c0a4a02 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev)
>  }
>  EXPORT_SYMBOL_GPL(hid_hw_close);
>  
> +static void hid_hw_close_and_stop(void *data)
> +{
> +	struct hid_device *hdev = data;
> +
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +/**
> + * devm_hid_hw_start_and_open - manage hid resources through custom action
> + *
> + * @hdev: hid device
> + * @connect_mask: which outputs to connect, see HID_CONNECT_*
> + *
> + * Bind the hid resource to the hid_device life cycle and register
> + * an action to release the hid resource. The users do not need to
> + * pay attention to the release of hid.

The only problem I see here is that this API is not completely "safe",
in the sense that a driver using it can not use manual kzalloc/kfree.
It is obvious, but probably not so much while looking at the comments
from Guenter where he asked you to also remove the .remove() callback.

So in the docs, we should probably state that if the .remove() is any
different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be
use with that function.

Cheers,
Benjamin

> + */
> +
> +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
> +{
> +	int ret;
> +
> +	ret = hid_hw_start(hdev, connect_mask);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed with %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hw open failed with %d\n", ret);
> +		hid_hw_stop(hdev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
> +}
> +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
> +
>  /**
>   * hid_hw_request - send report request to device
>   *
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 121d5b8bc867..0ce217aa5f62 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
>  void hid_hw_stop(struct hid_device *hdev);
>  int __must_check hid_hw_open(struct hid_device *hdev);
>  void hid_hw_close(struct hid_device *hdev);
> +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
> +					    unsigned int connect_mask);
>  void hid_hw_request(struct hid_device *hdev,
>  		    struct hid_report *report, enum hid_class_request reqtype);
>  int __hid_hw_raw_request(struct hid_device *hdev,
> -- 
> 2.34.1
> 

Re: [PATCH -next 01/19] HID: core: Use devm_add_action_or_reset helper to manage hid resources

[Li Zetao]

Hi,

在 2024/9/5 20:53, Benjamin Tissoires 写道:
> On Sep 04 2024, Li Zetao wrote:
>> By adding a custom action to the device, it can bind the hid resource
>> to the hid_device life cycle. The framework automatically close and stop
>> the hid resources before hid_device is released, and the users do not
>> need to pay attention to the timing of hid resource release.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>>   drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/hid.h    |  2 ++
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 30de92d0bf0f..71143c0a4a02 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev)
>>   }
>>   EXPORT_SYMBOL_GPL(hid_hw_close);
>>   
>> +static void hid_hw_close_and_stop(void *data)
>> +{
>> +	struct hid_device *hdev = data;
>> +
>> +	hid_hw_close(hdev);
>> +	hid_hw_stop(hdev);
>> +}
>> +
>> +/**
>> + * devm_hid_hw_start_and_open - manage hid resources through custom action
>> + *
>> + * @hdev: hid device
>> + * @connect_mask: which outputs to connect, see HID_CONNECT_*
>> + *
>> + * Bind the hid resource to the hid_device life cycle and register
>> + * an action to release the hid resource. The users do not need to
>> + * pay attention to the release of hid.
> 
> The only problem I see here is that this API is not completely "safe",
> in the sense that a driver using it can not use manual kzalloc/kfree.
> It is obvious, but probably not so much while looking at the comments
> from Guenter where he asked you to also remove the .remove() callback.
> 
> So in the docs, we should probably state that if the .remove() is any
> different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be
> use with that function.I'll add some comments to illustrate a scenario like this.

> 
> Cheers,
> Benjamin
> 
>> + */
>> +
>> +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
>> +{
>> +	int ret;
>> +
>> +	ret = hid_hw_start(hdev, connect_mask);
>> +	if (ret) {
>> +		hid_err(hdev, "hw start failed with %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = hid_hw_open(hdev);
>> +	if (ret) {
>> +		hid_err(hdev, "hw open failed with %d\n", ret);
>> +		hid_hw_stop(hdev);
>> +		return ret;
>> +	}
>> +
>> +	return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
>> +
>>   /**
>>    * hid_hw_request - send report request to device
>>    *
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 121d5b8bc867..0ce217aa5f62 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
>>   void hid_hw_stop(struct hid_device *hdev);
>>   int __must_check hid_hw_open(struct hid_device *hdev);
>>   void hid_hw_close(struct hid_device *hdev);
>> +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
>> +					    unsigned int connect_mask);
>>   void hid_hw_request(struct hid_device *hdev,
>>   		    struct hid_report *report, enum hid_class_request reqtype);
>>   int __hid_hw_raw_request(struct hid_device *hdev,
>> -- 
>> 2.34.1
>>

Thanks,
Li Zetao.

[PATCH -next 02/19] HID: cp2112: Use devm_hid_hw_start_and_open in cp2112_probe()

[Li Zetao]

Currently, the cp2112 module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_hid_close and err_hid_stop
lables.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-cp2112.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 20a0d1315d90..6d65c65f1b83 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1215,22 +1215,14 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "hw start failed\n");
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "hw open failed\n");
-		goto err_hid_stop;
-	}
 
 	ret = hid_hw_power(hdev, PM_HINT_FULLON);
 	if (ret < 0) {
 		hid_err(hdev, "power management error: %d\n", ret);
-		goto err_hid_close;
+		return ret;
 	}
 
 	ret = cp2112_hid_get(hdev, CP2112_GET_VERSION_INFO, buf, sizeof(buf),
@@ -1334,10 +1326,6 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	i2c_del_adapter(&dev->adap);
 err_power_normal:
 	hid_hw_power(hdev, PM_HINT_NORMAL);
-err_hid_close:
-	hid_hw_close(hdev);
-err_hid_stop:
-	hid_hw_stop(hdev);
 	return ret;
 }
 
@@ -1354,14 +1342,6 @@ static void cp2112_remove(struct hid_device *hdev)
 	}
 
 	gpiochip_remove(&dev->gc);
-	/* i2c_del_adapter has finished removing all i2c devices from our
-	 * adapter. Well behaved devices should no longer call our cp2112_xfer
-	 * and should have waited for any pending calls to finish. It has also
-	 * waited for device_unregister(&adap->dev) to complete. Therefore we
-	 * can safely free our struct cp2112_device.
-	 */
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
-- 
2.34.1

[PATCH -next 03/19] HID: ft260: Use devm_hid_hw_start_and_open in ft260_probe()

[Li Zetao]

Currently, the ft260 module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_hid_close, err_hid_stop
and err_i2c_free lables, and directly return the error code when an
error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-ft260.c | 32 +++++++-------------------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 333341e80b0e..272165ebf46c 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -976,23 +976,15 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, 0);
-	if (ret) {
-		hid_err(hdev, "failed to start HID HW\n");
+	ret = devm_hid_hw_start_and_open(hdev, 0);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "failed to open HID HW\n");
-		goto err_hid_stop;
-	}
 
 	ret = ft260_hid_feature_report_get(hdev, FT260_CHIP_VERSION,
 					   (u8 *)&version, sizeof(version));
 	if (ret < 0) {
 		hid_err(hdev, "failed to retrieve chip version\n");
-		goto err_hid_close;
+		return ret;
 	}
 
 	hid_info(hdev, "chip code: %02x%02x %02x%02x\n",
@@ -1001,7 +993,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ret = ft260_is_interface_enabled(hdev);
 	if (ret <= 0)
-		goto err_hid_close;
+		return ret;
 
 	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n",
 		hdev->version >> 8, hdev->version & 0xff, hdev->name,
@@ -1028,24 +1020,17 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	ret = i2c_add_adapter(&dev->adap);
 	if (ret) {
 		hid_err(hdev, "failed to add i2c adapter\n");
-		goto err_hid_close;
+		return ret;
 	}
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &ft260_attr_group);
 	if (ret < 0) {
 		hid_err(hdev, "failed to create sysfs attrs\n");
-		goto err_i2c_free;
+		i2c_del_adapter(&dev->adap);
+		return ret;
 	}
 
 	return 0;
-
-err_i2c_free:
-	i2c_del_adapter(&dev->adap);
-err_hid_close:
-	hid_hw_close(hdev);
-err_hid_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void ft260_remove(struct hid_device *hdev)
@@ -1057,9 +1042,6 @@ static void ft260_remove(struct hid_device *hdev)
 
 	sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
 	i2c_del_adapter(&dev->adap);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
-- 
2.34.1

[PATCH -next 04/19] HID: mcp2200: Use devm_hid_hw_start_and_open in mcp2200_probe()

[Li Zetao]

Currently, the mcp2200 module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
that hid resources are consistent with the device life cycle, and release
hid resources before device is released. So there is no need to close and
stop hid when an error occurs. At the same time, since there is no need to
do any operations in mcp2200_remove() now, so delete .remote operation.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-mcp2200.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
index bf57f7f6caa0..56d72fc5623d 100644
--- a/drivers/hid/hid-mcp2200.c
+++ b/drivers/hid/hid-mcp2200.c
@@ -329,22 +329,13 @@ static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, 0);
-	if (ret) {
-		hid_err(hdev, "can't start hardware\n");
+	ret = devm_hid_hw_start_and_open(hdev, 0);
+	if (ret)
 		return ret;
-	}
 
 	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
 			hdev->version & 0xff, hdev->name, hdev->phys);
 
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "can't open device\n");
-		hid_hw_stop(hdev);
-		return ret;
-	}
-
 	mutex_init(&mcp->lock);
 	init_completion(&mcp->wait_in_report);
 	hid_set_drvdata(hdev, mcp);
@@ -356,20 +347,12 @@ static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id
 	ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
 	if (ret < 0) {
 		hid_err(hdev, "Unable to register gpiochip\n");
-		hid_hw_close(hdev);
-		hid_hw_stop(hdev);
 		return ret;
 	}
 
 	return 0;
 }
 
-static void mcp2200_remove(struct hid_device *hdev)
-{
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
-}
-
 static const struct hid_device_id mcp2200_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
 	{ }
@@ -380,7 +363,6 @@ static struct hid_driver mcp2200_driver = {
 	.name		= "mcp2200",
 	.id_table	= mcp2200_devices,
 	.probe		= mcp2200_probe,
-	.remove		= mcp2200_remove,
 	.raw_event	= mcp2200_raw_event,
 };
 
-- 
2.34.1

[PATCH -next 05/19] HID: mcp2221: Use devm_hid_hw_start_and_open in mcp2221_probe()

[Li Zetao]

Currently, the mcp2221 module use devm_add_action_or_reset() to manage
device resource for HID unregistration, now that a universal interface
has been provided, consider using a universal interface to replace it.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-mcp2221.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 0f93c22a479f..3b8269f7e923 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -932,15 +932,6 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 	return 1;
 }
 
-/* Device resource managed function for HID unregistration */
-static void mcp2221_hid_unregister(void *ptr)
-{
-	struct hid_device *hdev = ptr;
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
-}
-
 /* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
 static void mcp2221_remove(struct hid_device *hdev)
 {
@@ -1141,31 +1132,18 @@ static int mcp2221_probe(struct hid_device *hdev,
 	 * This driver uses the .raw_event callback and therefore does not need any
 	 * HID_CONNECT_xxx flags.
 	 */
-	ret = hid_hw_start(hdev, 0);
-	if (ret) {
-		hid_err(hdev, "can't start hardware\n");
+	ret = devm_hid_hw_start_and_open(hdev, 0);
+	if (ret)
 		return ret;
-	}
 
 	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
 			hdev->version & 0xff, hdev->name, hdev->phys);
 
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "can't open device\n");
-		hid_hw_stop(hdev);
-		return ret;
-	}
-
 	mutex_init(&mcp->lock);
 	init_completion(&mcp->wait_in_report);
 	hid_set_drvdata(hdev, mcp);
 	mcp->hdev = hdev;
 
-	ret = devm_add_action_or_reset(&hdev->dev, mcp2221_hid_unregister, hdev);
-	if (ret)
-		return ret;
-
 	hid_device_io_start(hdev);
 
 	/* Set I2C bus clock diviser */
-- 
2.34.1

[PATCH -next 06/19] HID: nintendo: Use devm_hid_hw_start_and_open in nintendo_hid_probe()

[Li Zetao]

Currently, the nintendo module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_close and err_stop lables.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-nintendo.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 58cd0506e431..45ac4fd3c7ea 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2673,31 +2673,23 @@ static int nintendo_hid_probe(struct hid_device *hdev,
 	 */
 	hdev->version |= 0x8000;
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "HW start failed\n");
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		goto err_wq;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "cannot start hardware I/O\n");
-		goto err_stop;
-	}
 
 	hid_device_io_start(hdev);
 
 	ret = joycon_init(hdev);
 	if (ret) {
 		hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
-		goto err_close;
+		goto err_wq;
 	}
 
 	/* Initialize the leds */
 	ret = joycon_leds_create(ctlr);
 	if (ret) {
 		hid_err(hdev, "Failed to create leds; ret=%d\n", ret);
-		goto err_close;
+		goto err_wq;
 	}
 
 	/* Initialize the battery power supply */
@@ -2720,10 +2712,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
 
 err_ida:
 	ida_free(&nintendo_player_id_allocator, ctlr->player_id);
-err_close:
-	hid_hw_close(hdev);
-err_stop:
-	hid_hw_stop(hdev);
 err_wq:
 	destroy_workqueue(ctlr->rumble_queue);
 err:
@@ -2745,9 +2733,6 @@ static void nintendo_hid_remove(struct hid_device *hdev)
 
 	destroy_workqueue(ctlr->rumble_queue);
 	ida_free(&nintendo_player_id_allocator, ctlr->player_id);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 #ifdef CONFIG_PM
-- 
2.34.1

[PATCH -next 07/19] HID: shield: Use devm_hid_hw_start_and_open in shield_probe()

[Li Zetao]

Currently, the shield module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_stop and err_ts_create lables, and
directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-nvidia-shield.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index ff9078ad1961..747996a21dd9 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -1070,27 +1070,15 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ts = container_of(shield_dev, struct thunderstrike, base);
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDINPUT);
 	if (ret) {
-		hid_err(hdev, "Failed to start HID device\n");
-		goto err_ts_create;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "Failed to open HID device\n");
-		goto err_stop;
+		thunderstrike_destroy(ts);
+		return ret;
 	}
 
 	thunderstrike_device_init_info(shield_dev);
 
 	return ret;
-
-err_stop:
-	hid_hw_stop(hdev);
-err_ts_create:
-	thunderstrike_destroy(ts);
-	return ret;
 }
 
 static void shield_remove(struct hid_device *hdev)
@@ -1100,11 +1088,9 @@ static void shield_remove(struct hid_device *hdev)
 
 	ts = container_of(dev, struct thunderstrike, base);
 
-	hid_hw_close(hdev);
 	thunderstrike_destroy(ts);
 	del_timer_sync(&ts->psy_stats_timer);
 	cancel_work_sync(&ts->hostcmd_req_work);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id shield_devices[] = {
-- 
2.34.1

Re: [PATCH -next 07/19] HID: shield: Use devm_hid_hw_start_and_open in shield_probe()

[Benjamin Tissoires]

On Sep 04 2024, Li Zetao wrote:
> Currently, the shield module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 err_stop and err_ts_create lables, and
> directly return the error code when an error occurs.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hid/hid-nvidia-shield.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
> index ff9078ad1961..747996a21dd9 100644
> --- a/drivers/hid/hid-nvidia-shield.c
> +++ b/drivers/hid/hid-nvidia-shield.c
> @@ -1070,27 +1070,15 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	ts = container_of(shield_dev, struct thunderstrike, base);
>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDINPUT);
>  	if (ret) {
> -		hid_err(hdev, "Failed to start HID device\n");
> -		goto err_ts_create;
> -	}
> -
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev, "Failed to open HID device\n");
> -		goto err_stop;
> +		thunderstrike_destroy(ts);
> +		return ret;
>  	}
>  
>  	thunderstrike_device_init_info(shield_dev);
>  
>  	return ret;
> -
> -err_stop:
> -	hid_hw_stop(hdev);
> -err_ts_create:
> -	thunderstrike_destroy(ts);
> -	return ret;
>  }
>  
>  static void shield_remove(struct hid_device *hdev)
> @@ -1100,11 +1088,9 @@ static void shield_remove(struct hid_device *hdev)
>  
>  	ts = container_of(dev, struct thunderstrike, base);
>  
> -	hid_hw_close(hdev);
>  	thunderstrike_destroy(ts);
>  	del_timer_sync(&ts->psy_stats_timer);
>  	cancel_work_sync(&ts->hostcmd_req_work);
> -	hid_hw_stop(hdev);

There is definitely something fishy here. The 2 calls to hid_hw_close
and hid_hw_stop are not one after the other and at the very end of the
.remove(). The patch is changing the behavior, and this might be an
issue.

Cheers,
Benjamin

>  }
>  
>  static const struct hid_device_id shield_devices[] = {
> -- 
> 2.34.1
> 

[PATCH -next 08/19] HID: hid-picolcd: Use devm_hid_hw_start_and_open in picolcd_probe()

[Li Zetao]

Currently, the hid-picolcd module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_cleanup_hid_ll and
err_cleanup_hid_hw lables.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-picolcd_core.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 297103be3381..973822d1b2db 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -551,23 +551,15 @@ static int picolcd_probe(struct hid_device *hdev,
 		goto err_cleanup_data;
 	}
 
-	error = hid_hw_start(hdev, 0);
+	error = devm_hid_hw_start_and_open(hdev, 0);
 	if (error) {
-		hid_err(hdev, "hardware start failed\n");
+		hid_err(hdev, "hardware start and open failed\n");
 		goto err_cleanup_data;
 	}
 
-	error = hid_hw_open(hdev);
-	if (error) {
-		hid_err(hdev, "failed to open input interrupt pipe for key and IR events\n");
-		goto err_cleanup_hid_hw;
-	}
-
 	error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
-	if (error) {
-		hid_err(hdev, "failed to create sysfs attributes\n");
-		goto err_cleanup_hid_ll;
-	}
+	if (error)
+		goto err_cleanup_data;
 
 	error = device_create_file(&hdev->dev, &dev_attr_operation_mode);
 	if (error) {
@@ -589,10 +581,6 @@ static int picolcd_probe(struct hid_device *hdev,
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
 err_cleanup_sysfs1:
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
-err_cleanup_hid_ll:
-	hid_hw_close(hdev);
-err_cleanup_hid_hw:
-	hid_hw_stop(hdev);
 err_cleanup_data:
 	kfree(data);
 	return error;
@@ -611,8 +599,6 @@ static void picolcd_remove(struct hid_device *hdev)
 	picolcd_exit_devfs(data);
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 
 	/* Shortcut potential pending reply that will never arrive */
 	spin_lock_irqsave(&data->lock, flags);
-- 
2.34.1

Re: [PATCH -next 08/19] HID: hid-picolcd: Use devm_hid_hw_start_and_open in picolcd_probe()

[Benjamin Tissoires]

On Sep 04 2024, Li Zetao wrote:
> Currently, the hid-picolcd module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 err_cleanup_hid_ll and
> err_cleanup_hid_hw lables.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hid/hid-picolcd_core.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 297103be3381..973822d1b2db 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -551,23 +551,15 @@ static int picolcd_probe(struct hid_device *hdev,
>  		goto err_cleanup_data;
>  	}
>  
> -	error = hid_hw_start(hdev, 0);
> +	error = devm_hid_hw_start_and_open(hdev, 0);
>  	if (error) {
> -		hid_err(hdev, "hardware start failed\n");
> +		hid_err(hdev, "hardware start and open failed\n");
>  		goto err_cleanup_data;
>  	}
>  
> -	error = hid_hw_open(hdev);
> -	if (error) {
> -		hid_err(hdev, "failed to open input interrupt pipe for key and IR events\n");
> -		goto err_cleanup_hid_hw;
> -	}
> -
>  	error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
> -	if (error) {
> -		hid_err(hdev, "failed to create sysfs attributes\n");
> -		goto err_cleanup_hid_ll;
> -	}
> +	if (error)
> +		goto err_cleanup_data;
>  
>  	error = device_create_file(&hdev->dev, &dev_attr_operation_mode);
>  	if (error) {
> @@ -589,10 +581,6 @@ static int picolcd_probe(struct hid_device *hdev,
>  	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
>  err_cleanup_sysfs1:
>  	device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
> -err_cleanup_hid_ll:
> -	hid_hw_close(hdev);
> -err_cleanup_hid_hw:
> -	hid_hw_stop(hdev);
>  err_cleanup_data:
>  	kfree(data);
>  	return error;
> @@ -611,8 +599,6 @@ static void picolcd_remove(struct hid_device *hdev)
>  	picolcd_exit_devfs(data);
>  	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
>  	device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);

Again, this doesn't seem correct. the spin_lock from below expects the
hw to be stopped, and this patch changes that by postponing the stop
after the remove.

CHeers,
Benjamin

>  
>  	/* Shortcut potential pending reply that will never arrive */
>  	spin_lock_irqsave(&data->lock, flags);
> -- 
> 2.34.1
> 

[PATCH -next 09/19] HID: playstation: Use devm_hid_hw_start_and_open in ps_probe()

[Li Zetao]

Currently, the playstation module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_close and err_stop lables, and
directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-playstation.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 0d90d7ee693c..6dddb9451a37 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2704,41 +2704,25 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "Failed to start HID device\n");
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "Failed to open HID device\n");
-		goto err_stop;
-	}
 
 	if (id->driver_data == PS_TYPE_PS4_DUALSHOCK4) {
 		dev = dualshock4_create(hdev);
 		if (IS_ERR(dev)) {
 			hid_err(hdev, "Failed to create dualshock4.\n");
-			ret = PTR_ERR(dev);
-			goto err_close;
+			return PTR_ERR(dev);
 		}
 	} else if (id->driver_data == PS_TYPE_PS5_DUALSENSE) {
 		dev = dualsense_create(hdev);
 		if (IS_ERR(dev)) {
 			hid_err(hdev, "Failed to create dualsense.\n");
-			ret = PTR_ERR(dev);
-			goto err_close;
+			return PTR_ERR(dev);
 		}
 	}
 
 	return ret;
-
-err_close:
-	hid_hw_close(hdev);
-err_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void ps_remove(struct hid_device *hdev)
@@ -2750,9 +2734,6 @@ static void ps_remove(struct hid_device *hdev)
 
 	if (dev->remove)
 		dev->remove(dev);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id ps_devices[] = {
-- 
2.34.1

[PATCH -next 10/19] HID: hid-steam: Use devm_hid_hw_start_and_open in steam_probe()

[Li Zetao]

Currently, the hid-steam module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_hw_close and err_hw_stop lables.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-steam.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index bf8b633114be..d393762bf52f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1236,18 +1236,10 @@ static int steam_probe(struct hid_device *hdev,
 	 * With the real steam controller interface, do not connect hidraw.
 	 * Instead, create the client_hid and connect that.
 	 */
-	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
 	if (ret)
 		goto err_cancel_work;
 
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev,
-			"%s:hid_hw_open\n",
-			__func__);
-		goto err_hw_stop;
-	}
-
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver connected");
 		/* If using a wireless adaptor ask for connection status */
@@ -1261,7 +1253,7 @@ static int steam_probe(struct hid_device *hdev,
 			hid_err(hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-			goto err_hw_close;
+			goto err_cancel_work;
 		}
 	}
 
@@ -1283,10 +1275,6 @@ static int steam_probe(struct hid_device *hdev,
 err_steam_unregister:
 	if (steam->connected)
 		steam_unregister(steam);
-err_hw_close:
-	hid_hw_close(hdev);
-err_hw_stop:
-	hid_hw_stop(hdev);
 err_cancel_work:
 	cancel_work_sync(&steam->work_connect);
 	cancel_delayed_work_sync(&steam->mode_switch);
@@ -1312,8 +1300,6 @@ static void steam_remove(struct hid_device *hdev)
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
 		hid_info(hdev, "Steam wireless receiver disconnected");
 	}
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 	steam_unregister(steam);
 }
 
-- 
2.34.1

Re: [PATCH -next 10/19] HID: hid-steam: Use devm_hid_hw_start_and_open in steam_probe()

[Benjamin Tissoires]

On Sep 04 2024, Li Zetao wrote:
> Currently, the hid-steam module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 err_hw_close and err_hw_stop lables.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hid/hid-steam.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index bf8b633114be..d393762bf52f 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1236,18 +1236,10 @@ static int steam_probe(struct hid_device *hdev,
>  	 * With the real steam controller interface, do not connect hidraw.
>  	 * Instead, create the client_hid and connect that.
>  	 */
> -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
>  	if (ret)
>  		goto err_cancel_work;
>  
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev,
> -			"%s:hid_hw_open\n",
> -			__func__);
> -		goto err_hw_stop;
> -	}
> -
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver connected");
>  		/* If using a wireless adaptor ask for connection status */
> @@ -1261,7 +1253,7 @@ static int steam_probe(struct hid_device *hdev,
>  			hid_err(hdev,
>  				"%s:steam_register failed with error %d\n",
>  				__func__, ret);
> -			goto err_hw_close;
> +			goto err_cancel_work;
>  		}
>  	}
>  
> @@ -1283,10 +1275,6 @@ static int steam_probe(struct hid_device *hdev,
>  err_steam_unregister:
>  	if (steam->connected)
>  		steam_unregister(steam);
> -err_hw_close:
> -	hid_hw_close(hdev);
> -err_hw_stop:
> -	hid_hw_stop(hdev);
>  err_cancel_work:
>  	cancel_work_sync(&steam->work_connect);
>  	cancel_delayed_work_sync(&steam->mode_switch);
> @@ -1312,8 +1300,6 @@ static void steam_remove(struct hid_device *hdev)
>  	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
>  		hid_info(hdev, "Steam wireless receiver disconnected");
>  	}
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  	steam_unregister(steam);

steam_unregister was called after hid_hw_stop, and now it's before.
Someone should ensure this is correct...

Cheers,
Benjamin

>  }
>  
> -- 
> 2.34.1
> 

[PATCH -next 11/19] HID: wiimote: Use devm_hid_hw_start_and_open in wiimote_hid_probe()

[Li Zetao]

Currently, the wiimote module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 err_close and err_stop lables.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hid/hid-wiimote-core.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 26167cfb696f..28cd9ccbb617 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1780,8 +1780,6 @@ static void wiimote_destroy(struct wiimote_data *wdata)
 	wiimote_ext_unload(wdata);
 	wiimote_modules_unload(wdata);
 	cancel_work_sync(&wdata->queue.worker);
-	hid_hw_close(wdata->hdev);
-	hid_hw_stop(wdata->hdev);
 
 	kfree(wdata);
 }
@@ -1806,22 +1804,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		goto err;
 	}
 
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "HW start failed\n");
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		goto err;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "cannot start hardware I/O\n");
-		goto err_stop;
-	}
 
 	ret = device_create_file(&hdev->dev, &dev_attr_extension);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
-		goto err_close;
+		goto err;
 	}
 
 	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
@@ -1847,10 +1837,6 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 
 err_ext:
 	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
-err_close:
-	hid_hw_close(hdev);
-err_stop:
-	hid_hw_stop(hdev);
 err:
 	input_free_device(wdata->ir);
 	input_free_device(wdata->accel);
-- 
2.34.1

Re: [PATCH -next 11/19] HID: wiimote: Use devm_hid_hw_start_and_open in wiimote_hid_probe()

[David Rheinsberg]

Hi

On Wed, Sep 4, 2024, at 2:35 PM, Li Zetao wrote:
> Currently, the wiimote module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 err_close and err_stop lables.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hid/hid-wiimote-core.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 26167cfb696f..28cd9ccbb617 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1780,8 +1780,6 @@ static void wiimote_destroy(struct wiimote_data *wdata)
>  	wiimote_ext_unload(wdata);
>  	wiimote_modules_unload(wdata);
>  	cancel_work_sync(&wdata->queue.worker);
> -	hid_hw_close(wdata->hdev);
> -	hid_hw_stop(wdata->hdev);
> 
>  	kfree(wdata);
>  }
> @@ -1806,22 +1804,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  		goto err;
>  	}
> 
> -	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> -	if (ret) {
> -		hid_err(hdev, "HW start failed\n");
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
>  		goto err;
> -	}
> -
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev, "cannot start hardware I/O\n");
> -		goto err_stop;
> -	}
> 
>  	ret = device_create_file(&hdev->dev, &dev_attr_extension);
>  	if (ret) {
>  		hid_err(hdev, "cannot create sysfs attribute\n");
> -		goto err_close;
> +		goto err;
>  	}
> 
>  	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
> @@ -1847,10 +1837,6 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> 
>  err_ext:
>  	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> -err_close:
> -	hid_hw_close(hdev);
> -err_stop:
> -	hid_hw_stop(hdev);
>  err:
>  	input_free_device(wdata->ir);
>  	input_free_device(wdata->accel);
> -- 
> 2.34.1

Looks good!

Reviewed-by: David Rheinsberg <david@readahead.eu>

Thanks
David

Re: [PATCH -next 11/19] HID: wiimote: Use devm_hid_hw_start_and_open in wiimote_hid_probe()

[Benjamin Tissoires]

On Sep 04 2024, Li Zetao wrote:
> Currently, the wiimote module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 err_close and err_stop lables.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hid/hid-wiimote-core.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 26167cfb696f..28cd9ccbb617 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1780,8 +1780,6 @@ static void wiimote_destroy(struct wiimote_data *wdata)
>  	wiimote_ext_unload(wdata);
>  	wiimote_modules_unload(wdata);
>  	cancel_work_sync(&wdata->queue.worker);
> -	hid_hw_close(wdata->hdev);
> -	hid_hw_stop(wdata->hdev);
>  
>  	kfree(wdata);

This is wrong because wdata is not devres managed.

So there is probably a race condition where it gets freed while the
underlying bus wasn't stopped.

Cheers,
Benjamin

>  }
> @@ -1806,22 +1804,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  		goto err;
>  	}
>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> -	if (ret) {
> -		hid_err(hdev, "HW start failed\n");
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
>  		goto err;
> -	}
> -
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev, "cannot start hardware I/O\n");
> -		goto err_stop;
> -	}
>  
>  	ret = device_create_file(&hdev->dev, &dev_attr_extension);
>  	if (ret) {
>  		hid_err(hdev, "cannot create sysfs attribute\n");
> -		goto err_close;
> +		goto err;
>  	}
>  
>  	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
> @@ -1847,10 +1837,6 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  
>  err_ext:
>  	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> -err_close:
> -	hid_hw_close(hdev);
> -err_stop:
> -	hid_hw_stop(hdev);
>  err:
>  	input_free_device(wdata->ir);
>  	input_free_device(wdata->accel);
> -- 
> 2.34.1
> 

[PATCH -next 12/19] hwmon: (aquacomputer_d5next) Use devm_hid_hw_start_and_open in aqc_probe()

[Li Zetao]

Currently, the aquacomputer_d5next module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/aquacomputer_d5next.c | 39 +++++++----------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 8e55cd2f46f5..9b66ff0fe6e1 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -1556,14 +1556,10 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	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 fail_and_stop;
-
 	switch (hdev->product) {
 	case USB_PRODUCT_ID_AQUAERO:
 		/*
@@ -1577,10 +1573,8 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * they present. The two other devices have the type of the second element in
 		 * their respective collections set to 1, while the real device has it set to 0.
 		 */
-		if (hdev->collection[1].type != 0) {
-			ret = -ENODEV;
-			goto fail_and_close;
-		}
+		if (hdev->collection[1].type != 0)
+			return -ENODEV;
 
 		priv->kind = aquaero;
 
@@ -1740,10 +1734,8 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * Choose the right Leakshield device, because
 		 * the other one acts as a keyboard
 		 */
-		if (hdev->type != 2) {
-			ret = -ENODEV;
-			goto fail_and_close;
-		}
+		if (hdev->type != 2)
+			return -ENODEV;
 
 		priv->kind = leakshield;
 
@@ -1865,30 +1857,20 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	priv->name = aqc_device_names[priv->kind];
 
 	priv->buffer = devm_kzalloc(&hdev->dev, priv->buffer_size, GFP_KERNEL);
-	if (!priv->buffer) {
-		ret = -ENOMEM;
-		goto fail_and_close;
-	}
+	if (!priv->buffer)
+		return -ENOMEM;
 
 	mutex_init(&priv->mutex);
 
 	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, priv->name, priv,
 							  &aqc_chip_info, NULL);
 
-	if (IS_ERR(priv->hwmon_dev)) {
-		ret = PTR_ERR(priv->hwmon_dev);
-		goto fail_and_close;
-	}
+	if (IS_ERR(priv->hwmon_dev))
+		return PTR_ERR(priv->hwmon_dev);
 
 	aqc_debugfs_init(priv);
 
 	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void aqc_remove(struct hid_device *hdev)
@@ -1897,9 +1879,6 @@ static void aqc_remove(struct hid_device *hdev)
 
 	debugfs_remove_recursive(priv->debugfs);
 	hwmon_device_unregister(priv->hwmon_dev);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id aqc_table[] = {
-- 
2.34.1

[PATCH -next 13/19] hwmon: Use devm_hid_hw_start_and_open in rog_ryujin_probe()

[Li Zetao]

Currently, the rog_ryujin module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/asus_rog_ryujin.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
index f8b20346a995..da03ba3b4e0f 100644
--- a/drivers/hwmon/asus_rog_ryujin.c
+++ b/drivers/hwmon/asus_rog_ryujin.c
@@ -520,23 +520,13 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
 	}
 
 	/* Enable hidraw so existing user-space tools can continue to work */
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "hid hw start failed with %d\n", ret);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "hid hw open failed with %d\n", ret);
-		goto fail_and_stop;
-	}
 
 	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
-	if (!priv->buffer) {
-		ret = -ENOMEM;
-		goto fail_and_close;
-	}
+	if (!priv->buffer)
+		return -ENOMEM;
 
 	mutex_init(&priv->status_report_request_mutex);
 	mutex_init(&priv->buffer_lock);
@@ -553,16 +543,10 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
 	if (IS_ERR(priv->hwmon_dev)) {
 		ret = PTR_ERR(priv->hwmon_dev);
 		hid_err(hdev, "hwmon registration failed with %d\n", ret);
-		goto fail_and_close;
+		return ret;
 	}
 
 	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void rog_ryujin_remove(struct hid_device *hdev)
@@ -570,9 +554,6 @@ static void rog_ryujin_remove(struct hid_device *hdev)
 	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
 
 	hwmon_device_unregister(priv->hwmon_dev);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id rog_ryujin_table[] = {
-- 
2.34.1

Re: [PATCH -next 13/19] hwmon: Use devm_hid_hw_start_and_open in rog_ryujin_probe()

[Guenter Roeck]

On 9/4/24 05:36, Li Zetao wrote:
> Currently, the rog_ryujin module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 fail_and_close and fail_and_stop
> lables, and directly return the error code when an error occurs.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>   drivers/hwmon/asus_rog_ryujin.c | 29 +++++------------------------
>   1 file changed, 5 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index f8b20346a995..da03ba3b4e0f 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c
> @@ -520,23 +520,13 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>   	}
>   
>   	/* Enable hidraw so existing user-space tools can continue to work */
> -	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> -	if (ret) {
> -		hid_err(hdev, "hid hw start failed with %d\n", ret);
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
>   		return ret;
> -	}
> -
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev, "hid hw open failed with %d\n", ret);
> -		goto fail_and_stop;
> -	}
>   
>   	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
> -	if (!priv->buffer) {
> -		ret = -ENOMEM;
> -		goto fail_and_close;
> -	}
> +	if (!priv->buffer)
> +		return -ENOMEM;
>   
>   	mutex_init(&priv->status_report_request_mutex);
>   	mutex_init(&priv->buffer_lock);
> @@ -553,16 +543,10 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>   	if (IS_ERR(priv->hwmon_dev)) {
>   		ret = PTR_ERR(priv->hwmon_dev);
>   		hid_err(hdev, "hwmon registration failed with %d\n", ret);
> -		goto fail_and_close;
> +		return ret;
>   	}

	struct device *hwmon_dev;

         hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
                                                     priv, &rog_ryujin_chip_info, NULL);
	return PTR_ERR_OR_ZERO(hwmon_dev);

and drop the remove function.

Thanks,
Guenter

>   
>   	return 0;
> -
> -fail_and_close:
> -	hid_hw_close(hdev);
> -fail_and_stop:
> -	hid_hw_stop(hdev);
> -	return ret;
>   }
>   
>   static void rog_ryujin_remove(struct hid_device *hdev)
> @@ -570,9 +554,6 @@ static void rog_ryujin_remove(struct hid_device *hdev)
>   	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
>   
>   	hwmon_device_unregister(priv->hwmon_dev);
> -
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>   }
>   
>   static const struct hid_device_id rog_ryujin_table[] = {

[PATCH -next 14/19] hwmon: (corsair-cpro) Use devm_hid_hw_start_and_open in ccp_probe()

[Li Zetao]

Currently, the corsair-cpro module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 hid_hw_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/corsair-cpro.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index e1a7f7aa7f80..7bba30840f32 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -601,14 +601,10 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	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;
-
 	ccp->hdev = hdev;
 	hid_set_drvdata(hdev, ccp);
 
@@ -621,28 +617,20 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	/* temp and fan connection status only updates when device is powered on */
 	ret = get_temp_cnct(ccp);
 	if (ret)
-		goto out_hw_close;
+		return ret;
 
 	ret = get_fan_cnct(ccp);
 	if (ret)
-		goto out_hw_close;
+		return ret;
 
 	ccp_debugfs_init(ccp);
 
 	ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
 							 ccp, &ccp_chip_info, NULL);
-	if (IS_ERR(ccp->hwmon_dev)) {
-		ret = PTR_ERR(ccp->hwmon_dev);
-		goto out_hw_close;
-	}
+	if (IS_ERR(ccp->hwmon_dev))
+		return PTR_ERR(ccp->hwmon_dev);
 
 	return 0;
-
-out_hw_close:
-	hid_hw_close(hdev);
-out_hw_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void ccp_remove(struct hid_device *hdev)
@@ -651,8 +639,6 @@ static void ccp_remove(struct hid_device *hdev)
 
 	debugfs_remove_recursive(ccp->debugfs);
 	hwmon_device_unregister(ccp->hwmon_dev);
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id ccp_devices[] = {
-- 
2.34.1

[PATCH -next 15/19] hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in corsairpsu_probe()

[Li Zetao]

Currently, the corsair-psu module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/corsair-psu.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index f8f22b8a67cd..b574ec9cd00f 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -787,14 +787,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 	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 fail_and_stop;
-
 	priv->hdev = hdev;
 	hid_set_drvdata(hdev, priv);
 	mutex_init(&priv->lock);
@@ -805,13 +801,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 	ret = corsairpsu_init(priv);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
-		goto fail_and_stop;
+		return ret;
 	}
 
 	ret = corsairpsu_fwinfo(priv);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
-		goto fail_and_stop;
+		return ret;
 	}
 
 	corsairpsu_get_criticals(priv);
@@ -820,20 +816,12 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 	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);
-		goto fail_and_close;
-	}
+	if (IS_ERR(priv->hwmon_dev))
+		return PTR_ERR(priv->hwmon_dev);
 
 	corsairpsu_debugfs_init(priv);
 
 	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void corsairpsu_remove(struct hid_device *hdev)
@@ -842,8 +830,6 @@ static void corsairpsu_remove(struct hid_device *hdev)
 
 	debugfs_remove_recursive(priv->debugfs);
 	hwmon_device_unregister(priv->hwmon_dev);
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data,
-- 
2.34.1

Re: [PATCH -next 15/19] hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in corsairpsu_probe()

[Wilken Gottwalt]

On Wed, 4 Sep 2024 20:36:03 +0800
Li Zetao <lizetao1@huawei.com> wrote:

> Currently, the corsair-psu module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 fail_and_close and fail_and_stop
> lables, and directly return the error code when an error occurs.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  drivers/hwmon/corsair-psu.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index f8f22b8a67cd..b574ec9cd00f 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -787,14 +787,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id 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 fail_and_stop;
> -
>  	priv->hdev = hdev;
>  	hid_set_drvdata(hdev, priv);
>  	mutex_init(&priv->lock);
> @@ -805,13 +801,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id ret = corsairpsu_init(priv);
>  	if (ret < 0) {
>  		dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
> -		goto fail_and_stop;
> +		return ret;
>  	}
>  
>  	ret = corsairpsu_fwinfo(priv);
>  	if (ret < 0) {
>  		dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
> -		goto fail_and_stop;
> +		return ret;
>  	}
>  
>  	corsairpsu_get_criticals(priv);
> @@ -820,20 +816,12 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id 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);
> -		goto fail_and_close;
> -	}
> +	if (IS_ERR(priv->hwmon_dev))
> +		return PTR_ERR(priv->hwmon_dev);
>  
>  	corsairpsu_debugfs_init(priv);
>  
>  	return 0;
> -
> -fail_and_close:
> -	hid_hw_close(hdev);
> -fail_and_stop:
> -	hid_hw_stop(hdev);
> -	return ret;
>  }
>  
>  static void corsairpsu_remove(struct hid_device *hdev)
> @@ -842,8 +830,6 @@ static void corsairpsu_remove(struct hid_device *hdev)
>  
>  	debugfs_remove_recursive(priv->debugfs);
>  	hwmon_device_unregister(priv->hwmon_dev);
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
>  }
>  
>  static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data,

So far looks fine to me.

Reviewed-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

greetings,
Wilken

[PATCH -next 16/19] hwmon: (gigabyte_waterforce) Use devm_hid_hw_start_and_open in waterforce_probe()

[Li Zetao]

Currently, the waterforce module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/gigabyte_waterforce.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/gigabyte_waterforce.c b/drivers/hwmon/gigabyte_waterforce.c
index 8129d7b3ceaf..9052d1c3d5aa 100644
--- a/drivers/hwmon/gigabyte_waterforce.c
+++ b/drivers/hwmon/gigabyte_waterforce.c
@@ -337,23 +337,13 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
 	/*
 	 * Enable hidraw so existing user-space tools can continue to work.
 	 */
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "hid hw start failed with %d\n", ret);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "hid hw open failed with %d\n", ret);
-		goto fail_and_stop;
-	}
 
 	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
-	if (!priv->buffer) {
-		ret = -ENOMEM;
-		goto fail_and_close;
-	}
+	if (!priv->buffer)
+		return -ENOMEM;
 
 	mutex_init(&priv->status_report_request_mutex);
 	mutex_init(&priv->buffer_lock);
@@ -371,18 +361,12 @@ static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id
 	if (IS_ERR(priv->hwmon_dev)) {
 		ret = PTR_ERR(priv->hwmon_dev);
 		hid_err(hdev, "hwmon registration failed with %d\n", ret);
-		goto fail_and_close;
+		return ret;
 	}
 
 	waterforce_debugfs_init(priv);
 
 	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void waterforce_remove(struct hid_device *hdev)
@@ -391,9 +375,6 @@ static void waterforce_remove(struct hid_device *hdev)
 
 	debugfs_remove_recursive(priv->debugfs);
 	hwmon_device_unregister(priv->hwmon_dev);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id waterforce_table[] = {
-- 
2.34.1

[PATCH -next 17/19] hwmon: (nzxt-kraken2) Use devm_hid_hw_start_and_open in kraken2_probe()

[Li Zetao]

Currently, the nzxt-kraken2 module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/nzxt-kraken2.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
index 7caf387eb144..aaaf857b42ed 100644
--- a/drivers/hwmon/nzxt-kraken2.c
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -158,17 +158,9 @@ static int kraken2_probe(struct hid_device *hdev,
 	/*
 	 * Enable hidraw so existing user-space tools can continue to work.
 	 */
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "hid hw start failed with %d\n", ret);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "hid hw open failed with %d\n", ret);
-		goto fail_and_stop;
-	}
 
 	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
 							  priv, &kraken2_chip_info,
@@ -176,16 +168,10 @@ static int kraken2_probe(struct hid_device *hdev,
 	if (IS_ERR(priv->hwmon_dev)) {
 		ret = PTR_ERR(priv->hwmon_dev);
 		hid_err(hdev, "hwmon registration failed with %d\n", ret);
-		goto fail_and_close;
+		return ret;
 	}
 
 	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void kraken2_remove(struct hid_device *hdev)
@@ -193,9 +179,6 @@ 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);
 }
 
 static const struct hid_device_id kraken2_table[] = {
-- 
2.34.1

Re: [PATCH -next 17/19] hwmon: (nzxt-kraken2) Use devm_hid_hw_start_and_open in kraken2_probe()

[Guenter Roeck]

On 9/4/24 05:36, Li Zetao wrote:
> Currently, the nzxt-kraken2 module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> 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 fail_and_close and fail_and_stop
> lables, and directly return the error code when an error occurs.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>   drivers/hwmon/nzxt-kraken2.c | 23 +++--------------------
>   1 file changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
> index 7caf387eb144..aaaf857b42ed 100644
> --- a/drivers/hwmon/nzxt-kraken2.c
> +++ b/drivers/hwmon/nzxt-kraken2.c
> @@ -158,17 +158,9 @@ static int kraken2_probe(struct hid_device *hdev,
>   	/*
>   	 * Enable hidraw so existing user-space tools can continue to work.
>   	 */
> -	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> -	if (ret) {
> -		hid_err(hdev, "hid hw start failed with %d\n", ret);
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
>   		return ret;
> -	}
> -
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev, "hid hw open failed with %d\n", ret);
> -		goto fail_and_stop;
> -	}
>   
>   	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
>   							  priv, &kraken2_chip_info,
> @@ -176,16 +168,10 @@ static int kraken2_probe(struct hid_device *hdev,
>   	if (IS_ERR(priv->hwmon_dev)) {
>   		ret = PTR_ERR(priv->hwmon_dev);
>   		hid_err(hdev, "hwmon registration failed with %d\n", ret);
> -		goto fail_and_close;
> +		return ret;
>   	}
>   
>   	return 0;

	struct device *hwmon_dev;	// also drop from struct kraken2_priv_data
	...
	hwmon_dev = devm_hwmon_device_register_with_info(...);
	return PTR_ERR_OR_ZERO(hwmon_dev);

and drop the remove function.

Thanks,
Guenter

> -
> -fail_and_close:
> -	hid_hw_close(hdev);
> -fail_and_stop:
> -	hid_hw_stop(hdev);
> -	return ret;
>   }
>   
>   static void kraken2_remove(struct hid_device *hdev)
> @@ -193,9 +179,6 @@ 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);
>   }
>   
>   static const struct hid_device_id kraken2_table[] = {

[PATCH -next 18/19] hwmon: (nzxt-kraken3) Use devm_hid_hw_start_and_open in kraken3_probe()

[Li Zetao]

Currently, the nzxt-kraken2 module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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 fail_and_close and fail_and_stop
lables, and directly return the error code when an error occurs.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 drivers/hwmon/nzxt-kraken3.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
index 00f3ac90a290..71b8c21cfe1b 100644
--- a/drivers/hwmon/nzxt-kraken3.c
+++ b/drivers/hwmon/nzxt-kraken3.c
@@ -897,17 +897,9 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
 	}
 
 	/* Enable hidraw so existing user-space tools can continue to work */
-	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (ret) {
-		hid_err(hdev, "hid hw start failed with %d\n", ret);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "hid hw open failed with %d\n", ret);
-		goto fail_and_stop;
-	}
 
 	switch (hdev->product) {
 	case USB_PRODUCT_ID_X53:
@@ -928,15 +920,12 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
 		device_name = "kraken2023elite";
 		break;
 	default:
-		ret = -ENODEV;
-		goto fail_and_close;
+		return -ENODEV;
 	}
 
 	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
-	if (!priv->buffer) {
-		ret = -ENOMEM;
-		goto fail_and_close;
-	}
+	if (!priv->buffer)
+		return -ENOMEM;
 
 	mutex_init(&priv->buffer_lock);
 	mutex_init(&priv->z53_status_request_lock);
@@ -948,7 +937,7 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
 	ret = kraken3_init_device(hdev);
 	if (ret < 0) {
 		hid_err(hdev, "device init failed with %d\n", ret);
-		goto fail_and_close;
+		return ret;
 	}
 
 	ret = kraken3_get_fw_ver(hdev);
@@ -960,18 +949,12 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
 	if (IS_ERR(priv->hwmon_dev)) {
 		ret = PTR_ERR(priv->hwmon_dev);
 		hid_err(hdev, "hwmon registration failed with %d\n", ret);
-		goto fail_and_close;
+		return ret;
 	}
 
 	kraken3_debugfs_init(priv, device_name);
 
 	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
 }
 
 static void kraken3_remove(struct hid_device *hdev)
@@ -980,9 +963,6 @@ static void kraken3_remove(struct hid_device *hdev)
 
 	debugfs_remove_recursive(priv->debugfs);
 	hwmon_device_unregister(priv->hwmon_dev);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
 }
 
 static const struct hid_device_id kraken3_table[] = {
-- 
2.34.1

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

[Li Zetao]

Currently, the nzxt-smart2 module needs to maintain hid resources
by itself. Consider using devm_hid_hw_start_and_open helper to ensure
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;
-
-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[] = {
-- 
2.34.1

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

[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/

> 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.

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[] = {

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

[Li Zetao]

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[] = {
>