linux-kernel - Re: [PATCH v2 1/5] HID: steelseries: preparation for adding SteelSeries Arctis 9 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3b791f85-17ab-4987-afb5-9dbdbd6df1ec@mayer-bgk.de>
Date: Wed, 15 Jan 2025 18:20:14 +0100
From: Christian Mayer <christian@...er-bgk.de>
To: Bastien Nocera <hadess@...ess.net>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 Christian Mayer <git@...er-bgk.de>, linux-input@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <benjamin.tissoires@...hat.com>
Subject: Re: [PATCH v2 1/5] HID: steelseries: preparation for adding
 SteelSeries Arctis 9 support



Am 13.01.25 um 14:42 schrieb Bastien Nocera:
> On Sun, 2025-01-12 at 13:02 +0100, Christophe JAILLET wrote:
>> Le 12/01/2025 à 12:44, Christian Mayer a écrit :
>>> Refactor code and add calls to hid_hw_open/hid_hw_closed in
>>> preparation
>>> for adding support for the SteelSeries Arctis 9 headset.
>>>
>>> Signed-off-by: Christian Mayer <git@...er-bgk.de>
> 
> Feel free to add my:
> Reviewed-by: Bastien Nocera <hadess@...ess.net>
> Tested-by: Bastien Nocera <hadess@...ess.net>
> 
> once you've made the change Christophe mentions.
> 
>>> ---
>>>    drivers/hid/hid-steelseries.c | 19 +++++++++++++------
>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
>>> steelseries.c
>>> index f9ff5be94309..dc4ab55d7c22 100644
>>> --- a/drivers/hid/hid-steelseries.c
>>> +++ b/drivers/hid/hid-steelseries.c
>>> @@ -377,20 +377,21 @@ static void steelseries_srws1_remove(struct
>>> hid_device *hdev)
>>>    #define ARCTIS_1_BATTERY_RESPONSE_LEN		8
>>>    static const char arctis_1_battery_request[] = { 0x06, 0x12 };
>>>    
>>> -static int steelseries_headset_arctis_1_fetch_battery(struct
>>> hid_device *hdev)
>>> +static int steelseries_headset_request_battery(struct hid_device
>>> *hdev,
>>> +	const char *request, size_t len)
>>>    {
>>>    	u8 *write_buf;
>>>    	int ret;
>>>    
>>>    	/* Request battery information */
>>> -	write_buf = kmemdup(arctis_1_battery_request,
>>> sizeof(arctis_1_battery_request), GFP_KERNEL);
>>> +	write_buf = kmemdup(request, len, GFP_KERNEL);
>>>    	if (!write_buf)
>>>    		return -ENOMEM;
>>>    
>>> -	ret = hid_hw_raw_request(hdev,
>>> arctis_1_battery_request[0],
>>> -				 write_buf,
>>> sizeof(arctis_1_battery_request),
>>> +	hid_dbg(hdev, "Sending battery request report");
>>> +	ret = hid_hw_raw_request(hdev, request[0], write_buf, len,
>>>    				 HID_OUTPUT_REPORT,
>>> HID_REQ_SET_REPORT);
>>> -	if (ret < (int)sizeof(arctis_1_battery_request)) {
>>> +	if (ret < (int)len) {
>>>    		hid_err(hdev, "hid_hw_raw_request() failed with
>>> %d\n", ret);
>>>    		ret = -ENODATA;
>>>    	}
>>> @@ -404,7 +405,8 @@ static void
>>> steelseries_headset_fetch_battery(struct hid_device *hdev)
>>>    	int ret = 0;
>>>    
>>>    	if (sd->quirks & STEELSERIES_ARCTIS_1)
>>> -		ret =
>>> steelseries_headset_arctis_1_fetch_battery(hdev);
>>> +		ret = steelseries_headset_request_battery(hdev,
>>> +			arctis_1_battery_request,
>>> sizeof(arctis_1_battery_request));
>>>    
>>>    	if (ret < 0)
>>>    		hid_dbg(hdev,
>>> @@ -554,6 +556,10 @@ static int steelseries_probe(struct hid_device
>>> *hdev, const struct hid_device_id
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> +	ret = hid_hw_open(hdev);
>>> +	if (ret)
>>> +		return ret;
>>
>> Should we call hid_hw_stop() if
>> steelseries_headset_battery_register()
>> below fails, as done in the remove funcion?
Did you mean hid_hw_close instead of hid_hw_stop?
If the battery_register fails, the driver will still get assigned to the 
device, because ret is not getting set and is still 0 from hid_hw_open, 
right?

hid_hw_close doesn't get called before hid_hw_stop and the remove 
function will get called when the device disconnects.
That doesn't seem right to me, or am i wrong?

>>
>>> +
>>>    	if (steelseries_headset_battery_register(sd) < 0)
>>>    		hid_err(sd->hdev,
>>>    			"Failed to register battery for
>>> headset\n");
>>> @@ -580,6 +586,7 @@ static void steelseries_remove(struct
>>> hid_device *hdev)
>>>    
>>>    	cancel_delayed_work_sync(&sd->battery_work);
>>>    
>>> +	hid_hw_close(hdev);
>>>    	hid_hw_stop(hdev);
>>>    }
>>>    
>>
>