| 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
| ||
|
Message-ID: <baad8f90-e9cb-4c1d-bbbc-fa556c1c2e1a@mayer-bgk.de>
Date: Sun, 5 Jan 2025 11:16:16 +0100
From: Christian Mayer <christian@...er-bgk.de>
To: Bastien Nocera <hadess@...ess.net>, 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 1/3] HID: steelseries: add SteelSeries Arctis 9 support
> On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote:
>> Add support for the SteelSeries Arctis 9 headset. This driver
>> will export the battery information like it already does for
>> the Arcits 1 headset.
>>
>> Signed-off-by: Christian Mayer <git@...er-bgk.de>
>
>
> Please remove all the whitespace changes, and split off the
> steelseries_headset_request_battery() refactoring into its own commit.
>
> Detailed review comments below.
Thank you for your feedback Bastien.
I will repost my patches with the fixes within the next days.
Please see one follow-up question about hid_hw_open below.
>
>> ---
>> drivers/hid/hid-steelseries.c | 105 +++++++++++++++++++++++++++++-----
>> 1 file changed, 90 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
>> index f9ff5be94309..f102045a9b6b 100644
>> --- a/drivers/hid/hid-steelseries.c
>> +++ b/drivers/hid/hid-steelseries.c
>> @@ -18,7 +18,8 @@
>> #include "hid-ids.h"
>>
>> #define STEELSERIES_SRWS1 BIT(0)
>> -#define STEELSERIES_ARCTIS_1 BIT(1)
>> +#define STEELSERIES_ARCTIS_1 BIT(1)
>
> Whitespace change
>
>> +#define STEELSERIES_ARCTIS_9 BIT(2)
>>
>> struct steelseries_device {
>> struct hid_device *hdev;
>> @@ -35,7 +36,7 @@ struct steelseries_device {
>> };
>>
>> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \
>> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>
> Whitespace change
>
>> #define SRWS1_NUMBER_LEDS 15
>> struct steelseries_srws1_data {
>> __u16 led_state;
>> @@ -95,7 +96,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = {
>> 0x29, 0x11, /* Usage Maximum (11h), */
>> 0x95, 0x11, /* Report Count (17), */
>> 0x81, 0x02, /* Input (Variable), */
>> - /* ---- Dial patch starts here ---- */
>> + /* ---- Dial patch starts here ---- */
>
> Whitespace change
>
>> 0x05, 0x01, /* Usage Page (Desktop), */
>> 0x09, 0x33, /* Usage (RX), */
>> 0x75, 0x04, /* Report Size (4), */
>> @@ -108,7 +109,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = {
>> 0x95, 0x01, /* Report Count (1), */
>> 0x25, 0x03, /* Logical Maximum (3), */
>> 0x81, 0x02, /* Input (Variable), */
>> - /* ---- Dial patch ends here ---- */
>> + /* ---- Dial patch ends here ---- */
>
> Whitespace change
>
>> 0x06, 0x00, 0xFF, /* Usage Page (FF00h), */
>> 0x09, 0x01, /* Usage (01h), */
>> 0x75, 0x04, /* Changed Report Size (4), */
>> @@ -125,7 +126,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = {
>> };
>>
>> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \
>> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>
> Whitespace change
>
>> static void steelseries_srws1_set_leds(struct hid_device *hdev, __u16 leds)
>> {
>> struct list_head *report_list = &hdev->report_enum[HID_OUTPUT_REPORT].report_list;
>> @@ -368,32 +369,36 @@ static void steelseries_srws1_remove(struct hid_device *hdev)
>>
>> hid_hw_stop(hdev);
>> kfree(drv_data);
>> - return;
>
> Should be a separate commit.
>
>> }
>> #endif
>>
>> #define STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS 3000
>>
>> #define ARCTIS_1_BATTERY_RESPONSE_LEN 8
>> +#define ARCTIS_9_BATTERY_RESPONSE_LEN 64
>> static const char arctis_1_battery_request[] = { 0x06, 0x12 };
>> +static const char arctis_9_battery_request[] = { 0x00, 0x20 };
>>
>> -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)
>
> This change should be a two-step process (so two separate commits),
> first factor out the battery request function so it take the request
> and len as you've done, then add support for that new device.
>
>> {
>> 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;
>> }
>> +
>> kfree(write_buf);
>> return ret;
>> }
>> @@ -404,7 +409,11 @@ 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));
>> + else if (sd->quirks & STEELSERIES_ARCTIS_9)
>> + ret = steelseries_headset_request_battery(hdev,
>> + arctis_9_battery_request, sizeof(arctis_9_battery_request));
>>
>> if (ret < 0)
>> hid_dbg(hdev,
>> @@ -520,9 +529,22 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd)
>> INIT_DELAYED_WORK(&sd->battery_work, steelseries_headset_battery_timer_tick);
>> steelseries_headset_fetch_battery(sd->hdev);
>>
>> + if (sd->quirks & STEELSERIES_ARCTIS_9) {
>> + // The first fetch_battery request can remain unanswered in some cases
>
> No C++-style comments.
>
>> + schedule_delayed_work(&sd->battery_work,
>> + msecs_to_jiffies(STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS));
>> + }
>> +
>> return 0;
>> }
>>
>> +static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t usagePage)
>> +{
>> + return hdev->rdesc[0] == 0x06 &&
>> + hdev->rdesc[1] == usagePage &&
>> + hdev->rdesc[2] == 0xff;
>> +}
>
> Should probably be usage_page.
>
>> +
>> static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> {
>> struct steelseries_device *sd;
>> @@ -537,7 +559,7 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>>
>> if (sd->quirks & STEELSERIES_SRWS1) {
>> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \
>> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>> return steelseries_srws1_probe(hdev, id);
>> #else
>> return -ENODEV;
>> @@ -548,12 +570,22 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>> if (ret)
>> return ret;
>>
>> + if (sd->quirks & STEELSERIES_ARCTIS_9 &&
>> + !steelseries_is_vendor_usage_page(hdev, 0xc0))
>> + return -ENODEV;
>> +
>> spin_lock_init(&sd->lock);
>>
>> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> if (ret)
>> return ret;
>>
>> + if (sd->quirks & STEELSERIES_ARCTIS_9) {
>> + ret = hid_hw_open(hdev);
>
> Is this needed? If so, this probably needs to be added as a separate
> commit, for all headsets rather than just this one.
Yes that's mandatory to get raw_events from the device.
I thought about adding this to all headsets, but i was not sure if this
breaks anything for the Arctis 1 headset.
But i can add this for all headsets, that's fine with me.
Would you like me to create a completely separate patch for this or
should i submit this with the preparation patch which refactors
steelseries_headset_fetch_battery?
>
>> + if (ret)
>> + return ret;
>> + }
>> +
>> if (steelseries_headset_battery_register(sd) < 0)
>> hid_err(sd->hdev,
>> "Failed to register battery for headset\n");
>> @@ -568,7 +600,7 @@ static void steelseries_remove(struct hid_device *hdev)
>>
>> if (sd->quirks & STEELSERIES_SRWS1) {
>> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \
>> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES))
>> steelseries_srws1_remove(hdev);
>> #endif
>> return;
>> @@ -580,6 +612,9 @@ static void steelseries_remove(struct hid_device *hdev)
>>
>> cancel_delayed_work_sync(&sd->battery_work);
>>
>> + if (sd->quirks & STEELSERIES_ARCTIS_9)
>> + hid_hw_close(hdev);
>> +
>> hid_hw_stop(hdev);
>> }
>>
>> @@ -599,6 +634,15 @@ static const __u8 *steelseries_srws1_report_fixup(struct hid_device *hdev,
>> return rdesc;
>> }
>>
>> +static uint8_t steelseries_headset_map_capacity(uint8_t capacity, uint8_t min_in, uint8_t max_in)
>> +{
>> + if (capacity > max_in)
>> + return 100;
>> + if (capacity < min_in)
>> + return 0;
>> + return (capacity - min_in) * 100 / (max_in - min_in);
>> +}
>> +
>> static int steelseries_headset_raw_event(struct hid_device *hdev,
>> struct hid_report *report, u8 *read_buf,
>> int size)
>> @@ -630,6 +674,32 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
>> }
>> }
>>
>> + if (sd->quirks & STEELSERIES_ARCTIS_9) {
>> + hid_dbg(sd->hdev,
>> + "Parsing raw event for Arctis 9 headset (%*ph)\n", size, read_buf);
>> + if (size < ARCTIS_9_BATTERY_RESPONSE_LEN) {
>> + if (!delayed_work_pending(&sd->battery_work))
>> + goto request_battery;
>> + return 0;
>> + }
>> +
>> + if (read_buf[0] == 0xaa && read_buf[1] == 0x01) {
>> + connected = true;
>> +
>> + /*
>> + * Found no official documentation about min and max.
>> + * Values defined by testing.
>> + */
>> + capacity = steelseries_headset_map_capacity(read_buf[3], 0x68, 0x9d);
>> + } else {
>> + /*
>> + * Device is off and sends the last known status read_buf[1] == 0x03 or
>> + * there is no known status of the device read_buf[0] == 0x55
>> + */
>> + connected = false;
>> + }
>> + }
>> +
>> if (connected != sd->headset_connected) {
>> hid_dbg(sd->hdev,
>> "Connected status changed from %sconnected to %sconnected\n",
>> @@ -663,7 +733,11 @@ static const struct hid_device_id steelseries_devices[] = {
>>
>> { /* SteelSeries Arctis 1 Wireless for XBox */
>> HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12b6),
>> - .driver_data = STEELSERIES_ARCTIS_1 },
>> + .driver_data = STEELSERIES_ARCTIS_1 },
>
> Whitespace change.
>
>> +
>> + { /* SteelSeries Arctis 9 Wireless for XBox */
>> + HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12c2),
>> + .driver_data = STEELSERIES_ARCTIS_9 },
>>
>> { }
>> };
>> @@ -683,3 +757,4 @@ MODULE_DESCRIPTION("HID driver for Steelseries devices");
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("Bastien Nocera <hadess@...ess.net>");
>> MODULE_AUTHOR("Simon Wood <simon@...gewell.org>");
>> +MODULE_AUTHOR("Christian Mayer <git@...er-bgk.de>");
>
Powered by blists - more mailing lists