[PATCH 1/2] HID: hid-steam: remove pointless error message

[PATCH 1/2] HID: hid-steam: remove pointless error message

[Dan Carpenter]

This error message doesn't really add any information.  If modprobe
fails then the user will already know what the error code is.  In the
case of kmalloc() it's a style violation to print an error message for
that because kmalloc has it's own better error messages built in.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/hid/hid-steam.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index b3c4e50e248a..59df6ead7b54 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1109,10 +1109,9 @@ static int steam_probe(struct hid_device *hdev,
 		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 
 	steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
-	if (!steam) {
-		ret = -ENOMEM;
-		goto steam_alloc_fail;
-	}
+	if (!steam)
+		return -ENOMEM;
+
 	steam->hdev = hdev;
 	hid_set_drvdata(hdev, steam);
 	spin_lock_init(&steam->lock);
@@ -1179,9 +1178,6 @@ static int steam_probe(struct hid_device *hdev,
 	cancel_work_sync(&steam->work_connect);
 	cancel_delayed_work_sync(&steam->mode_switch);
 	cancel_work_sync(&steam->rumble_work);
-steam_alloc_fail:
-	hid_err(hdev, "%s: failed with error %d\n",
-			__func__, ret);
 	return ret;
 }
 
-- 
2.43.0

[PATCH 2/2] HID: hid-steam: Fix cleanup in probe()

[Dan Carpenter]

There are a number of issues in this code.  First of all if
steam_create_client_hid() fails then it leads to an error pointer
dereference when we call hid_destroy_device(steam->client_hdev).

Also there are a number of leaks.  hid_hw_stop() is not called if
hid_hw_open() fails for example.  And it doesn't call steam_unregister()
or hid_hw_close().

Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
This is just from static analysis and code review.  I haven't tested
it.  I only included the fixes tag for the error pointer dereference.

 drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 59df6ead7b54..b08a5ab58528 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
 	 */
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
 	if (ret)
-		goto hid_hw_start_fail;
+		goto err_cancel_work;
 
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev,
 			"%s:hid_hw_open\n",
 			__func__);
-		goto hid_hw_open_fail;
+		goto err_hw_stop;
 	}
 
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
@@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
 			hid_err(hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-			goto input_register_fail;
+			goto err_hw_close;
 		}
 	}
 
 	steam->client_hdev = steam_create_client_hid(hdev);
 	if (IS_ERR(steam->client_hdev)) {
 		ret = PTR_ERR(steam->client_hdev);
-		goto client_hdev_fail;
+		goto err_stream_unregister;
 	}
 	steam->client_hdev->driver_data = steam;
 
 	ret = hid_add_device(steam->client_hdev);
 	if (ret)
-		goto client_hdev_add_fail;
+		goto err_destroy;
 
 	return 0;
 
-client_hdev_add_fail:
-	hid_hw_stop(hdev);
-client_hdev_fail:
+err_destroy:
 	hid_destroy_device(steam->client_hdev);
-input_register_fail:
-hid_hw_open_fail:
-hid_hw_start_fail:
+err_stream_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);
 	cancel_work_sync(&steam->rumble_work);
+
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH 2/2] HID: hid-steam: Fix cleanup in probe()

[Vicki Pfau]

I have applied this to our downstream and made sure it compiles and runs 
without obvious issues.

Reviewed-by: Vicki Pfau <vi@endrift.com>

Thanks,
Vicki

On 1/12/24 06:35, Dan Carpenter wrote:
> There are a number of issues in this code.  First of all if
> steam_create_client_hid() fails then it leads to an error pointer
> dereference when we call hid_destroy_device(steam->client_hdev).
> 
> Also there are a number of leaks.  hid_hw_stop() is not called if
> hid_hw_open() fails for example.  And it doesn't call steam_unregister()
> or hid_hw_close().
> 
> Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This is just from static analysis and code review.  I haven't tested
> it.  I only included the fixes tag for the error pointer dereference.
> 
>   drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 59df6ead7b54..b08a5ab58528 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
>   	 */
>   	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
>   	if (ret)
> -		goto hid_hw_start_fail;
> +		goto err_cancel_work;
>   
>   	ret = hid_hw_open(hdev);
>   	if (ret) {
>   		hid_err(hdev,
>   			"%s:hid_hw_open\n",
>   			__func__);
> -		goto hid_hw_open_fail;
> +		goto err_hw_stop;
>   	}
>   
>   	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> @@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
>   			hid_err(hdev,
>   				"%s:steam_register failed with error %d\n",
>   				__func__, ret);
> -			goto input_register_fail;
> +			goto err_hw_close;
>   		}
>   	}
>   
>   	steam->client_hdev = steam_create_client_hid(hdev);
>   	if (IS_ERR(steam->client_hdev)) {
>   		ret = PTR_ERR(steam->client_hdev);
> -		goto client_hdev_fail;
> +		goto err_stream_unregister;
>   	}
>   	steam->client_hdev->driver_data = steam;
>   
>   	ret = hid_add_device(steam->client_hdev);
>   	if (ret)
> -		goto client_hdev_add_fail;
> +		goto err_destroy;
>   
>   	return 0;
>   
> -client_hdev_add_fail:
> -	hid_hw_stop(hdev);
> -client_hdev_fail:
> +err_destroy:
>   	hid_destroy_device(steam->client_hdev);
> -input_register_fail:
> -hid_hw_open_fail:
> -hid_hw_start_fail:
> +err_stream_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);
>   	cancel_work_sync(&steam->rumble_work);
> +
>   	return ret;
>   }
>   

Re: [PATCH 1/2] HID: hid-steam: remove pointless error message

[Benjamin Tissoires]

On Fri, 12 Jan 2024 17:34:14 +0300, Dan Carpenter wrote:
> This error message doesn't really add any information.  If modprobe
> fails then the user will already know what the error code is.  In the
> case of kmalloc() it's a style violation to print an error message for
> that because kmalloc has it's own better error messages built in.
> 
> 

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.8/upstream-fixes), thanks!

[1/2] HID: hid-steam: remove pointless error message
      https://git.kernel.org/hid/hid/c/a96681699611
[2/2] HID: hid-steam: Fix cleanup in probe()
      https://git.kernel.org/hid/hid/c/a9f1da09c69f

Cheers,
-- 
Benjamin Tissoires <bentiss@kernel.org>