* [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver
@ 2026-05-12 20:00 Vicki Pfau
2026-05-12 20:00 ` [PATCH v5 1/3] " Vicki Pfau
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Vicki Pfau @ 2026-05-12 20:00 UTC (permalink / raw)
To: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, linux-input; +Cc: Vicki Pfau
This series adds preliminary support for Switch 2 controllers using the
same split-driver model as previous versions. This is a minor iteration on
v4, fixing some leaked input_dev objects in error paths as well as fixing a
merge/rebase error when CONFIG_PM ifdefs got removed in 7.0.
Vicki Pfau (3):
HID: nintendo: Add preliminary Switch 2 controller driver
HID: nintendo: Add rumble support for Switch 2 controllers
HID: nintendo: Add unified report format support
MAINTAINERS | 1 +
drivers/hid/Kconfig | 19 +-
drivers/hid/hid-ids.h | 4 +
drivers/hid/hid-nintendo.c | 1504 ++++++++++++++++-
drivers/hid/hid-nintendo.h | 72 +
drivers/input/joystick/Kconfig | 11 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/nintendo-switch2-usb.c | 353 ++++
8 files changed, 1951 insertions(+), 14 deletions(-)
create mode 100644 drivers/hid/hid-nintendo.h
create mode 100644 drivers/input/joystick/nintendo-switch2-usb.c
--
2.54.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver 2026-05-12 20:00 [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Vicki Pfau @ 2026-05-12 20:00 ` Vicki Pfau 2026-05-14 0:15 ` sashiko-bot 2026-06-22 9:47 ` [PATCH] HID: nintendo: fix clang build of switch2_probe guard/goto Ryan McClelland 2026-05-12 20:00 ` [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers Vicki Pfau ` (2 subsequent siblings) 3 siblings, 2 replies; 15+ messages in thread From: Vicki Pfau @ 2026-05-12 20:00 UTC (permalink / raw) To: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, linux-input Cc: Vicki Pfau, Silvan Jegen This adds a new driver for the Switch 2 controllers. The Switch 2 uses an unusual split-interface design such that input and rumble occur on the main HID interface, but all other communication occurs over a "configuration" interface. This is the case on both USB and Bluetooth, so this new driver uses a split-driver design with the HID interface being the "main" driver and the configuration interface is a secondary driver that looks up to the HID interface, sharing resources on a common struct. Due to using a non-standard pairing interface as well as Bluetooth communications being extremely limited in the kernel, a custom interface between userspace and the kernel will need to be designed, along with bringup in BlueZ. That is beyond the scope of this initial patch, which only contains the generic HID and USB configuration interface drivers. This initial work supports general input for the Joy-Con 2, Pro Controller 2, and GameCube NSO controllers. IMU, rumble and battery support is not yet present. Reviewed-by: Silvan Jegen <s.jegen@gmail.com> Tested-by: Silvan Jegen <s.jegen@gmail.com> Signed-off-by: Vicki Pfau <vi@endrift.com> --- MAINTAINERS | 1 + drivers/hid/Kconfig | 11 +- drivers/hid/hid-ids.h | 4 + drivers/hid/hid-nintendo.c | 1191 ++++++++++++++++- drivers/hid/hid-nintendo.h | 72 + drivers/input/joystick/Kconfig | 11 + drivers/input/joystick/Makefile | 1 + drivers/input/joystick/nintendo-switch2-usb.c | 353 +++++ 8 files changed, 1634 insertions(+), 10 deletions(-) create mode 100644 drivers/hid/hid-nintendo.h create mode 100644 drivers/input/joystick/nintendo-switch2-usb.c diff --git a/MAINTAINERS b/MAINTAINERS index d317e2ca65c1..9ebecc0297a9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18882,6 +18882,7 @@ F: drivers/scsi/nsp32* NINTENDO HID DRIVER M: Daniel J. Ogorchock <djogorchock@gmail.com> +M: Vicki Pfau <vi@endrift.com> L: linux-input@vger.kernel.org S: Maintained F: drivers/hid/hid-nintendo* diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index f9bcaeb66385..19c77c323ec9 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -843,10 +843,13 @@ config HID_NINTENDO depends on LEDS_CLASS select POWER_SUPPLY help - Adds support for the Nintendo Switch Joy-Cons, NSO, Pro Controller. - All controllers support bluetooth, and the Pro Controller also supports - its USB mode. This also includes support for the Nintendo Switch Online - Controllers which include the NES, Genesis, SNES, and N64 controllers. + Adds support for the Nintendo Switch Joy-Cons, NSO, Pro Controller, as + well as Nintendo Switch 2 Joy-Cons, Pro Controller, and NSO GameCube + controllers. All Switch controllers support bluetooth, and the Pro + Controller also supports its USB mode. This also includes support for + the Nintendo Switch Online Controllers which include the NES, Genesis, + SNES, and N64 controllers. Switch 2 controllers currently only support + USB mode. To compile this driver as a module, choose M here: the module will be called hid-nintendo. diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 20e7d2dbbf7c..0ac906fbbf1c 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -1098,6 +1098,10 @@ #define USB_DEVICE_ID_NINTENDO_SNESCON 0x2017 #define USB_DEVICE_ID_NINTENDO_GENCON 0x201e #define USB_DEVICE_ID_NINTENDO_N64CON 0x2019 +#define USB_DEVICE_ID_NINTENDO_NS2_JOYCONR 0x2066 +#define USB_DEVICE_ID_NINTENDO_NS2_JOYCONL 0x2067 +#define USB_DEVICE_ID_NINTENDO_NS2_PROCON 0x2069 +#define USB_DEVICE_ID_NINTENDO_NS2_GCCON 0x2073 #define USB_VENDOR_ID_NOVATEK 0x0603 #define USB_DEVICE_ID_NOVATEK_PCT 0x0600 diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 29008c2cc530..2193afcdefab 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -1,11 +1,13 @@ // SPDX-License-Identifier: GPL-2.0+ /* - * HID driver for Nintendo Switch Joy-Cons and Pro Controllers + * HID driver for Nintendo Switch Joy-Cons and Pro Controllers, as well as + * Nintendo Switch 2 Joy-Cons, Pro Controller, and GameCube Controller * * Copyright (c) 2019-2021 Daniel J. Ogorchock <djogorchock@gmail.com> * Portions Copyright (c) 2020 Nadia Holmquist Pedersen <nadia@nhp.sh> * Copyright (c) 2022 Emily Strickland <linux@emily.st> * Copyright (c) 2023 Ryan McClelland <rymcclel@gmail.com> + * Copyright (c) 2026 Valve Software * * The following resources/projects were referenced for this driver: * https://github.com/dekuNukem/Nintendo_Switch_Reverse_Engineering @@ -13,6 +15,8 @@ * https://github.com/FrotBot/SwitchProConLinuxUSB * https://github.com/MTCKC/ProconXInput * https://github.com/Davidobot/BetterJoyForCemu + * https://gist.github.com/shinyquagsire23/66f006b46c56216acbaac6c1e2279b64 + * https://github.com/ndeadly/switch2_controller_research * hid-wiimote kernel hid driver * hid-logitech-hidpp driver * hid-sony driver @@ -29,6 +33,7 @@ */ #include "hid-ids.h" +#include "hid-nintendo.h" #include <linux/unaligned.h> #include <linux/delay.h> #include <linux/device.h> @@ -41,6 +46,8 @@ #include <linux/module.h> #include <linux/power_supply.h> #include <linux/spinlock.h> +#include <linux/usb.h> +#include "usbhid/usbhid.h" /* * Reference the url below for the following HID report defines: @@ -2614,7 +2621,7 @@ static int joycon_ctlr_handle_event(struct joycon_ctlr *ctlr, u8 *data, return ret; } -static int nintendo_hid_event(struct hid_device *hdev, +static int joycon_event(struct hid_device *hdev, struct hid_report *report, u8 *raw_data, int size) { struct joycon_ctlr *ctlr = hid_get_drvdata(hdev); @@ -2625,7 +2632,7 @@ static int nintendo_hid_event(struct hid_device *hdev, return joycon_ctlr_handle_event(ctlr, raw_data, size); } -static int nintendo_hid_probe(struct hid_device *hdev, +static int joycon_probe(struct hid_device *hdev, const struct hid_device_id *id) { int ret; @@ -2729,7 +2736,7 @@ static int nintendo_hid_probe(struct hid_device *hdev, return ret; } -static void nintendo_hid_remove(struct hid_device *hdev) +static void joycon_remove(struct hid_device *hdev) { struct joycon_ctlr *ctlr = hid_get_drvdata(hdev); unsigned long flags; @@ -2748,7 +2755,7 @@ static void nintendo_hid_remove(struct hid_device *hdev) hid_hw_stop(hdev); } -static int nintendo_hid_resume(struct hid_device *hdev) +static int joycon_resume(struct hid_device *hdev) { struct joycon_ctlr *ctlr = hid_get_drvdata(hdev); int ret; @@ -2771,7 +2778,7 @@ static int nintendo_hid_resume(struct hid_device *hdev) return ret; } -static int nintendo_hid_suspend(struct hid_device *hdev, pm_message_t message) +static int joycon_suspend(struct hid_device *hdev, pm_message_t message) { struct joycon_ctlr *ctlr = hid_get_drvdata(hdev); @@ -2790,7 +2797,1121 @@ static int nintendo_hid_suspend(struct hid_device *hdev, pm_message_t message) return 0; } +/* + * ============================================================================= + * Switch 2 support + * ============================================================================= + */ +#define NS2_BTNR_B BIT(0) +#define NS2_BTNR_A BIT(1) +#define NS2_BTNR_Y BIT(2) +#define NS2_BTNR_X BIT(3) +#define NS2_BTNR_R BIT(4) +#define NS2_BTNR_ZR BIT(5) +#define NS2_BTNR_PLUS BIT(6) +#define NS2_BTNR_RS BIT(7) + +#define NS2_BTNL_DOWN BIT(0) +#define NS2_BTNL_RIGHT BIT(1) +#define NS2_BTNL_LEFT BIT(2) +#define NS2_BTNL_UP BIT(3) +#define NS2_BTNL_L BIT(4) +#define NS2_BTNL_ZL BIT(5) +#define NS2_BTNL_MINUS BIT(6) +#define NS2_BTNL_LS BIT(7) + +#define NS2_BTN3_C BIT(4) +#define NS2_BTN3_SR BIT(6) +#define NS2_BTN3_SL BIT(7) + +#define NS2_BTN_JCR_HOME BIT(0) +#define NS2_BTN_JCR_GR BIT(2) +#define NS2_BTN_JCR_C NS2_BTN3_C +#define NS2_BTN_JCR_SR NS2_BTN3_SR +#define NS2_BTN_JCR_SL NS2_BTN3_SL + +#define NS2_BTN_JCL_CAPTURE BIT(0) +#define NS2_BTN_JCL_GL BIT(2) +#define NS2_BTN_JCL_SR NS2_BTN3_SR +#define NS2_BTN_JCL_SL NS2_BTN3_SL + +#define NS2_BTN_PRO_HOME BIT(0) +#define NS2_BTN_PRO_CAPTURE BIT(1) +#define NS2_BTN_PRO_GR BIT(2) +#define NS2_BTN_PRO_GL BIT(3) +#define NS2_BTN_PRO_C NS2_BTN3_C + +#define NS2_BTN_GC_HOME BIT(0) +#define NS2_BTN_GC_CAPTURE BIT(1) +#define NS2_BTN_GC_C NS2_BTN3_C + +#define NS2_TRIGGER_RANGE 4095 +#define NS2_AXIS_MIN -32768 +#define NS2_AXIS_MAX 32767 + +#define NS2_MAX_PLAYER_ID 8 + +#define NS2_MAX_INIT_RETRIES 4 + +#define NS2_FLASH_ADDR_SERIAL 0x13002 +#define NS2_FLASH_ADDR_FACTORY_PRIMARY_CALIB 0x130a8 +#define NS2_FLASH_ADDR_FACTORY_SECONDARY_CALIB 0x130e8 +#define NS2_FLASH_ADDR_FACTORY_TRIGGER_CALIB 0x13140 +#define NS2_FLASH_ADDR_USER_PRIMARY_CALIB 0x1fc040 +#define NS2_FLASH_ADDR_USER_SECONDARY_CALIB 0x1fc080 + +#define NS2_FLASH_SIZE_SERIAL 0x10 +#define NS2_FLASH_SIZE_FACTORY_AXIS_CALIB 9 +#define NS2_FLASH_SIZE_FACTORY_TRIGGER_CALIB 2 +#define NS2_FLASH_SIZE_USER_AXIS_CALIB 11 + +#define NS2_USER_CALIB_MAGIC 0xa1b2 + +#define NS2_FEATURE_BUTTONS BIT(0) +#define NS2_FEATURE_ANALOG BIT(1) +#define NS2_FEATURE_IMU BIT(2) +#define NS2_FEATURE_MOUSE BIT(4) +#define NS2_FEATURE_RUMBLE BIT(5) +#define NS2_FEATURE_MAGNETO BIT(7) + +enum switch2_subcmd_flash { + NS2_SUBCMD_FLASH_READ_BLOCK = 0x01, + NS2_SUBCMD_FLASH_WRITE_BLOCK = 0x02, + NS2_SUBCMD_FLASH_ERASE_BLOCK = 0x03, + NS2_SUBCMD_FLASH_READ = 0x04, + NS2_SUBCMD_FLASH_WRITE = 0x05, +}; + +enum switch2_subcmd_init { + NS2_SUBCMD_INIT_SELECT_REPORT = 0xa, + NS2_SUBCMD_INIT_USB = 0xd, +}; + +enum switch2_subcmd_feature_select { + NS2_SUBCMD_FEATSEL_GET_INFO = 0x1, + NS2_SUBCMD_FEATSEL_SET_MASK = 0x2, + NS2_SUBCMD_FEATSEL_CLEAR_MASK = 0x3, + NS2_SUBCMD_FEATSEL_ENABLE = 0x4, + NS2_SUBCMD_FEATSEL_DISABLE = 0x5, +}; + +enum switch2_subcmd_grip { + NS2_SUBCMD_GRIP_GET_INFO = 0x1, + NS2_SUBCMD_GRIP_ENABLE_BUTTONS = 0x2, + NS2_SUBCMD_GRIP_GET_INFO_EXT = 0x3, +}; + +enum switch2_subcmd_led { + NS2_SUBCMD_LED_P1 = 0x1, + NS2_SUBCMD_LED_P2 = 0x2, + NS2_SUBCMD_LED_P3 = 0x3, + NS2_SUBCMD_LED_P4 = 0x4, + NS2_SUBCMD_LED_ALL_ON = 0x5, + NS2_SUBCMD_LED_ALL_OFF = 0x6, + NS2_SUBCMD_LED_PATTERN = 0x7, + NS2_SUBCMD_LED_BLINK = 0x8, +}; + +enum switch2_subcmd_fw_info { + NS2_SUBCMD_FW_INFO_GET = 0x1, +}; + +enum switch2_ctlr_type { + NS2_CTLR_TYPE_JCL = 0x00, + NS2_CTLR_TYPE_JCR = 0x01, + NS2_CTLR_TYPE_PRO = 0x02, + NS2_CTLR_TYPE_GC = 0x03, +}; + +enum switch2_report_id { + NS2_REPORT_UNIFIED = 0x05, + NS2_REPORT_JCL = 0x07, + NS2_REPORT_JCR = 0x08, + NS2_REPORT_PRO = 0x09, + NS2_REPORT_GC = 0x0a, +}; + +enum switch2_init_step { + NS2_INIT_READ_SERIAL, + NS2_INIT_GET_FIRMWARE_INFO, + NS2_INIT_READ_FACTORY_PRIMARY_CALIB, + NS2_INIT_READ_FACTORY_SECONDARY_CALIB, + NS2_INIT_READ_FACTORY_TRIGGER_CALIB, + NS2_INIT_READ_USER_PRIMARY_CALIB, + NS2_INIT_READ_USER_SECONDARY_CALIB, + NS2_INIT_SET_FEATURE_MASK, + NS2_INIT_ENABLE_FEATURES, + NS2_INIT_GRIP_BUTTONS, + NS2_INIT_REPORT_FORMAT, + NS2_INIT_SET_PLAYER_LEDS, + NS2_INIT_INPUT, + NS2_INIT_FINISH, + NS2_INIT_DONE, +}; + +struct switch2_version_info { + uint8_t major; + uint8_t minor; + uint8_t patch; + uint8_t ctlr_type; + __le32 unk; + int8_t dsp_major; + int8_t dsp_minor; + int8_t dsp_patch; + int8_t dsp_type; +}; + +struct switch2_axis_calibration { + uint16_t neutral; + uint16_t negative; + uint16_t positive; +}; + +struct switch2_stick_calibration { + struct switch2_axis_calibration x; + struct switch2_axis_calibration y; +}; + +struct switch2_controller { + struct hid_device *hdev; + struct switch2_cfg_intf *cfg; + + char name[64]; + char phys[64]; + struct list_head entry; + struct mutex lock; + + enum switch2_ctlr_type ctlr_type; + enum switch2_init_step init_step; + struct input_dev __rcu *input; + char serial[NS2_FLASH_SIZE_SERIAL + 1]; + struct switch2_version_info version; + + struct switch2_stick_calibration stick_calib[2]; + uint8_t lt_zero; + uint8_t rt_zero; + + uint32_t player_id; + struct led_classdev leds[4]; +}; + +static DEFINE_MUTEX(switch2_controllers_lock); +static LIST_HEAD(switch2_controllers); + +struct switch2_ctlr_button_mapping { + uint32_t code; + int byte; + uint32_t bit; +}; + +static const struct switch2_ctlr_button_mapping ns2_left_joycon_button_mappings[] = { + { BTN_DPAD_LEFT, 0, NS2_BTNL_LEFT, }, + { BTN_DPAD_UP, 0, NS2_BTNL_UP, }, + { BTN_DPAD_DOWN, 0, NS2_BTNL_DOWN, }, + { BTN_DPAD_RIGHT, 0, NS2_BTNL_RIGHT, }, + { BTN_TL, 0, NS2_BTNL_L, }, + { BTN_TL2, 0, NS2_BTNL_ZL, }, + { BTN_SELECT, 0, NS2_BTNL_MINUS, }, + { BTN_THUMBL, 0, NS2_BTNL_LS, }, + { KEY_RECORD, 1, NS2_BTN_JCL_CAPTURE, }, + { BTN_GRIPR, 1, NS2_BTN_JCL_SL, }, + { BTN_GRIPR2, 1, NS2_BTN_JCL_SR, }, + { BTN_GRIPL, 1, NS2_BTN_JCL_GL, }, + { /* sentinel */ }, +}; + +static const struct switch2_ctlr_button_mapping ns2_right_joycon_button_mappings[] = { + { BTN_SOUTH, 0, NS2_BTNR_A, }, + { BTN_EAST, 0, NS2_BTNR_B, }, + { BTN_NORTH, 0, NS2_BTNR_X, }, + { BTN_WEST, 0, NS2_BTNR_Y, }, + { BTN_TR, 0, NS2_BTNR_R, }, + { BTN_TR2, 0, NS2_BTNR_ZR, }, + { BTN_START, 0, NS2_BTNR_PLUS, }, + { BTN_THUMBR, 0, NS2_BTNR_RS, }, + { BTN_C, 1, NS2_BTN_JCR_C, }, + { BTN_MODE, 1, NS2_BTN_JCR_HOME, }, + { BTN_GRIPL2, 1, NS2_BTN_JCR_SL, }, + { BTN_GRIPL, 1, NS2_BTN_JCR_SR, }, + { BTN_GRIPR, 1, NS2_BTN_JCR_GR, }, + { /* sentinel */ }, +}; + +static const struct switch2_ctlr_button_mapping ns2_procon_mappings[] = { + { BTN_SOUTH, 0, NS2_BTNR_A, }, + { BTN_EAST, 0, NS2_BTNR_B, }, + { BTN_NORTH, 0, NS2_BTNR_X, }, + { BTN_WEST, 0, NS2_BTNR_Y, }, + { BTN_TL, 1, NS2_BTNL_L, }, + { BTN_TR, 0, NS2_BTNR_R, }, + { BTN_TL2, 1, NS2_BTNL_ZL, }, + { BTN_TR2, 0, NS2_BTNR_ZR, }, + { BTN_SELECT, 1, NS2_BTNL_MINUS, }, + { BTN_START, 0, NS2_BTNR_PLUS, }, + { BTN_THUMBL, 1, NS2_BTNL_LS, }, + { BTN_THUMBR, 0, NS2_BTNR_RS, }, + { BTN_MODE, 2, NS2_BTN_PRO_HOME }, + { KEY_RECORD, 2, NS2_BTN_PRO_CAPTURE }, + { BTN_GRIPR, 2, NS2_BTN_PRO_GR }, + { BTN_GRIPL, 2, NS2_BTN_PRO_GL }, + { BTN_C, 2, NS2_BTN_PRO_C }, + { /* sentinel */ }, +}; + +static const struct switch2_ctlr_button_mapping ns2_gccon_mappings[] = { + { BTN_SOUTH, 0, NS2_BTNR_A, }, + { BTN_EAST, 0, NS2_BTNR_B, }, + { BTN_NORTH, 0, NS2_BTNR_X, }, + { BTN_WEST, 0, NS2_BTNR_Y, }, + { BTN_TL2, 1, NS2_BTNL_L, }, + { BTN_TR2, 0, NS2_BTNR_R, }, + { BTN_TL, 1, NS2_BTNL_ZL, }, + { BTN_TR, 0, NS2_BTNR_ZR, }, + { BTN_SELECT, 1, NS2_BTNL_MINUS, }, + { BTN_START, 0, NS2_BTNR_PLUS, }, + { BTN_MODE, 2, NS2_BTN_GC_HOME }, + { KEY_RECORD, 2, NS2_BTN_GC_CAPTURE }, + { BTN_C, 2, NS2_BTN_GC_C }, + { /* sentinel */ }, +}; + +static const uint8_t switch2_init_cmd_data[] = { + /* + * The last 6 bytes of this packet are the MAC address of + * the console, but we don't need that for USB + */ + 0x01, 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF +}; + +static const uint8_t switch2_one_data[] = { 0x01, 0x00, 0x00, 0x00 }; + +static const uint8_t switch2_feature_mask[] = { + NS2_FEATURE_BUTTONS | NS2_FEATURE_ANALOG | NS2_FEATURE_IMU, + 0x00, 0x00, 0x00 +}; + +static void switch2_init_step_done(struct switch2_controller *ns2, enum switch2_init_step step) +{ + if (ns2->init_step != step) + return; + + ns2->init_step++; +} + +static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type) +{ + return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR; +} + +static int switch2_set_leds(struct switch2_controller *ns2) +{ + int i; + uint8_t message[8] = { 0 }; + + for (i = 0; i < JC_NUM_LEDS; i++) + message[0] |= (!!ns2->leds[i].brightness) << i; + + if (!ns2->cfg) + return -ENOTCONN; + return ns2->cfg->send_command(NS2_CMD_LED, NS2_SUBCMD_LED_PATTERN, + &message, sizeof(message), + ns2->cfg); +} + +static int switch2_player_led_brightness_set(struct led_classdev *led, + enum led_brightness brightness) +{ + struct device *dev = led->dev->parent; + struct hid_device *hdev = to_hid_device(dev); + struct switch2_controller *ns2 = hid_get_drvdata(hdev); + + if (!ns2) + return -ENODEV; + + guard(mutex)(&ns2->lock); + return switch2_set_leds(ns2); +} + +static void switch2_leds_create(struct switch2_controller *ns2) +{ + struct hid_device *hdev = ns2->hdev; + struct led_classdev *led; + int i; + int player_led_pattern; + + player_led_pattern = ns2->player_id % JC_NUM_LED_PATTERNS; + hid_dbg(hdev, "assigned player %d led pattern", player_led_pattern + 1); + + for (i = 0; i < JC_NUM_LEDS; i++) { + led = &ns2->leds[i]; + led->brightness = joycon_player_led_patterns[player_led_pattern][i]; + led->max_brightness = 1; + led->brightness_set_blocking = switch2_player_led_brightness_set; + led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE; + } +} + +static void switch2_config_buttons(struct input_dev *idev, + const struct switch2_ctlr_button_mapping button_mappings[]) +{ + const struct switch2_ctlr_button_mapping *button; + + for (button = button_mappings; button->code; button++) + input_set_capability(idev, EV_KEY, button->code); +} + +static int switch2_init_input(struct switch2_controller *ns2) +{ + struct input_dev *input; + struct hid_device *hdev = ns2->hdev; + int i; + int ret; + + switch2_init_step_done(ns2, NS2_INIT_FINISH); + + rcu_read_lock(); + input = rcu_dereference(ns2->input); + rcu_read_unlock(); + + if (input) + return 0; + + input = devm_input_allocate_device(&hdev->dev); + if (!input) + return -ENOMEM; + + input_set_drvdata(input, ns2); + input->dev.parent = &hdev->dev; + input->id.bustype = hdev->bus; + input->id.vendor = hdev->vendor; + input->id.product = hdev->product; + input->id.version = hdev->version; + input->uniq = ns2->serial; + input->name = ns2->name; + input->phys = hdev->phys; + + switch (ns2->ctlr_type) { + case NS2_CTLR_TYPE_JCL: + input_set_abs_params(input, ABS_X, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_Y, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + switch2_config_buttons(input, ns2_left_joycon_button_mappings); + break; + case NS2_CTLR_TYPE_JCR: + input_set_abs_params(input, ABS_X, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_Y, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + switch2_config_buttons(input, ns2_right_joycon_button_mappings); + break; + case NS2_CTLR_TYPE_GC: + input_set_abs_params(input, ABS_X, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_Y, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_RX, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_RY, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_Z, 0, NS2_TRIGGER_RANGE, 32, 128); + input_set_abs_params(input, ABS_RZ, 0, NS2_TRIGGER_RANGE, 32, 128); + input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0); + input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0); + switch2_config_buttons(input, ns2_gccon_mappings); + break; + case NS2_CTLR_TYPE_PRO: + input_set_abs_params(input, ABS_X, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_Y, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_RX, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_RY, NS2_AXIS_MIN, NS2_AXIS_MAX, 32, 128); + input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0); + input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0); + switch2_config_buttons(input, ns2_procon_mappings); + break; + default: + input_free_device(input); + return -EINVAL; + } + + hid_info(ns2->hdev, "Firmware version %u.%u.%u (type %i)\n", ns2->version.major, + ns2->version.minor, ns2->version.patch, ns2->version.ctlr_type); + if (ns2->version.dsp_type >= 0) + hid_info(ns2->hdev, "DSP version %u.%u.%u\n", ns2->version.dsp_major, + ns2->version.dsp_minor, ns2->version.dsp_patch); + + ret = input_register_device(input); + if (ret < 0) { + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret); + input_free_device(input); + return ret; + } + + for (i = 0; i < JC_NUM_LEDS; i++) { + struct led_classdev *led = &ns2->leds[i]; + char *name = devm_kasprintf(&input->dev, GFP_KERNEL, "%s:%s:%s", + dev_name(&input->dev), + "green", + joycon_player_led_names[i]); + + if (!name) { + input_unregister_device(input); + return -ENOMEM; + } + + led->name = name; + ret = devm_led_classdev_register(&input->dev, led); + if (ret < 0) { + dev_err(&input->dev, "Failed to register player %d LED; ret=%d\n", + i + 1, ret); + input_unregister_device(input); + return ret; + } + } + + rcu_assign_pointer(ns2->input, input); + synchronize_rcu(); + return 0; +} + +static struct switch2_controller *switch2_get_controller(const char *phys) +{ + struct switch2_controller *ns2; + + guard(mutex)(&switch2_controllers_lock); + list_for_each_entry(ns2, &switch2_controllers, entry) { + if (strncmp(ns2->phys, phys, sizeof(ns2->phys)) == 0) + return ns2; + } + ns2 = kzalloc(sizeof(*ns2), GFP_KERNEL); + if (!ns2) + return ERR_PTR(-ENOMEM); + + mutex_init(&ns2->lock); + INIT_LIST_HEAD(&ns2->entry); + list_add(&ns2->entry, &switch2_controllers); + strscpy(ns2->phys, phys, sizeof(ns2->phys)); + return ns2; +} + +static void switch2_controller_put(struct switch2_controller *ns2) +{ + struct input_dev *input; + bool do_free; + + guard(mutex)(&switch2_controllers_lock); + mutex_lock(&ns2->lock); + + rcu_read_lock(); + input = rcu_dereference(ns2->input); + rcu_read_unlock(); + + rcu_assign_pointer(ns2->input, NULL); + synchronize_rcu(); + + ns2->init_step = 0; + do_free = !ns2->hdev && !ns2->cfg; + mutex_unlock(&ns2->lock); + + if (input) + input_unregister_device(input); + + if (do_free) { + list_del_init(&ns2->entry); + mutex_destroy(&ns2->lock); + kfree(ns2); + } +} + +static bool switch2_parse_stick_calibration(struct switch2_stick_calibration *calib, + const uint8_t *data) +{ + static const uint8_t UNCALIBRATED[9] = { + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF + }; + if (memcmp(UNCALIBRATED, data, sizeof(UNCALIBRATED)) == 0) + return false; + + calib->x.neutral = data[0]; + calib->x.neutral |= (data[1] & 0x0F) << 8; + + calib->y.neutral = data[1] >> 4; + calib->y.neutral |= data[2] << 4; + + calib->x.positive = data[3]; + calib->x.positive |= (data[4] & 0x0F) << 8; + + calib->y.positive = data[4] >> 4; + calib->y.positive |= data[5] << 4; + + calib->x.negative = data[6]; + calib->x.negative |= (data[7] & 0x0F) << 8; + + calib->y.negative = data[7] >> 4; + calib->y.negative |= data[8] << 4; + + return true; +} + +static void switch2_handle_flash_read(struct switch2_controller *ns2, uint8_t size, + uint32_t address, const uint8_t *data) +{ + bool ok; + + switch (address) { + case NS2_FLASH_ADDR_SERIAL: + if (size != NS2_FLASH_SIZE_SERIAL) + return; + memcpy(ns2->serial, data, size); + switch2_init_step_done(ns2, NS2_INIT_READ_SERIAL); + break; + case NS2_FLASH_ADDR_FACTORY_PRIMARY_CALIB: + if (size != NS2_FLASH_SIZE_FACTORY_AXIS_CALIB) + return; + switch2_init_step_done(ns2, NS2_INIT_READ_FACTORY_PRIMARY_CALIB); + ok = switch2_parse_stick_calibration(&ns2->stick_calib[0], data); + if (ok) { + hid_dbg(ns2->hdev, "Got factory primary stick calibration:\n"); + hid_dbg(ns2->hdev, "Left max: %i, neutral: %i, right max: %i\n", + ns2->stick_calib[0].x.negative, + ns2->stick_calib[0].x.neutral, + ns2->stick_calib[0].x.positive); + hid_dbg(ns2->hdev, "Down max: %i, neutral: %i, up max: %i\n", + ns2->stick_calib[0].y.negative, + ns2->stick_calib[0].y.neutral, + ns2->stick_calib[0].y.positive); + } else { + hid_dbg(ns2->hdev, "Factory primary stick calibration not present\n"); + } + break; + case NS2_FLASH_ADDR_FACTORY_SECONDARY_CALIB: + if (size != NS2_FLASH_SIZE_FACTORY_AXIS_CALIB) + return; + switch2_init_step_done(ns2, NS2_INIT_READ_FACTORY_SECONDARY_CALIB); + ok = switch2_parse_stick_calibration(&ns2->stick_calib[1], data); + if (ok) { + hid_dbg(ns2->hdev, "Got factory secondary stick calibration:\n"); + hid_dbg(ns2->hdev, "Left max: %i, neutral: %i, right max: %i\n", + ns2->stick_calib[1].x.negative, + ns2->stick_calib[1].x.neutral, + ns2->stick_calib[1].x.positive); + hid_dbg(ns2->hdev, "Down max: %i, neutral: %i, up max: %i\n", + ns2->stick_calib[1].y.negative, + ns2->stick_calib[1].y.neutral, + ns2->stick_calib[1].y.positive); + } else { + hid_dbg(ns2->hdev, "Factory secondary stick calibration not present\n"); + } + break; + case NS2_FLASH_ADDR_FACTORY_TRIGGER_CALIB: + if (size != NS2_FLASH_SIZE_FACTORY_TRIGGER_CALIB) + return; + switch2_init_step_done(ns2, NS2_INIT_READ_FACTORY_TRIGGER_CALIB); + if (data[0] != 0xFF && data[1] != 0xFF) { + ns2->lt_zero = data[0]; + ns2->rt_zero = data[1]; + + hid_dbg(ns2->hdev, "Got factory trigger calibration:\n"); + hid_dbg(ns2->hdev, "Left zero point: %i\n", ns2->lt_zero); + hid_dbg(ns2->hdev, "Right zero point: %i\n", ns2->rt_zero); + } else { + hid_dbg(ns2->hdev, "Factory trigger calibration not present\n"); + } + break; + case NS2_FLASH_ADDR_USER_PRIMARY_CALIB: + if (size != NS2_FLASH_SIZE_USER_AXIS_CALIB) + return; + switch2_init_step_done(ns2, NS2_INIT_READ_USER_PRIMARY_CALIB); + if (__le16_to_cpu(*(__le16 *)data) != NS2_USER_CALIB_MAGIC) { + hid_dbg(ns2->hdev, "No user primary stick calibration present\n"); + break; + } + + ok = switch2_parse_stick_calibration(&ns2->stick_calib[0], &data[2]); + if (ok) { + hid_dbg(ns2->hdev, "Got user primary stick calibration:\n"); + hid_dbg(ns2->hdev, "Left max: %i, neutral: %i, right max: %i\n", + ns2->stick_calib[0].x.negative, + ns2->stick_calib[0].x.neutral, + ns2->stick_calib[0].x.positive); + hid_dbg(ns2->hdev, "Down max: %i, neutral: %i, up max: %i\n", + ns2->stick_calib[0].y.negative, + ns2->stick_calib[0].y.neutral, + ns2->stick_calib[0].y.positive); + } else { + hid_dbg(ns2->hdev, "No user primary stick calibration present\n"); + } + break; + case NS2_FLASH_ADDR_USER_SECONDARY_CALIB: + if (size != NS2_FLASH_SIZE_USER_AXIS_CALIB) + return; + switch2_init_step_done(ns2, NS2_INIT_READ_USER_SECONDARY_CALIB); + if (__le16_to_cpu(*(__le16 *)data) != NS2_USER_CALIB_MAGIC) { + hid_dbg(ns2->hdev, "No user secondary stick calibration present\n"); + break; + } + + ok = switch2_parse_stick_calibration(&ns2->stick_calib[1], &data[2]); + if (ok) { + hid_dbg(ns2->hdev, "Got user secondary stick calibration:\n"); + hid_dbg(ns2->hdev, "Left max: %i, neutral: %i, right max: %i\n", + ns2->stick_calib[1].x.negative, + ns2->stick_calib[1].x.neutral, + ns2->stick_calib[1].x.positive); + hid_dbg(ns2->hdev, "Down max: %i, neutral: %i, up max: %i\n", + ns2->stick_calib[1].y.negative, + ns2->stick_calib[1].y.neutral, + ns2->stick_calib[1].y.positive); + } else { + hid_dbg(ns2->hdev, "No user secondary stick calibration present\n"); + } + break; + } +} + +static void switch2_report_buttons(struct input_dev *input, const uint8_t *bytes, + const struct switch2_ctlr_button_mapping button_mappings[]) +{ + const struct switch2_ctlr_button_mapping *button; + + for (button = button_mappings; button->code; button++) + input_report_key(input, button->code, bytes[button->byte] & button->bit); +} + +static void switch2_report_axis(struct input_dev *input, struct switch2_axis_calibration *calib, + int axis, bool invert, int value) +{ + if (calib && calib->neutral && calib->negative && calib->positive) { + value -= calib->neutral; + value *= NS2_AXIS_MAX + 1; + if (value < 0) + value /= calib->negative; + else + value /= calib->positive; + } else { + value = (value - 2048) * 16; + } + + if (invert) + value = -value; + input_report_abs(input, axis, + clamp(value, NS2_AXIS_MIN, NS2_AXIS_MAX)); +} + +static void switch2_report_stick(struct input_dev *input, struct switch2_stick_calibration *calib, + int x, bool invert_x, int y, bool invert_y, const uint8_t *data) +{ + switch2_report_axis(input, &calib->x, x, invert_x, data[0] | ((data[1] & 0x0F) << 8)); + switch2_report_axis(input, &calib->y, y, invert_y, (data[1] >> 4) | (data[2] << 4)); +} + +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data) +{ + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); + + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE)); +} + +static int switch2_event(struct hid_device *hdev, struct hid_report *report, uint8_t *raw_data, + int size) +{ + struct switch2_controller *ns2 = hid_get_drvdata(hdev); + struct input_dev *input; + + if (report->type != HID_INPUT_REPORT) + return 0; + + if (size < 15) + return -EINVAL; + + guard(rcu)(); + input = rcu_dereference(ns2->input); + + if (!input) + return 0; + + switch (report->id) { + case NS2_REPORT_UNIFIED: + /* + * TODO + * This won't be sent unless the report type gets changed via command + * 03-0A, but we should support it at some point regardless. + */ + break; + case NS2_REPORT_JCL: + switch2_report_stick(input, &ns2->stick_calib[0], ABS_X, false, + ABS_Y, true, &raw_data[6]); + switch2_report_buttons(input, &raw_data[3], ns2_left_joycon_button_mappings); + break; + case NS2_REPORT_JCR: + switch2_report_stick(input, &ns2->stick_calib[0], ABS_X, false, + ABS_Y, true, &raw_data[6]); + switch2_report_buttons(input, &raw_data[3], ns2_right_joycon_button_mappings); + break; + case NS2_REPORT_GC: + input_report_abs(input, ABS_HAT0X, + !!(raw_data[4] & NS2_BTNL_RIGHT) - + !!(raw_data[4] & NS2_BTNL_LEFT)); + input_report_abs(input, ABS_HAT0Y, + !!(raw_data[4] & NS2_BTNL_DOWN) - + !!(raw_data[4] & NS2_BTNL_UP)); + switch2_report_buttons(input, &raw_data[3], ns2_gccon_mappings); + switch2_report_stick(input, &ns2->stick_calib[0], ABS_X, false, + ABS_Y, true, &raw_data[6]); + switch2_report_stick(input, &ns2->stick_calib[1], ABS_RX, false, + ABS_RY, true, &raw_data[9]); + switch2_report_trigger(input, ns2->lt_zero, ABS_Z, raw_data[13]); + switch2_report_trigger(input, ns2->rt_zero, ABS_RZ, raw_data[14]); + break; + case NS2_REPORT_PRO: + input_report_abs(input, ABS_HAT0X, + !!(raw_data[4] & NS2_BTNL_RIGHT) - + !!(raw_data[4] & NS2_BTNL_LEFT)); + input_report_abs(input, ABS_HAT0Y, + !!(raw_data[4] & NS2_BTNL_DOWN) - + !!(raw_data[4] & NS2_BTNL_UP)); + switch2_report_buttons(input, &raw_data[3], ns2_procon_mappings); + switch2_report_stick(input, &ns2->stick_calib[0], ABS_X, false, + ABS_Y, true, &raw_data[6]); + switch2_report_stick(input, &ns2->stick_calib[1], ABS_RX, false, + ABS_RY, true, &raw_data[9]); + break; + default: + return -EINVAL; + } + + input_sync(input); + return 0; +} + +static int switch2_features_enable(struct switch2_controller *ns2, int features) +{ + __le32 feature_bits = __cpu_to_le32(features); + + if (!ns2->cfg) + return -ENOTCONN; + return ns2->cfg->send_command(NS2_CMD_FEATSEL, NS2_SUBCMD_FEATSEL_ENABLE, + &feature_bits, sizeof(feature_bits), + ns2->cfg); +} + +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address, + uint8_t size) +{ + uint8_t message[8] = { size, 0x7e }; + + if (!ns2->cfg) + return -ENOTCONN; + *(__le32 *)&message[4] = __cpu_to_le32(address); + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message, + sizeof(message), ns2->cfg); +} + +static int switch2_set_player_id(struct switch2_controller *ns2, uint32_t player_id) +{ + int i; + int player_led_pattern = player_id % JC_NUM_LED_PATTERNS; + + for (i = 0; i < JC_NUM_LEDS; i++) + ns2->leds[i].brightness = joycon_player_led_patterns[player_led_pattern][i]; + + return switch2_set_leds(ns2); +} + +static int switch2_set_report_format(struct switch2_controller *ns2, enum switch2_report_id fmt) +{ + __le32 format_id = __cpu_to_le32(fmt); + + if (!ns2->cfg) + return -ENOTCONN; + return ns2->cfg->send_command(NS2_CMD_INIT, NS2_SUBCMD_INIT_SELECT_REPORT, + &format_id, sizeof(format_id), + ns2->cfg); +} + +static int switch2_init_controller(struct switch2_controller *ns2) +{ + if (ns2->init_step == NS2_INIT_DONE) + return 0; + + if (!ns2->cfg) + return -ENOTCONN; + + switch (ns2->init_step) { + case NS2_INIT_READ_SERIAL: + return switch2_read_flash(ns2, NS2_FLASH_ADDR_SERIAL, + NS2_FLASH_SIZE_SERIAL); + case NS2_INIT_GET_FIRMWARE_INFO: + return ns2->cfg->send_command(NS2_CMD_FW_INFO, NS2_SUBCMD_FW_INFO_GET, + NULL, 0, ns2->cfg); + case NS2_INIT_READ_FACTORY_PRIMARY_CALIB: + return switch2_read_flash(ns2, NS2_FLASH_ADDR_FACTORY_PRIMARY_CALIB, + NS2_FLASH_SIZE_FACTORY_AXIS_CALIB); + case NS2_INIT_READ_FACTORY_SECONDARY_CALIB: + if (switch2_ctlr_is_joycon(ns2->ctlr_type)) { + switch2_init_step_done(ns2, ns2->init_step); + return switch2_init_controller(ns2); + } + return switch2_read_flash(ns2, NS2_FLASH_ADDR_FACTORY_SECONDARY_CALIB, + NS2_FLASH_SIZE_FACTORY_AXIS_CALIB); + case NS2_INIT_READ_FACTORY_TRIGGER_CALIB: + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) { + switch2_init_step_done(ns2, ns2->init_step); + return switch2_init_controller(ns2); + } + return switch2_read_flash(ns2, NS2_FLASH_ADDR_FACTORY_TRIGGER_CALIB, + NS2_FLASH_SIZE_FACTORY_TRIGGER_CALIB); + case NS2_INIT_READ_USER_PRIMARY_CALIB: + return switch2_read_flash(ns2, NS2_FLASH_ADDR_USER_PRIMARY_CALIB, + NS2_FLASH_SIZE_USER_AXIS_CALIB); + case NS2_INIT_READ_USER_SECONDARY_CALIB: + if (switch2_ctlr_is_joycon(ns2->ctlr_type)) { + switch2_init_step_done(ns2, ns2->init_step); + return switch2_init_controller(ns2); + } + return switch2_read_flash(ns2, NS2_FLASH_ADDR_USER_SECONDARY_CALIB, + NS2_FLASH_SIZE_USER_AXIS_CALIB); + case NS2_INIT_SET_FEATURE_MASK: + return ns2->cfg->send_command(NS2_CMD_FEATSEL, NS2_SUBCMD_FEATSEL_SET_MASK, + switch2_feature_mask, sizeof(switch2_feature_mask), ns2->cfg); + case NS2_INIT_ENABLE_FEATURES: + return switch2_features_enable(ns2, NS2_FEATURE_BUTTONS | NS2_FEATURE_ANALOG); + case NS2_INIT_GRIP_BUTTONS: + if (!switch2_ctlr_is_joycon(ns2->ctlr_type)) { + switch2_init_step_done(ns2, ns2->init_step); + return switch2_init_controller(ns2); + } + return ns2->cfg->send_command(NS2_CMD_GRIP, NS2_SUBCMD_GRIP_ENABLE_BUTTONS, + switch2_one_data, sizeof(switch2_one_data), + ns2->cfg); + case NS2_INIT_REPORT_FORMAT: + switch (ns2->ctlr_type) { + case NS2_CTLR_TYPE_JCL: + return switch2_set_report_format(ns2, NS2_REPORT_JCL); + case NS2_CTLR_TYPE_JCR: + return switch2_set_report_format(ns2, NS2_REPORT_JCR); + case NS2_CTLR_TYPE_PRO: + return switch2_set_report_format(ns2, NS2_REPORT_PRO); + case NS2_CTLR_TYPE_GC: + return switch2_set_report_format(ns2, NS2_REPORT_GC); + default: + switch2_init_step_done(ns2, ns2->init_step); + return switch2_init_controller(ns2); + } + case NS2_INIT_SET_PLAYER_LEDS: + return switch2_set_player_id(ns2, ns2->player_id); + case NS2_INIT_INPUT: + return ns2->cfg->send_command(NS2_CMD_INIT, NS2_SUBCMD_INIT_USB, + switch2_init_cmd_data, sizeof(switch2_init_cmd_data), ns2->cfg); + case NS2_INIT_FINISH: + if (ns2->hdev) + return switch2_init_input(ns2); + break; + default: + WARN_ON_ONCE(1); + break; + } + return 0; +} + +int switch2_receive_command(struct switch2_controller *ns2, + const uint8_t *message, size_t length) +{ + const struct switch2_cmd_header *header; + int ret = 0; + + if (length < 8) + return -EINVAL; + + print_hex_dump_debug("got cmd: ", DUMP_PREFIX_OFFSET, 16, 1, message, length, false); + + guard(mutex)(&ns2->lock); + + header = (const struct switch2_cmd_header *)message; + if (!(header->flags & NS2_FLAG_OK)) { + ret = -EIO; + goto exit; + } + message = &message[8]; + switch (header->command) { + case NS2_CMD_FLASH: + if (header->subcommand == NS2_SUBCMD_FLASH_READ) { + uint8_t read_size; + uint32_t read_address; + + if (length < 16) { + ret = -EINVAL; + goto exit; + } + read_size = message[0]; + read_address = __le32_to_cpu(*(__le32 *)&message[4]); + if (length < read_size + 16) { + ret = -EINVAL; + goto exit; + } + switch2_handle_flash_read(ns2, read_size, read_address, &message[8]); + } + break; + case NS2_CMD_INIT: + if (header->subcommand == NS2_SUBCMD_INIT_USB) + switch2_init_step_done(ns2, NS2_INIT_INPUT); + else if (header->subcommand == NS2_SUBCMD_INIT_SELECT_REPORT) + switch2_init_step_done(ns2, NS2_INIT_REPORT_FORMAT); + break; + case NS2_CMD_GRIP: + if (header->subcommand == NS2_SUBCMD_GRIP_ENABLE_BUTTONS) + switch2_init_step_done(ns2, NS2_INIT_GRIP_BUTTONS); + break; + case NS2_CMD_LED: + if (header->subcommand == NS2_SUBCMD_LED_PATTERN) + switch2_init_step_done(ns2, NS2_INIT_SET_PLAYER_LEDS); + break; + case NS2_CMD_FEATSEL: + if (header->subcommand == NS2_SUBCMD_FEATSEL_SET_MASK) + switch2_init_step_done(ns2, NS2_INIT_SET_FEATURE_MASK); + else if (header->subcommand == NS2_SUBCMD_FEATSEL_ENABLE) + switch2_init_step_done(ns2, NS2_INIT_ENABLE_FEATURES); + break; + case NS2_CMD_FW_INFO: + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) { + if (length < sizeof(ns2->version)) { + ret = -EINVAL; + goto exit; + } + memcpy(&ns2->version, message, sizeof(ns2->version)); + ns2->ctlr_type = ns2->version.ctlr_type; + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO); + } + break; + default: + break; + } + +exit: + if (ns2->init_step < NS2_INIT_DONE) + switch2_init_controller(ns2); + + return ret; +} +EXPORT_SYMBOL_GPL(switch2_receive_command); + +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg) +{ + struct switch2_controller *ns2 = switch2_get_controller(phys); + + if (IS_ERR(ns2)) + return PTR_ERR(ns2); + + cfg->parent = ns2; + + guard(mutex)(&ns2->lock); + WARN_ON(ns2->cfg); + ns2->cfg = cfg; + + if (ns2->hdev) + return switch2_init_controller(ns2); + return 0; +} +EXPORT_SYMBOL_GPL(switch2_controller_attach_cfg); + +void switch2_controller_detach_cfg(struct switch2_controller *ns2) +{ + mutex_lock(&ns2->lock); + WARN_ON(ns2 != ns2->cfg->parent); + ns2->cfg = NULL; + mutex_unlock(&ns2->lock); + switch2_controller_put(ns2); +} +EXPORT_SYMBOL_GPL(switch2_controller_detach_cfg); + +static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id) +{ + struct switch2_controller *ns2; + struct usb_device *udev; + char phys[64]; + int ret; + + if (!hid_is_usb(hdev)) + return -ENODEV; + + udev = hid_to_usb_dev(hdev); + if (usb_make_path(udev, phys, sizeof(phys)) < 0) + return -EINVAL; + + ret = hid_parse(hdev); + if (ret) { + hid_err(hdev, "parse failed %d\n", ret); + return ret; + } + + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW); + if (ret) { + hid_err(hdev, "hw_start failed %d\n", ret); + return ret; + } + + ret = hid_hw_open(hdev); + if (ret) { + hid_err(hdev, "hw_open failed %d\n", ret); + goto err_stop; + } + + ns2 = switch2_get_controller(phys); + if (IS_ERR(ns2)) { + ret = PTR_ERR(ns2); + goto err_close; + } + + guard(mutex)(&ns2->lock); + WARN_ON(ns2->hdev); + ns2->hdev = hdev; + switch (hdev->product | (hdev->vendor << 16)) { + default: + strscpy(ns2->name, hdev->name, sizeof(ns2->name)); + break; + /* Some controllers have slightly wrong names so we override them */ + case USB_DEVICE_ID_NINTENDO_NS2_JOYCONR | (USB_VENDOR_ID_NINTENDO << 16): + /* Missing the "2" in the name */ + strscpy(ns2->name, "Nintendo Joy-Con 2 (R)", sizeof(ns2->name)); + break; + case USB_DEVICE_ID_NINTENDO_NS2_GCCON | (USB_VENDOR_ID_NINTENDO << 16): + /* Has "Nintendo" in the name twice */ + strscpy(ns2->name, "Nintendo GameCube Controller", sizeof(ns2->name)); + break; + } + + ns2->player_id = U32_MAX; + ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL); + if (ret < 0) + hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret); + else + ns2->player_id = ret; + + switch2_leds_create(ns2); + + hid_set_drvdata(hdev, ns2); + + if (ns2->cfg) + return switch2_init_controller(ns2); + + return 0; + +err_close: + hid_hw_close(hdev); +err_stop: + hid_hw_stop(hdev); + + return ret; +} + +static void switch2_remove(struct hid_device *hdev) +{ + struct switch2_controller *ns2 = hid_get_drvdata(hdev); + + hid_hw_close(hdev); + mutex_lock(&ns2->lock); + WARN_ON(ns2->hdev != hdev); + ns2->hdev = NULL; + mutex_unlock(&ns2->lock); + ida_free(&nintendo_player_id_allocator, ns2->player_id); + switch2_controller_put(ns2); + hid_hw_stop(hdev); +} + static const struct hid_device_id nintendo_hid_devices[] = { + /* Switch devices */ { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_PROCON) }, { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, @@ -2813,10 +3934,67 @@ static const struct hid_device_id nintendo_hid_devices[] = { USB_DEVICE_ID_NINTENDO_GENCON) }, { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_N64CON) }, + /* Switch 2 devices */ + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_NS2_JOYCONL) }, + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_NS2_JOYCONR) }, + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_NS2_PROCON) }, + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_NS2_GCCON) }, { } }; MODULE_DEVICE_TABLE(hid, nintendo_hid_devices); +static bool nintendo_is_switch2(struct hid_device *hdev) +{ + return hdev->vendor == USB_VENDOR_ID_NINTENDO && + hdev->product >= USB_DEVICE_ID_NINTENDO_NS2_JOYCONR; +} + +static void nintendo_hid_remove(struct hid_device *hdev) +{ + if (nintendo_is_switch2(hdev)) + switch2_remove(hdev); + else + joycon_remove(hdev); +} + +static int nintendo_hid_event(struct hid_device *hdev, + struct hid_report *report, u8 *raw_data, int size) +{ + if (nintendo_is_switch2(hdev)) + return switch2_event(hdev, report, raw_data, size); + else + return joycon_event(hdev, report, raw_data, size); +} + +static int nintendo_hid_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + if (nintendo_is_switch2(hdev)) + return switch2_probe(hdev, id); + else + return joycon_probe(hdev, id); +} + +static int nintendo_hid_resume(struct hid_device *hdev) +{ + if (nintendo_is_switch2(hdev)) + return 0; + else + return joycon_resume(hdev); +} + +static int nintendo_hid_suspend(struct hid_device *hdev, pm_message_t message) +{ + if (nintendo_is_switch2(hdev)) + return 0; + else + return joycon_suspend(hdev, message); +} + static struct hid_driver nintendo_hid_driver = { .name = "nintendo", .id_table = nintendo_hid_devices, @@ -2844,4 +4022,5 @@ MODULE_LICENSE("GPL"); MODULE_AUTHOR("Ryan McClelland <rymcclel@gmail.com>"); MODULE_AUTHOR("Emily Strickland <linux@emily.st>"); MODULE_AUTHOR("Daniel J. Ogorchock <djogorchock@gmail.com>"); +MODULE_AUTHOR("Vicki Pfau <vi@endrift.com>"); MODULE_DESCRIPTION("Driver for Nintendo Switch Controllers"); diff --git a/drivers/hid/hid-nintendo.h b/drivers/hid/hid-nintendo.h new file mode 100644 index 000000000000..7aff22f30266 --- /dev/null +++ b/drivers/hid/hid-nintendo.h @@ -0,0 +1,72 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * HID driver for Nintendo Switch 2 controllers + * + * Copyright (c) 2025 Valve Software + * + * This driver is based on the following work: + * https://gist.github.com/shinyquagsire23/66f006b46c56216acbaac6c1e2279b64 + * https://github.com/ndeadly/switch2_controller_research + */ + +#ifndef __HID_NINTENDO_H +#define __HID_NINTENDO_H + +#include <linux/bits.h> + +#define NS2_FLAG_OK BIT(0) +#define NS2_FLAG_NACK BIT(2) + +enum switch2_cmd { + NS2_CMD_NFC = 0x01, + NS2_CMD_FLASH = 0x02, + NS2_CMD_INIT = 0x03, + NS2_CMD_GRIP = 0x08, + NS2_CMD_LED = 0x09, + NS2_CMD_VIBRATE = 0x0a, + NS2_CMD_BATTERY = 0x0b, + NS2_CMD_FEATSEL = 0x0c, + NS2_CMD_FW_UPD = 0x0d, + NS2_CMD_FW_INFO = 0x10, + NS2_CMD_BT_PAIR = 0x15, +}; + +enum switch2_direction { + NS2_DIR_IN = 0x00, + NS2_DIR_OUT = 0x90, +}; + +enum switch2_transport { + NS2_TRANS_USB = 0x00, + NS2_TRANS_BT = 0x01, +}; + +struct switch2_cmd_header { + uint8_t command; + uint8_t flags; + uint8_t transport; + uint8_t subcommand; + uint8_t unk1; + uint8_t length; + uint16_t unk2; +}; +static_assert(sizeof(struct switch2_cmd_header) == 8); + +struct device; +struct switch2_controller; +struct switch2_cfg_intf { + struct switch2_controller *parent; + struct device *dev; + + int (*send_command)(enum switch2_cmd command, uint8_t subcommand, + const void *message, size_t length, + struct switch2_cfg_intf *intf); +}; + +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg); +void switch2_controller_detach_cfg(struct switch2_controller *controller); + +int switch2_receive_command(struct switch2_controller *controller, + const uint8_t *message, size_t length); + +#endif diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig index 7755e5b454d2..868262c6ccd9 100644 --- a/drivers/input/joystick/Kconfig +++ b/drivers/input/joystick/Kconfig @@ -422,4 +422,15 @@ config JOYSTICK_SEESAW To compile this driver as a module, choose M here: the module will be called adafruit-seesaw. +config JOYSTICK_NINTENDO_SWITCH2_USB + tristate "Wired Nintendo Switch 2 controller support" + depends on HID_NINTENDO + depends on USB + help + Say Y here if you want to enable support for wired Nintendo Switch 2 + controllers. + + To compile this driver as a module, choose M here: the + module will be called nintendo-switch2-usb. + endif diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile index 9976f596a920..8f92900ae885 100644 --- a/drivers/input/joystick/Makefile +++ b/drivers/input/joystick/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o obj-$(CONFIG_JOYSTICK_STINGER) += stinger.o +obj-$(CONFIG_JOYSTICK_NINTENDO_SWITCH2_USB) += nintendo-switch2-usb.o obj-$(CONFIG_JOYSTICK_TMDC) += tmdc.o obj-$(CONFIG_JOYSTICK_TURBOGRAFX) += turbografx.o obj-$(CONFIG_JOYSTICK_TWIDJOY) += twidjoy.o diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c new file mode 100644 index 000000000000..ebd89d852e21 --- /dev/null +++ b/drivers/input/joystick/nintendo-switch2-usb.c @@ -0,0 +1,353 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * USB driver for Nintendo Switch 2 controllers configuration interface + * + * Copyright (c) 2025 Valve Software + * + * This driver is based on the following work: + * https://gist.github.com/shinyquagsire23/66f006b46c56216acbaac6c1e2279b64 + * https://github.com/ndeadly/switch2_controller_research + */ + +#include "../../hid/hid-ids.h" +#include "../../hid/hid-nintendo.h" +#include <linux/module.h> +#include <linux/usb/input.h> + +#define NS2_BULK_SIZE 64 +#define NS2_IN_URBS 2 +#define NS2_OUT_URBS 4 + +static struct usb_driver switch2_usb; + +struct switch2_urb { + struct urb *urb; + uint8_t *data; + bool active; +}; + +struct switch2_usb { + struct switch2_cfg_intf cfg; + struct usb_device *udev; + + struct switch2_urb bulk_in[NS2_IN_URBS]; + struct usb_anchor bulk_in_anchor; + spinlock_t bulk_in_lock; + + struct switch2_urb bulk_out[NS2_OUT_URBS]; + struct usb_anchor bulk_out_anchor; + spinlock_t bulk_out_lock; + + int message_in; + struct work_struct message_in_work; +}; + +static void switch2_bulk_in(struct urb *urb) +{ + struct switch2_usb *ns2_usb = urb->context; + int i; + bool schedule = false; + unsigned long flags; + + switch (urb->status) { + case 0: + schedule = true; + break; + case -ECONNRESET: + case -ENOENT: + case -ESHUTDOWN: + dev_dbg(&ns2_usb->udev->dev, "shutting down input urb: %d\n", urb->status); + return; + default: + dev_dbg(&ns2_usb->udev->dev, "unknown input urb status: %d\n", urb->status); + break; + } + + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); + for (i = 0; i < NS2_IN_URBS; i++) { + int err; + struct switch2_urb *ns2_urb; + + if (ns2_usb->bulk_in[i].urb == urb) { + ns2_usb->message_in = i; + continue; + } + + if (ns2_usb->bulk_in[i].active) + continue; + + ns2_urb = &ns2_usb->bulk_in[i]; + usb_anchor_urb(ns2_urb->urb, &ns2_usb->bulk_in_anchor); + err = usb_submit_urb(ns2_urb->urb, GFP_ATOMIC); + if (err) { + usb_unanchor_urb(ns2_urb->urb); + dev_dbg(&ns2_usb->udev->dev, "failed to queue input urb: %d\n", err); + } else { + ns2_urb->active = true; + } + } + spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags); + + if (schedule) + schedule_work(&ns2_usb->message_in_work); +} + +static void switch2_bulk_out(struct urb *urb) +{ + struct switch2_usb *ns2_usb = urb->context; + int i; + + guard(spinlock_irqsave)(&ns2_usb->bulk_out_lock); + + switch (urb->status) { + case 0: + break; + case -ECONNRESET: + case -ENOENT: + case -ESHUTDOWN: + dev_dbg(&ns2_usb->udev->dev, "shutting down output urb: %d\n", urb->status); + return; + default: + dev_dbg(&ns2_usb->udev->dev, "unknown output urb status: %d\n", urb->status); + return; + } + + for (i = 0; i < NS2_OUT_URBS; i++) { + if (ns2_usb->bulk_out[i].urb != urb) + continue; + + ns2_usb->bulk_out[i].active = false; + break; + } +} + +static int switch2_usb_send_cmd(enum switch2_cmd command, uint8_t subcommand, + const void *message, size_t size, struct switch2_cfg_intf *cfg) +{ + struct switch2_usb *ns2_usb = (struct switch2_usb *)cfg; + struct switch2_urb *urb = NULL; + int i; + int ret; + unsigned long flags; + + struct switch2_cmd_header header = { + command, NS2_DIR_OUT | NS2_FLAG_OK, NS2_TRANS_USB, subcommand, 0, size + }; + + if (WARN_ON(size > 56)) + return -EINVAL; + + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags); + for (i = 0; i < NS2_OUT_URBS; i++) { + if (ns2_usb->bulk_out[i].active) + continue; + + urb = &ns2_usb->bulk_out[i]; + urb->active = true; + break; + } + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags); + + if (!urb) { + dev_warn(&ns2_usb->udev->dev, "output queue full, dropping message\n"); + return -ENOBUFS; + } + + memcpy(urb->data, &header, sizeof(header)); + if (message && size) + memcpy(&urb->data[8], message, size); + urb->urb->transfer_buffer_length = size + sizeof(header); + + print_hex_dump_debug("sending cmd: ", DUMP_PREFIX_OFFSET, 16, 1, urb->data, + size + sizeof(header), false); + + usb_anchor_urb(urb->urb, &ns2_usb->bulk_out_anchor); + ret = usb_submit_urb(urb->urb, GFP_ATOMIC); + if (ret) { + if (ret != -ENODEV) + dev_warn(&ns2_usb->udev->dev, "failed to submit output urb: %i", ret); + urb->active = false; + usb_unanchor_urb(urb->urb); + return ret; + } + + return 0; +} + +static void switch2_usb_message_in_work(struct work_struct *work) +{ + struct switch2_usb *ns2_usb = container_of(work, struct switch2_usb, message_in_work); + struct switch2_urb *urb; + int err; + unsigned long flags; + + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); + urb = &ns2_usb->bulk_in[ns2_usb->message_in]; + spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags); + + err = switch2_receive_command(ns2_usb->cfg.parent, urb->urb->transfer_buffer, + urb->urb->actual_length); + if (err) + dev_dbg(&ns2_usb->udev->dev, "receive command failed: %d\n", err); + + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); + urb->active = false; + spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags); +} + +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) +{ + struct switch2_usb *ns2_usb; + struct usb_device *udev; + struct usb_endpoint_descriptor *bulk_in, *bulk_out; + char phys[64]; + int ret; + int i; + + udev = interface_to_usbdev(intf); + if (usb_make_path(udev, phys, sizeof(phys)) < 0) + return -EINVAL; + + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL); + if (ret) { + dev_err(&intf->dev, "failed to find bulk EPs\n"); + return ret; + } + + ns2_usb = devm_kzalloc(&intf->dev, sizeof(*ns2_usb), GFP_KERNEL); + if (!ns2_usb) + return -ENOMEM; + + ns2_usb->udev = udev; + for (i = 0; i < NS2_IN_URBS; i++) { + ns2_usb->bulk_in[i].urb = usb_alloc_urb(0, GFP_KERNEL); + if (!ns2_usb->bulk_in[i].urb) { + ret = -ENOMEM; + goto err_free_in; + } + + ns2_usb->bulk_in[i].data = usb_alloc_coherent(udev, NS2_BULK_SIZE, GFP_KERNEL, + &ns2_usb->bulk_in[i].urb->transfer_dma); + if (!ns2_usb->bulk_in[i].data) { + ret = -ENOMEM; + goto err_free_in; + } + + usb_fill_bulk_urb(ns2_usb->bulk_in[i].urb, udev, + usb_rcvbulkpipe(udev, bulk_in->bEndpointAddress), + ns2_usb->bulk_in[i].data, NS2_BULK_SIZE, switch2_bulk_in, ns2_usb); + ns2_usb->bulk_in[i].urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + } + + for (i = 0; i < NS2_OUT_URBS; i++) { + ns2_usb->bulk_out[i].urb = usb_alloc_urb(0, GFP_KERNEL); + if (!ns2_usb->bulk_out[i].urb) { + ret = -ENOMEM; + goto err_free_out; + } + + ns2_usb->bulk_out[i].data = usb_alloc_coherent(udev, NS2_BULK_SIZE, GFP_KERNEL, + &ns2_usb->bulk_out[i].urb->transfer_dma); + if (!ns2_usb->bulk_out[i].data) { + ret = -ENOMEM; + goto err_free_out; + } + + usb_fill_bulk_urb(ns2_usb->bulk_out[i].urb, udev, + usb_sndbulkpipe(udev, bulk_out->bEndpointAddress), + ns2_usb->bulk_out[i].data, NS2_BULK_SIZE, switch2_bulk_out, ns2_usb); + ns2_usb->bulk_out[i].urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + } + + ns2_usb->bulk_in[0].active = true; + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC); + if (ret < 0) + goto err_free_out; + + init_usb_anchor(&ns2_usb->bulk_out_anchor); + spin_lock_init(&ns2_usb->bulk_out_lock); + init_usb_anchor(&ns2_usb->bulk_in_anchor); + spin_lock_init(&ns2_usb->bulk_in_lock); + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work); + + usb_set_intfdata(intf, ns2_usb); + + ns2_usb->cfg.dev = &ns2_usb->udev->dev; + ns2_usb->cfg.send_command = switch2_usb_send_cmd; + + ret = switch2_controller_attach_cfg(phys, &ns2_usb->cfg); + if (ret < 0) + goto err_kill_urb; + + return 0; + +err_kill_urb: + usb_kill_urb(ns2_usb->bulk_in[0].urb); +err_free_out: + for (i = 0; i < NS2_OUT_URBS; i++) { + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, + ns2_usb->bulk_out[i].urb->transfer_dma); + usb_free_urb(ns2_usb->bulk_out[i].urb); + } +err_free_in: + for (i = 0; i < NS2_IN_URBS; i++) { + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data, + ns2_usb->bulk_in[i].urb->transfer_dma); + usb_free_urb(ns2_usb->bulk_in[i].urb); + } + devm_kfree(&intf->dev, ns2_usb); + + return ret; +} + +static void switch2_usb_disconnect(struct usb_interface *intf) +{ + struct switch2_usb *ns2_usb = usb_get_intfdata(intf); + unsigned long flags; + int i; + + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags); + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor); + for (i = 0; i < NS2_OUT_URBS; i++) { + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, + ns2_usb->bulk_out[i].urb->transfer_dma); + usb_free_urb(ns2_usb->bulk_out[i].urb); + } + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags); + + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor); + cancel_work_sync(&ns2_usb->message_in_work); + for (i = 0; i < NS2_IN_URBS; i++) { + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data, + ns2_usb->bulk_in[i].urb->transfer_dma); + usb_free_urb(ns2_usb->bulk_in[i].urb); + } + spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags); + + switch2_controller_detach_cfg(ns2_usb->cfg.parent); +} + +#define SWITCH2_CONTROLLER(vend, prod) \ + USB_DEVICE_AND_INTERFACE_INFO(vend, prod, USB_CLASS_VENDOR_SPEC, 0, 0) + +static const struct usb_device_id switch2_usb_devices[] = { + { SWITCH2_CONTROLLER(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_NS2_JOYCONL) }, + { SWITCH2_CONTROLLER(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_NS2_JOYCONR) }, + { SWITCH2_CONTROLLER(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_NS2_PROCON) }, + { SWITCH2_CONTROLLER(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_NS2_GCCON) }, + { } +}; +MODULE_DEVICE_TABLE(usb, switch2_usb_devices); + +static struct usb_driver switch2_usb = { + .name = "switch2", + .id_table = switch2_usb_devices, + .probe = switch2_usb_probe, + .disconnect = switch2_usb_disconnect, +}; +module_usb_driver(switch2_usb); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Vicki Pfau <vi@endrift.com>"); +MODULE_DESCRIPTION("Driver for Nintendo Switch 2 Controllers"); -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver 2026-05-12 20:00 ` [PATCH v5 1/3] " Vicki Pfau @ 2026-05-14 0:15 ` sashiko-bot 2026-05-23 8:57 ` Silvan Jegen 2026-06-19 3:56 ` Vicki Pfau 2026-06-22 9:47 ` [PATCH] HID: nintendo: fix clang build of switch2_probe guard/goto Ryan McClelland 1 sibling, 2 replies; 15+ messages in thread From: sashiko-bot @ 2026-05-14 0:15 UTC (permalink / raw) To: Vicki Pfau; +Cc: linux-input, dmitry.torokhov Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider: - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect` - [High] Race condition in IN URB tracking causes permanent starvation - [High] Dangling pointers and use-after-free on probe failure - [High] Memory corruption and double free due to incorrect devm input lifecycle management - [High] NULL pointer dereference in USB probe error path - [High] Division by zero crash from unvalidated trigger calibration data - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response - [High] Race condition due to URB submission before initialization of synchronization primitives - [Medium] Infinite USB retry storm on protocol errors - [Medium] Unaligned memory access on byte array casts -- commit 00cb7265ad5983144c6923702e2ee2c3c047d29e Author: Vicki Pfau <vi@endrift.com> Subject: HID: nintendo: Add preliminary Switch 2 controller driver This adds a new driver for the Switch 2 controllers. The Switch 2 uses an unusual split-interface design such that input and rumble occur on the main HID interface, but all other communication occurs over a "configuration" interface. > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 29008c2cc5304..2193afcdefabc 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c [ ... ] > +static int switch2_init_input(struct switch2_controller *ns2) > +{ [ ... ] > + input = devm_input_allocate_device(&hdev->dev); > + if (!input) > + return -ENOMEM; [ ... ] > + ret = input_register_device(input); > + if (ret < 0) { > + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret); > + input_free_device(input); Calling input_free_device or input_unregister_device on a devm-managed input device can cause double frees when devres cleanup fires. Can this lead to memory corruption? > + return ret; > + } [ ... ] > +static void switch2_controller_put(struct switch2_controller *ns2) > +{ > + struct input_dev *input; > + bool do_free; [ ... ] > + if (input) > + input_unregister_device(input); Calling input_unregister_device on a devm-managed input device can cause issues. If the input device remains alive (tied to the HID device devres) and its child LEDs are not released, will reconnecting the USB cable allocate a new input device and corrupt the LED lists by re-registering the exact same ns2->leds structures? > + > + if (do_free) { [ ... ] > +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data) > +{ > + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); The zero variable is loaded directly from the controller's factory calibration flash. If a device reports a zero point of exactly 232, will this trigger a division by zero exception and crash the kernel? > + > + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE)); > +} [ ... ] > +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address, > + uint8_t size) > +{ > + uint8_t message[8] = { size, 0x7e }; > + > + if (!ns2->cfg) > + return -ENOTCONN; > + *(__le32 *)&message[4] = __cpu_to_le32(address); The driver directly casts &message[4] to __le32. Since message is a uint8_t array, it is only guaranteed to be 1-byte aligned. On strict architectures, will this unaligned memory write trigger a hardware alignment fault? > + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message, > + sizeof(message), ns2->cfg); > +} [ ... ] > +int switch2_receive_command(struct switch2_controller *ns2, > + const uint8_t *message, size_t length) > +{ [ ... ] > + header = (const struct switch2_cmd_header *)message; > + if (!(header->flags & NS2_FLAG_OK)) { > + ret = -EIO; > + goto exit; > + } > + message = &message[8]; > + switch (header->command) { [ ... ] > + case NS2_CMD_FW_INFO: > + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) { > + if (length < sizeof(ns2->version)) { > + ret = -EINVAL; > + goto exit; > + } > + memcpy(&ns2->version, message, sizeof(ns2->version)); The message pointer is advanced by 8 bytes to skip the header, but length is not decremented. The bounds check compares length against sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check passes, but the advanced message pointer only has length - 8 valid bytes remaining. Will the memcpy over-read into uninitialized memory? > + ns2->ctlr_type = ns2->version.ctlr_type; > + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO); > + } > + break; > + default: > + break; > + } > + > +exit: > + if (ns2->init_step < NS2_INIT_DONE) > + switch2_init_controller(ns2); If a command response has the NACK flag set or encounters another error, the code jumps to exit and invokes switch2_init_controller. Since ns2->init_step has not advanced, the state machine will immediately re-send the exact same command. Can this cause an infinite tight feedback loop of failures and retries, flooding the USB bus? > + > + return ret; > +} [ ... ] > +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg) > +{ [ ... ] > + guard(mutex)(&ns2->lock); > + WARN_ON(ns2->cfg); > + ns2->cfg = cfg; > + > + if (ns2->hdev) > + return switch2_init_controller(ns2); If switch2_init_controller fails here or in switch2_probe, the error is returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev remain set, potentially leading to a use-after-free when subsequent device events occur? > + return 0; > +} [ ... ] > +static void switch2_remove(struct hid_device *hdev) > +{ > + struct switch2_controller *ns2 = hid_get_drvdata(hdev); > + > + hid_hw_close(hdev); > + mutex_lock(&ns2->lock); > + WARN_ON(ns2->hdev != hdev); > + ns2->hdev = NULL; > + mutex_unlock(&ns2->lock); > + ida_free(&nintendo_player_id_allocator, ns2->player_id); If ida_alloc fails during probe, ns2->player_id remains at its default value of U32_MAX. When ida_free is called, the IDA framework enforces a check on the ID. Casting U32_MAX to a signed integer yields -1. Will this trigger the BUG_ON and cause a kernel panic? > + switch2_controller_put(ns2); > + hid_hw_stop(hdev); > +} [ ... ] > diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c > new file mode 100644 > index 0000000000000..ebd89d852e21a > --- /dev/null > +++ b/drivers/input/joystick/nintendo-switch2-usb.c [ ... ] > +static void switch2_bulk_in(struct urb *urb) > +{ [ ... ] > + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); > + for (i = 0; i < NS2_IN_URBS; i++) { > + int err; > + struct switch2_urb *ns2_urb; > + > + if (ns2_usb->bulk_in[i].urb == urb) { > + ns2_usb->message_in = i; If multiple URBs complete in quick succession before the work item executes, the message_in index is overwritten. Will this cause the work item to only process the last completed URB, leaving earlier URBs indefinitely active and leading to starvation of the URB pool? > + continue; > + } [ ... ] > +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) > +{ [ ... ] > + ns2_usb->bulk_in[0].active = true; > + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC); > + if (ret < 0) > + goto err_free_out; > + > + init_usb_anchor(&ns2_usb->bulk_out_anchor); > + spin_lock_init(&ns2_usb->bulk_out_lock); > + init_usb_anchor(&ns2_usb->bulk_in_anchor); > + spin_lock_init(&ns2_usb->bulk_in_lock); > + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work); usb_submit_urb is invoked for bulk_in[0] before the initialization of the spinlocks, anchors, and work struct. If the URB completes immediately, the completion handler will execute and attempt to use uninitialized synchronization primitives. Can this cause a crash? > + > + usb_set_intfdata(intf, ns2_usb); [ ... ] > +err_kill_urb: > + usb_kill_urb(ns2_usb->bulk_in[0].urb); > +err_free_out: > + for (i = 0; i < NS2_OUT_URBS; i++) { > + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, > + ns2_usb->bulk_out[i].urb->transfer_dma); > + usb_free_urb(ns2_usb->bulk_out[i].urb); > + } > +err_free_in: > + for (i = 0; i < NS2_IN_URBS; i++) { > + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data, > + ns2_usb->bulk_in[i].urb->transfer_dma); If usb_alloc_urb fails during the loop, it jumps to err_free_in. During cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is NULL for the failed index, will this trigger a NULL pointer dereference? > + usb_free_urb(ns2_usb->bulk_in[i].urb); > + } > + devm_kfree(&intf->dev, ns2_usb); > + > + return ret; > +} > + > +static void switch2_usb_disconnect(struct usb_interface *intf) > +{ > + struct switch2_usb *ns2_usb = usb_get_intfdata(intf); > + unsigned long flags; > + int i; > + > + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags); > + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor); Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to call them while holding a spinlock? > + for (i = 0; i < NS2_OUT_URBS; i++) { > + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, > + ns2_usb->bulk_out[i].urb->transfer_dma); > + usb_free_urb(ns2_usb->bulk_out[i].urb); > + } > + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags); > + > + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); > + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor); > + cancel_work_sync(&ns2_usb->message_in_work); Furthermore, cancel_work_sync waits for message_in_work which attempts to acquire bulk_in_lock. Does this cause a deadlock since the lock is already held by the disconnecting thread? > + for (i = 0; i < NS2_IN_URBS; i++) { -- Sashiko AI review · https://sashiko.dev/#/patchset/20260512200051.2534081-1-vi@endrift.com?part=1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver 2026-05-14 0:15 ` sashiko-bot @ 2026-05-23 8:57 ` Silvan Jegen 2026-06-19 4:02 ` Vicki Pfau 2026-06-19 3:56 ` Vicki Pfau 1 sibling, 1 reply; 15+ messages in thread From: Silvan Jegen @ 2026-05-23 8:57 UTC (permalink / raw) To: sashiko-reviews; +Cc: Vicki Pfau, linux-input, dmitry.torokhov Heyhey! Below some comments regarding the findings (note that I am not an expert on neither Linux kernel drivers in general nor USB HID specifically). sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider: > - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect` > - [High] Race condition in IN URB tracking causes permanent starvation > - [High] Dangling pointers and use-after-free on probe failure > - [High] Memory corruption and double free due to incorrect devm input lifecycle management > - [High] NULL pointer dereference in USB probe error path > - [High] Division by zero crash from unvalidated trigger calibration data > - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX > - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response > - [High] Race condition due to URB submission before initialization of synchronization primitives > - [Medium] Infinite USB retry storm on protocol errors > - [Medium] Unaligned memory access on byte array casts > -- > > commit 00cb7265ad5983144c6923702e2ee2c3c047d29e > Author: Vicki Pfau <vi@endrift.com> > Subject: HID: nintendo: Add preliminary Switch 2 controller driver > > This adds a new driver for the Switch 2 controllers. The Switch 2 uses an > unusual split-interface design such that input and rumble occur on the main > HID interface, but all other communication occurs over a "configuration" > interface. > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > index 29008c2cc5304..2193afcdefabc 100644 > > --- a/drivers/hid/hid-nintendo.c > > +++ b/drivers/hid/hid-nintendo.c > [ ... ] > > +static int switch2_init_input(struct switch2_controller *ns2) > > +{ > [ ... ] > > + input = devm_input_allocate_device(&hdev->dev); > > + if (!input) > > + return -ENOMEM; > [ ... ] > > + ret = input_register_device(input); > > + if (ret < 0) { > > + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret); > > + input_free_device(input); > > Calling input_free_device or input_unregister_device on a devm-managed > input device can cause double frees when devres cleanup fires. Can this > lead to memory corruption? I think sashiko is actually correct here, unfortunately. The documentation for devm_input_allocate_device states that Managed input devices do not need to be explicitly unregistered or freed as it will be done automatically when owner device unbinds from its driver (or binding fails). ... Looks like we can remove all the input_unregister_device and input_free_device calls here. > > > + return ret; > > + } > [ ... ] > > +static void switch2_controller_put(struct switch2_controller *ns2) > > +{ > > + struct input_dev *input; > > + bool do_free; > [ ... ] > > + if (input) > > + input_unregister_device(input); > > Calling input_unregister_device on a devm-managed input device can cause > issues. If the input device remains alive (tied to the HID device devres) > and its child LEDs are not released, will reconnecting the USB cable > allocate a new input device and corrupt the LED lists by re-registering > the exact same ns2->leds structures? This input_unregister_device can also be removed. > > > + > > + if (do_free) { > [ ... ] > > +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data) > > +{ > > + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); > > The zero variable is loaded directly from the controller's factory calibration > flash. If a device reports a zero point of exactly 232, will this trigger a > division by zero exception and crash the kernel? Not sure this is an issue in practice, but we might want to protect the code in any case by doing something like the below perhaps (untested). int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / max_t(int, (232 - zero), 1); > > > + > > + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE)); > > +} > [ ... ] > > +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address, > > + uint8_t size) > > +{ > > + uint8_t message[8] = { size, 0x7e }; > > + > > + if (!ns2->cfg) > > + return -ENOTCONN; > > + *(__le32 *)&message[4] = __cpu_to_le32(address); > > The driver directly casts &message[4] to __le32. Since message is a uint8_t > array, it is only guaranteed to be 1-byte aligned. On strict architectures, > will this unaligned memory write trigger a hardware alignment fault? I assume this could be an issue and we should just use something like the below (only compile-tested). put_unaligned_le32(__cpu_to_le32(address), &message[4]); > > > + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message, > > + sizeof(message), ns2->cfg); > > +} > [ ... ] > > +int switch2_receive_command(struct switch2_controller *ns2, > > + const uint8_t *message, size_t length) > > +{ > [ ... ] > > + header = (const struct switch2_cmd_header *)message; > > + if (!(header->flags & NS2_FLAG_OK)) { > > + ret = -EIO; > > + goto exit; > > + } > > + message = &message[8]; > > + switch (header->command) { > [ ... ] > > + case NS2_CMD_FW_INFO: > > + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) { > > + if (length < sizeof(ns2->version)) { > > + ret = -EINVAL; > > + goto exit; > > + } > > + memcpy(&ns2->version, message, sizeof(ns2->version)); > > The message pointer is advanced by 8 bytes to skip the header, but length > is not decremented. The bounds check compares length against > sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check > passes, but the advanced message pointer only has length - 8 valid bytes > remaining. Will the memcpy over-read into uninitialized memory? Sounds like something that we should fix by either decreasing length or increasing the size in the check. > > > + ns2->ctlr_type = ns2->version.ctlr_type; > > + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO); > > + } > > + break; > > + default: > > + break; > > + } > > + > > +exit: > > + if (ns2->init_step < NS2_INIT_DONE) > > + switch2_init_controller(ns2); > > If a command response has the NACK flag set or encounters another error, > the code jumps to exit and invokes switch2_init_controller. Since > ns2->init_step has not advanced, the state machine will immediately re-send > the exact same command. Can this cause an infinite tight feedback loop of > failures and retries, flooding the USB bus? We have discussed this before and it doesn't seem to be an issue in practice. If we want to be on the safe side, we could pass a retry count and abort on too many error retries, perhaps? > > + > > + return ret; > > +} > [ ... ] > > +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg) > > +{ > [ ... ] > > + guard(mutex)(&ns2->lock); > > + WARN_ON(ns2->cfg); > > + ns2->cfg = cfg; > > + > > + if (ns2->hdev) > > + return switch2_init_controller(ns2); > > If switch2_init_controller fails here or in switch2_probe, the error is > returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev > remain set, potentially leading to a use-after-free when subsequent device > events occur? AFAICT, since we are always checking for both ns2->cfg and ns2->hdev before doing any processing in switch2_init_controller, there should never be a use-after-free It *is* true that we don't call hid_hw_{close,stop} in switch2_probe, if the switch2_init_controller call fails though. It might make sense to return a specific error and use it to check whether we want to give up and call these? > > > + return 0; > > +} > [ ... ] > > +static void switch2_remove(struct hid_device *hdev) > > +{ > > + struct switch2_controller *ns2 = hid_get_drvdata(hdev); > > + > > + hid_hw_close(hdev); > > + mutex_lock(&ns2->lock); > > + WARN_ON(ns2->hdev != hdev); > > + ns2->hdev = NULL; > > + mutex_unlock(&ns2->lock); > > + ida_free(&nintendo_player_id_allocator, ns2->player_id); > > If ida_alloc fails during probe, ns2->player_id remains at its default > value of U32_MAX. When ida_free is called, the IDA framework enforces a > check on the ID. Casting U32_MAX to a signed integer yields -1. Will this > trigger the BUG_ON and cause a kernel panic? ida_free seems to return early on if ((int)id < 0) return; so that should be fine as is. > > > + switch2_controller_put(ns2); > > + hid_hw_stop(hdev); > > +} > [ ... ] > > diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c > > new file mode 100644 > > index 0000000000000..ebd89d852e21a > > --- /dev/null > > +++ b/drivers/input/joystick/nintendo-switch2-usb.c > [ ... ] > > +static void switch2_bulk_in(struct urb *urb) > > +{ > [ ... ] > > + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); > > + for (i = 0; i < NS2_IN_URBS; i++) { > > + int err; > > + struct switch2_urb *ns2_urb; > > + > > + if (ns2_usb->bulk_in[i].urb == urb) { > > + ns2_usb->message_in = i; > > If multiple URBs complete in quick succession before the work item executes, > the message_in index is overwritten. Will this cause the work item to only > process the last completed URB, leaving earlier URBs indefinitely active > and leading to starvation of the URB pool? > > > + continue; > > + } > [ ... ] > > +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) > > +{ > [ ... ] > > + ns2_usb->bulk_in[0].active = true; > > + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC); > > + if (ret < 0) > > + goto err_free_out; > > + > > + init_usb_anchor(&ns2_usb->bulk_out_anchor); > > + spin_lock_init(&ns2_usb->bulk_out_lock); > > + init_usb_anchor(&ns2_usb->bulk_in_anchor); > > + spin_lock_init(&ns2_usb->bulk_in_lock); > > + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work); > > usb_submit_urb is invoked for bulk_in[0] before the initialization of the > spinlocks, anchors, and work struct. If the URB completes immediately, the > completion handler will execute and attempt to use uninitialized > synchronization primitives. Can this cause a crash? As the documentation for usb_submit_urb mentions that the call is async, I assume that this is indeed a theoretical possibility. > > > + > > + usb_set_intfdata(intf, ns2_usb); > [ ... ] > > +err_kill_urb: > > + usb_kill_urb(ns2_usb->bulk_in[0].urb); > > +err_free_out: > > + for (i = 0; i < NS2_OUT_URBS; i++) { > > + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, > > + ns2_usb->bulk_out[i].urb->transfer_dma); > > + usb_free_urb(ns2_usb->bulk_out[i].urb); > > + } > > +err_free_in: > > + for (i = 0; i < NS2_IN_URBS; i++) { > > + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data, > > + ns2_usb->bulk_in[i].urb->transfer_dma); > > If usb_alloc_urb fails during the loop, it jumps to err_free_in. During > cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is > NULL for the failed index, will this trigger a NULL pointer dereference? Yes, most likely we would need a new label for this case. > > > + usb_free_urb(ns2_usb->bulk_in[i].urb); > > + } > > + devm_kfree(&intf->dev, ns2_usb); > > + > > + return ret; > > +} > > + > > +static void switch2_usb_disconnect(struct usb_interface *intf) > > +{ > > + struct switch2_usb *ns2_usb = usb_get_intfdata(intf); > > + unsigned long flags; > > + int i; > > + > > + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags); > > + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor); > > Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to > call them while holding a spinlock? According to some googling, this indeed seems to be an isuse. The non-sleeping way to cancel transfer requests would be to use https://www.kernel.org/doc/html/v6.1/driver-api/usb/usb.html#c.usb_unlink_anchored_urbs > > > + for (i = 0; i < NS2_OUT_URBS; i++) { > > + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, > > + ns2_usb->bulk_out[i].urb->transfer_dma); > > + usb_free_urb(ns2_usb->bulk_out[i].urb); > > + } > > + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags); > > + > > + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); > > + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor); > > + cancel_work_sync(&ns2_usb->message_in_work); > > Furthermore, cancel_work_sync waits for message_in_work which attempts to > acquire bulk_in_lock. Does this cause a deadlock since the lock is already > held by the disconnecting thread? Yes, this looks like a real concern as well. Cheers, Silvan > > > + for (i = 0; i < NS2_IN_URBS; i++) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver 2026-05-23 8:57 ` Silvan Jegen @ 2026-06-19 4:02 ` Vicki Pfau 0 siblings, 0 replies; 15+ messages in thread From: Vicki Pfau @ 2026-06-19 4:02 UTC (permalink / raw) To: Silvan Jegen, sashiko-reviews; +Cc: linux-input, dmitry.torokhov On 5/23/26 1:57 AM, Silvan Jegen wrote: > Heyhey! > > Below some comments regarding the findings (note that I am not an expert > on neither Linux kernel drivers in general nor USB HID specifically). > > sashiko-bot@kernel.org wrote: >> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider: >> - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect` >> - [High] Race condition in IN URB tracking causes permanent starvation >> - [High] Dangling pointers and use-after-free on probe failure >> - [High] Memory corruption and double free due to incorrect devm input lifecycle management >> - [High] NULL pointer dereference in USB probe error path >> - [High] Division by zero crash from unvalidated trigger calibration data >> - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX >> - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response >> - [High] Race condition due to URB submission before initialization of synchronization primitives >> - [Medium] Infinite USB retry storm on protocol errors >> - [Medium] Unaligned memory access on byte array casts >> -- >> >> commit 00cb7265ad5983144c6923702e2ee2c3c047d29e >> Author: Vicki Pfau <vi@endrift.com> >> Subject: HID: nintendo: Add preliminary Switch 2 controller driver >> >> This adds a new driver for the Switch 2 controllers. The Switch 2 uses an >> unusual split-interface design such that input and rumble occur on the main >> HID interface, but all other communication occurs over a "configuration" >> interface. >> >>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c >>> index 29008c2cc5304..2193afcdefabc 100644 >>> --- a/drivers/hid/hid-nintendo.c >>> +++ b/drivers/hid/hid-nintendo.c >> [ ... ] >>> +static int switch2_init_input(struct switch2_controller *ns2) >>> +{ >> [ ... ] >>> + input = devm_input_allocate_device(&hdev->dev); >>> + if (!input) >>> + return -ENOMEM; >> [ ... ] >>> + ret = input_register_device(input); >>> + if (ret < 0) { >>> + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret); >>> + input_free_device(input); >> >> Calling input_free_device or input_unregister_device on a devm-managed >> input device can cause double frees when devres cleanup fires. Can this >> lead to memory corruption? > > I think sashiko is actually correct here, unfortunately. The documentation > for devm_input_allocate_device states that > > Managed input devices do not need to be explicitly unregistered or > freed as it will be done automatically when owner device unbinds from > its driver (or binding fails). ... > > Looks like we can remove all the input_unregister_device and > input_free_device calls here. sashiko is not correct. See my previous reply. > >> >>> + return ret; >>> + } >> [ ... ] >>> +static void switch2_controller_put(struct switch2_controller *ns2) >>> +{ >>> + struct input_dev *input; >>> + bool do_free; >> [ ... ] >>> + if (input) >>> + input_unregister_device(input); >> >> Calling input_unregister_device on a devm-managed input device can cause >> issues. If the input device remains alive (tied to the HID device devres) >> and its child LEDs are not released, will reconnecting the USB cable >> allocate a new input device and corrupt the LED lists by re-registering >> the exact same ns2->leds structures? > > This input_unregister_device can also be removed. > >> >>> + >>> + if (do_free) { >> [ ... ] >>> +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data) >>> +{ >>> + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); >> >> The zero variable is loaded directly from the controller's factory calibration >> flash. If a device reports a zero point of exactly 232, will this trigger a >> division by zero exception and crash the kernel? > > Not sure this is an issue in practice, but we might want to protect the > code in any case by doing something like the below perhaps (untested). > > int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / max_t(int, (232 - zero), 1); > > It won't be an issue in practice but it's harmless to fix. >> >>> + >>> + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE)); >>> +} >> [ ... ] >>> +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address, >>> + uint8_t size) >>> +{ >>> + uint8_t message[8] = { size, 0x7e }; >>> + >>> + if (!ns2->cfg) >>> + return -ENOTCONN; >>> + *(__le32 *)&message[4] = __cpu_to_le32(address); >> >> The driver directly casts &message[4] to __le32. Since message is a uint8_t >> array, it is only guaranteed to be 1-byte aligned. On strict architectures, >> will this unaligned memory write trigger a hardware alignment fault? > > I assume this could be an issue and we should just use something like > the below (only compile-tested). > > put_unaligned_le32(__cpu_to_le32(address), &message[4]); > Correct. > >> >>> + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message, >>> + sizeof(message), ns2->cfg); >>> +} >> [ ... ] >>> +int switch2_receive_command(struct switch2_controller *ns2, >>> + const uint8_t *message, size_t length) >>> +{ >> [ ... ] >>> + header = (const struct switch2_cmd_header *)message; >>> + if (!(header->flags & NS2_FLAG_OK)) { >>> + ret = -EIO; >>> + goto exit; >>> + } >>> + message = &message[8]; >>> + switch (header->command) { >> [ ... ] >>> + case NS2_CMD_FW_INFO: >>> + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) { >>> + if (length < sizeof(ns2->version)) { >>> + ret = -EINVAL; >>> + goto exit; >>> + } >>> + memcpy(&ns2->version, message, sizeof(ns2->version)); >> >> The message pointer is advanced by 8 bytes to skip the header, but length >> is not decremented. The bounds check compares length against >> sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check >> passes, but the advanced message pointer only has length - 8 valid bytes >> remaining. Will the memcpy over-read into uninitialized memory? > > Sounds like something that we should fix by either decreasing length or > increasing the size in the check. > >> >>> + ns2->ctlr_type = ns2->version.ctlr_type; >>> + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO); >>> + } >>> + break; >>> + default: >>> + break; >>> + } >>> + >>> +exit: >>> + if (ns2->init_step < NS2_INIT_DONE) >>> + switch2_init_controller(ns2); >> >> If a command response has the NACK flag set or encounters another error, >> the code jumps to exit and invokes switch2_init_controller. Since >> ns2->init_step has not advanced, the state machine will immediately re-send >> the exact same command. Can this cause an infinite tight feedback loop of >> failures and retries, flooding the USB bus? > > We have discussed this before and it doesn't seem to be an issue in > practice. > > If we want to be on the safe side, we could pass a retry count and abort > on too many error retries, perhaps? I had considered a retry count but getting it to work properly was fiddly. This should only go wrong if there's a malicious device attached, as in practice the Switch 2 controllers won't do this, and bus abuse is trivial with a malicious device regardless. > > >>> + >>> + return ret; >>> +} >> [ ... ] >>> +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg) >>> +{ >> [ ... ] >>> + guard(mutex)(&ns2->lock); >>> + WARN_ON(ns2->cfg); >>> + ns2->cfg = cfg; >>> + >>> + if (ns2->hdev) >>> + return switch2_init_controller(ns2); >> >> If switch2_init_controller fails here or in switch2_probe, the error is >> returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev >> remain set, potentially leading to a use-after-free when subsequent device >> events occur? > > AFAICT, since we are always checking for both ns2->cfg and ns2->hdev > before doing any processing in switch2_init_controller, there should > never be a use-after-free > > It *is* true that we don't call hid_hw_{close,stop} in switch2_probe, > if the switch2_init_controller call fails though. It might make sense > to return a specific error and use it to check whether we want to give > up and call these? The problem was that it will return an error after setting ns2->cfg, which causes the caller to free it without removing it. That's a real bug that can cause UAF. > >> >>> + return 0; >>> +} >> [ ... ] >>> +static void switch2_remove(struct hid_device *hdev) >>> +{ >>> + struct switch2_controller *ns2 = hid_get_drvdata(hdev); >>> + >>> + hid_hw_close(hdev); >>> + mutex_lock(&ns2->lock); >>> + WARN_ON(ns2->hdev != hdev); >>> + ns2->hdev = NULL; >>> + mutex_unlock(&ns2->lock); >>> + ida_free(&nintendo_player_id_allocator, ns2->player_id); >> >> If ida_alloc fails during probe, ns2->player_id remains at its default >> value of U32_MAX. When ida_free is called, the IDA framework enforces a >> check on the ID. Casting U32_MAX to a signed integer yields -1. Will this >> trigger the BUG_ON and cause a kernel panic? > > ida_free seems to return early on > > if ((int)id < 0) > return; > > so that should be fine as is. > >> >>> + switch2_controller_put(ns2); >>> + hid_hw_stop(hdev); >>> +} >> [ ... ] >>> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c >>> new file mode 100644 >>> index 0000000000000..ebd89d852e21a >>> --- /dev/null >>> +++ b/drivers/input/joystick/nintendo-switch2-usb.c >> [ ... ] >>> +static void switch2_bulk_in(struct urb *urb) >>> +{ >> [ ... ] >>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); >>> + for (i = 0; i < NS2_IN_URBS; i++) { >>> + int err; >>> + struct switch2_urb *ns2_urb; >>> + >>> + if (ns2_usb->bulk_in[i].urb == urb) { >>> + ns2_usb->message_in = i; >> >> If multiple URBs complete in quick succession before the work item executes, >> the message_in index is overwritten. Will this cause the work item to only >> process the last completed URB, leaving earlier URBs indefinitely active >> and leading to starvation of the URB pool? >> >>> + continue; >>> + } >> [ ... ] >>> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> +{ >> [ ... ] >>> + ns2_usb->bulk_in[0].active = true; >>> + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC); >>> + if (ret < 0) >>> + goto err_free_out; >>> + >>> + init_usb_anchor(&ns2_usb->bulk_out_anchor); >>> + spin_lock_init(&ns2_usb->bulk_out_lock); >>> + init_usb_anchor(&ns2_usb->bulk_in_anchor); >>> + spin_lock_init(&ns2_usb->bulk_in_lock); >>> + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work); >> >> usb_submit_urb is invoked for bulk_in[0] before the initialization of the >> spinlocks, anchors, and work struct. If the URB completes immediately, the >> completion handler will execute and attempt to use uninitialized >> synchronization primitives. Can this cause a crash? > > As the documentation for usb_submit_urb mentions that the call is async, > I assume that this is indeed a theoretical possibility. > >> >>> + >>> + usb_set_intfdata(intf, ns2_usb); >> [ ... ] >>> +err_kill_urb: >>> + usb_kill_urb(ns2_usb->bulk_in[0].urb); >>> +err_free_out: >>> + for (i = 0; i < NS2_OUT_URBS; i++) { >>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, >>> + ns2_usb->bulk_out[i].urb->transfer_dma); >>> + usb_free_urb(ns2_usb->bulk_out[i].urb); >>> + } >>> +err_free_in: >>> + for (i = 0; i < NS2_IN_URBS; i++) { >>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data, >>> + ns2_usb->bulk_in[i].urb->transfer_dma); >> >> If usb_alloc_urb fails during the loop, it jumps to err_free_in. During >> cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is >> NULL for the failed index, will this trigger a NULL pointer dereference? > > Yes, most likely we would need a new label for this case. > >> >>> + usb_free_urb(ns2_usb->bulk_in[i].urb); >>> + } >>> + devm_kfree(&intf->dev, ns2_usb); >>> + >>> + return ret; >>> +} >>> + >>> +static void switch2_usb_disconnect(struct usb_interface *intf) >>> +{ >>> + struct switch2_usb *ns2_usb = usb_get_intfdata(intf); >>> + unsigned long flags; >>> + int i; >>> + >>> + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags); >>> + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor); >> >> Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to >> call them while holding a spinlock? > > According to some googling, this indeed seems to be an isuse. The > non-sleeping way to cancel transfer requests would be to use > https://www.kernel.org/doc/html/v6.1/driver-api/usb/usb.html#c.usb_unlink_anchored_urbs This was removed in 6.17 and has been unused since 2009. > > >> >>> + for (i = 0; i < NS2_OUT_URBS; i++) { >>> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, >>> + ns2_usb->bulk_out[i].urb->transfer_dma); >>> + usb_free_urb(ns2_usb->bulk_out[i].urb); >>> + } >>> + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags); >>> + >>> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); >>> + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor); >>> + cancel_work_sync(&ns2_usb->message_in_work); >> >> Furthermore, cancel_work_sync waits for message_in_work which attempts to >> acquire bulk_in_lock. Does this cause a deadlock since the lock is already >> held by the disconnecting thread? > > Yes, this looks like a real concern as well. > > Cheers, > Silvan > >> >>> + for (i = 0; i < NS2_IN_URBS; i++) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 1/3] HID: nintendo: Add preliminary Switch 2 controller driver 2026-05-14 0:15 ` sashiko-bot 2026-05-23 8:57 ` Silvan Jegen @ 2026-06-19 3:56 ` Vicki Pfau 1 sibling, 0 replies; 15+ messages in thread From: Vicki Pfau @ 2026-06-19 3:56 UTC (permalink / raw) To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov Guess I'm doing LLM review replies now. On 5/13/26 5:15 PM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider: > - [High] Sleeping in atomic context and AB-BA deadlock in `switch2_usb_disconnect` > - [High] Race condition in IN URB tracking causes permanent starvation > - [High] Dangling pointers and use-after-free on probe failure > - [High] Memory corruption and double free due to incorrect devm input lifecycle management > - [High] NULL pointer dereference in USB probe error path > - [High] Division by zero crash from unvalidated trigger calibration data > - [High] BUG_ON kernel panic when `ida_free` is called with U32_MAX > - [High] Out-of-bounds read when parsing NS2_CMD_FW_INFO response > - [High] Race condition due to URB submission before initialization of synchronization primitives > - [Medium] Infinite USB retry storm on protocol errors > - [Medium] Unaligned memory access on byte array casts > -- > > commit 00cb7265ad5983144c6923702e2ee2c3c047d29e > Author: Vicki Pfau <vi@endrift.com> > Subject: HID: nintendo: Add preliminary Switch 2 controller driver > > This adds a new driver for the Switch 2 controllers. The Switch 2 uses an > unusual split-interface design such that input and rumble occur on the main > HID interface, but all other communication occurs over a "configuration" > interface. > >> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c >> index 29008c2cc5304..2193afcdefabc 100644 >> --- a/drivers/hid/hid-nintendo.c >> +++ b/drivers/hid/hid-nintendo.c > [ ... ] >> +static int switch2_init_input(struct switch2_controller *ns2) >> +{ > [ ... ] >> + input = devm_input_allocate_device(&hdev->dev); >> + if (!input) >> + return -ENOMEM; > [ ... ] >> + ret = input_register_device(input); >> + if (ret < 0) { >> + hid_err(ns2->hdev, "Failed to register input; ret=%d\n", ret); >> + input_free_device(input); > > Calling input_free_device or input_unregister_device on a devm-managed > input device can cause double frees when devres cleanup fires. Can this > lead to memory corruption? False positive. The documentation clearly states there is no devm equivalent and input_unregister_device works fine on devm-managed devices. The source of input_free_device also clearly shows logic for handling devm, and the source of input_device_unregister calls into a static devm_input_device_unregister if needed. > >> + return ret; >> + } > [ ... ] >> +static void switch2_controller_put(struct switch2_controller *ns2) >> +{ >> + struct input_dev *input; >> + bool do_free; > [ ... ] >> + if (input) >> + input_unregister_device(input); > > Calling input_unregister_device on a devm-managed input device can cause > issues. If the input device remains alive (tied to the HID device devres) > and its child LEDs are not released, will reconnecting the USB cable > allocate a new input device and corrupt the LED lists by re-registering > the exact same ns2->leds structures? Same false positive > >> + >> + if (do_free) { > [ ... ] >> +static void switch2_report_trigger(struct input_dev *input, uint8_t zero, int abs, uint8_t data) >> +{ >> + int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); > > The zero variable is loaded directly from the controller's factory calibration > flash. If a device reports a zero point of exactly 232, will this trigger a > division by zero exception and crash the kernel? Ok, an actual finding. I'll have this fixed in v6. > >> + >> + input_report_abs(input, abs, clamp(value, 0, NS2_TRIGGER_RANGE)); >> +} > [ ... ] >> +static int switch2_read_flash(struct switch2_controller *ns2, uint32_t address, >> + uint8_t size) >> +{ >> + uint8_t message[8] = { size, 0x7e }; >> + >> + if (!ns2->cfg) >> + return -ENOTCONN; >> + *(__le32 *)&message[4] = __cpu_to_le32(address); > > The driver directly casts &message[4] to __le32. Since message is a uint8_t > array, it is only guaranteed to be 1-byte aligned. On strict architectures, > will this unaligned memory write trigger a hardware alignment fault? Good catch. Fixed in v6. > >> + return ns2->cfg->send_command(NS2_CMD_FLASH, NS2_SUBCMD_FLASH_READ, message, >> + sizeof(message), ns2->cfg); >> +} > [ ... ] >> +int switch2_receive_command(struct switch2_controller *ns2, >> + const uint8_t *message, size_t length) >> +{ > [ ... ] >> + header = (const struct switch2_cmd_header *)message; >> + if (!(header->flags & NS2_FLAG_OK)) { >> + ret = -EIO; >> + goto exit; >> + } >> + message = &message[8]; >> + switch (header->command) { > [ ... ] >> + case NS2_CMD_FW_INFO: >> + if (header->subcommand == NS2_SUBCMD_FW_INFO_GET) { >> + if (length < sizeof(ns2->version)) { >> + ret = -EINVAL; >> + goto exit; >> + } >> + memcpy(&ns2->version, message, sizeof(ns2->version)); > > The message pointer is advanced by 8 bytes to skip the header, but length > is not decremented. The bounds check compares length against > sizeof(ns2->version). If length is exactly sizeof(ns2->version), the check > passes, but the advanced message pointer only has length - 8 valid bytes > remaining. Will the memcpy over-read into uninitialized memory? Fixed in v6. > >> + ns2->ctlr_type = ns2->version.ctlr_type; >> + switch2_init_step_done(ns2, NS2_INIT_GET_FIRMWARE_INFO); >> + } >> + break; >> + default: >> + break; >> + } >> + >> +exit: >> + if (ns2->init_step < NS2_INIT_DONE) >> + switch2_init_controller(ns2); > > If a command response has the NACK flag set or encounters another error, > the code jumps to exit and invokes switch2_init_controller. Since > ns2->init_step has not advanced, the state machine will immediately re-send > the exact same command. Can this cause an infinite tight feedback loop of > failures and retries, flooding the USB bus? This doesn't happen for problems already described in the thread previously. > >> + >> + return ret; >> +} > [ ... ] >> +int switch2_controller_attach_cfg(const char *phys, struct switch2_cfg_intf *cfg) >> +{ > [ ... ] >> + guard(mutex)(&ns2->lock); >> + WARN_ON(ns2->cfg); >> + ns2->cfg = cfg; >> + >> + if (ns2->hdev) >> + return switch2_init_controller(ns2); > > If switch2_init_controller fails here or in switch2_probe, the error is > returned immediately without unrolling the state. Will ns2->cfg or ns2->hdev > remain set, potentially leading to a use-after-free when subsequent device > events occur? This is a good catch. I'm not sure what the right fix is though; obviously a half-configured state is bad, but I'm not sure a full teardown is the best approach; it can leave it in a weird state, even if it's not a UAF risk. I'll go with a full teardown for now at least. > >> + return 0; >> +} > [ ... ] >> +static void switch2_remove(struct hid_device *hdev) >> +{ >> + struct switch2_controller *ns2 = hid_get_drvdata(hdev); >> + >> + hid_hw_close(hdev); >> + mutex_lock(&ns2->lock); >> + WARN_ON(ns2->hdev != hdev); >> + ns2->hdev = NULL; >> + mutex_unlock(&ns2->lock); >> + ida_free(&nintendo_player_id_allocator, ns2->player_id); > > If ida_alloc fails during probe, ns2->player_id remains at its default > value of U32_MAX. When ida_free is called, the IDA framework enforces a > check on the ID. Casting U32_MAX to a signed integer yields -1. Will this > trigger the BUG_ON and cause a kernel panic? I can't find any evidence of such a BUG_ON. A negative number will just early return. This seems like a hallucination. > >> + switch2_controller_put(ns2); >> + hid_hw_stop(hdev); >> +} > [ ... ] >> diff --git a/drivers/input/joystick/nintendo-switch2-usb.c b/drivers/input/joystick/nintendo-switch2-usb.c >> new file mode 100644 >> index 0000000000000..ebd89d852e21a >> --- /dev/null >> +++ b/drivers/input/joystick/nintendo-switch2-usb.c > [ ... ] >> +static void switch2_bulk_in(struct urb *urb) >> +{ > [ ... ] >> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); >> + for (i = 0; i < NS2_IN_URBS; i++) { >> + int err; >> + struct switch2_urb *ns2_urb; >> + >> + if (ns2_usb->bulk_in[i].urb == urb) { >> + ns2_usb->message_in = i; > > If multiple URBs complete in quick succession before the work item executes, > the message_in index is overwritten. Will this cause the work item to only > process the last completed URB, leaving earlier URBs indefinitely active > and leading to starvation of the URB pool? > In practice this probably won't happen but I guess it's worth fixing. >> + continue; >> + } > [ ... ] >> +static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) >> +{ > [ ... ] >> + ns2_usb->bulk_in[0].active = true; >> + ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC); >> + if (ret < 0) >> + goto err_free_out; >> + >> + init_usb_anchor(&ns2_usb->bulk_out_anchor); >> + spin_lock_init(&ns2_usb->bulk_out_lock); >> + init_usb_anchor(&ns2_usb->bulk_in_anchor); >> + spin_lock_init(&ns2_usb->bulk_in_lock); >> + INIT_WORK(&ns2_usb->message_in_work, switch2_usb_message_in_work); > > usb_submit_urb is invoked for bulk_in[0] before the initialization of the > spinlocks, anchors, and work struct. If the URB completes immediately, the > completion handler will execute and attempt to use uninitialized > synchronization primitives. Can this cause a crash? I don't know if this is possible but I guess I'll fix it. > >> + >> + usb_set_intfdata(intf, ns2_usb); > [ ... ] >> +err_kill_urb: >> + usb_kill_urb(ns2_usb->bulk_in[0].urb); >> +err_free_out: >> + for (i = 0; i < NS2_OUT_URBS; i++) { >> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, >> + ns2_usb->bulk_out[i].urb->transfer_dma); >> + usb_free_urb(ns2_usb->bulk_out[i].urb); >> + } >> +err_free_in: >> + for (i = 0; i < NS2_IN_URBS; i++) { >> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_in[i].data, >> + ns2_usb->bulk_in[i].urb->transfer_dma); > > If usb_alloc_urb fails during the loop, it jumps to err_free_in. During > cleanup, it accesses ns2_usb->bulk_in[i].urb->transfer_dma. Since urb is > NULL for the failed index, will this trigger a NULL pointer dereference? I removed the NULL check due to checkpatch telling me I didn't need it but I guess I'll put it back. > >> + usb_free_urb(ns2_usb->bulk_in[i].urb); >> + } >> + devm_kfree(&intf->dev, ns2_usb); >> + >> + return ret; >> +} >> + >> +static void switch2_usb_disconnect(struct usb_interface *intf) >> +{ >> + struct switch2_usb *ns2_usb = usb_get_intfdata(intf); >> + unsigned long flags; >> + int i; >> + >> + spin_lock_irqsave(&ns2_usb->bulk_out_lock, flags); >> + usb_kill_anchored_urbs(&ns2_usb->bulk_out_anchor); > > Both usb_kill_anchored_urbs and cancel_work_sync can sleep. Is it safe to > call them while holding a spinlock? Fixing in v6. > >> + for (i = 0; i < NS2_OUT_URBS; i++) { >> + usb_free_coherent(ns2_usb->udev, NS2_BULK_SIZE, ns2_usb->bulk_out[i].data, >> + ns2_usb->bulk_out[i].urb->transfer_dma); >> + usb_free_urb(ns2_usb->bulk_out[i].urb); >> + } >> + spin_unlock_irqrestore(&ns2_usb->bulk_out_lock, flags); >> + >> + spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags); >> + usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor); >> + cancel_work_sync(&ns2_usb->message_in_work); > > Furthermore, cancel_work_sync waits for message_in_work which attempts to > acquire bulk_in_lock. Does this cause a deadlock since the lock is already > held by the disconnecting thread? Likewise. > >> + for (i = 0; i < NS2_IN_URBS; i++) { > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] HID: nintendo: fix clang build of switch2_probe guard/goto 2026-05-12 20:00 ` [PATCH v5 1/3] " Vicki Pfau 2026-05-14 0:15 ` sashiko-bot @ 2026-06-22 9:47 ` Ryan McClelland 1 sibling, 0 replies; 15+ messages in thread From: Ryan McClelland @ 2026-06-22 9:47 UTC (permalink / raw) To: vi, dmitry.torokhov, jikos, bentiss, linux-input; +Cc: s.jegen, Ryan McClelland clang rejects the forward `goto err_close/err_stop` in switch2_probe() because they jump across the scope of the `guard(mutex)(&ns2->lock)` cleanup variable ("cannot jump from this goto statement to its label"). All three early error paths run before the lock is acquired, so replace the gotos with explicit hid_hw_close()/hid_hw_stop() cleanup and drop the now-unused labels. No functional change; gcc accepted the original. Signed-off-by: Ryan McClelland <rymcclel@gmail.com> --- Hi Vicki, I've been testing your Switch 2 series on a local machine here with JC2, GC, and Pro all over USB and hits this clang build break, Patch below , feel free to just squash it in. drivers/hid/hid-nintendo.c:4147:3: error: cannot jump from this goto statement to its label drivers/hid/hid-nintendo.c:4153:3: error: cannot jump from this goto statement to its label with the explanatory notes pointing at: note: jump bypasses initialization of variable with __attribute__((cleanup)) guard(mutex)(&ns2->lock); drivers/hid/hid-nintendo.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 40f57036d810..6a286d49e5bd 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -4144,13 +4144,16 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id ret = hid_hw_open(hdev); if (ret) { hid_err(hdev, "hw_open failed %d\n", ret); - goto err_stop; + hid_hw_stop(hdev); + return ret; } ns2 = switch2_get_controller(phys); if (IS_ERR(ns2)) { ret = PTR_ERR(ns2); - goto err_close; + hid_hw_close(hdev); + hid_hw_stop(hdev); + return ret; } guard(mutex)(&ns2->lock); @@ -4194,13 +4197,6 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id return switch2_init_controller(ns2); return 0; - -err_close: - hid_hw_close(hdev); -err_stop: - hid_hw_stop(hdev); - - return ret; } static void switch2_remove(struct hid_device *hdev) -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers 2026-05-12 20:00 [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Vicki Pfau 2026-05-12 20:00 ` [PATCH v5 1/3] " Vicki Pfau @ 2026-05-12 20:00 ` Vicki Pfau 2026-05-14 0:53 ` sashiko-bot 2026-05-12 20:00 ` [PATCH v5 3/3] HID: nintendo: Add unified report format support Vicki Pfau 2026-06-10 16:18 ` [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Jiri Kosina 3 siblings, 1 reply; 15+ messages in thread From: Vicki Pfau @ 2026-05-12 20:00 UTC (permalink / raw) To: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, linux-input Cc: Vicki Pfau, Silvan Jegen This adds rumble support for both the "HD Rumble" linear resonant actuator type as used in the Joy-Cons and Pro Controller, as well as the eccentric rotating mass type used in the GameCube controller. Note that since there's currently no API for exposing full control of LRAs with evdev, it only simulates a basic rumble for now. Reviewed-by: Silvan Jegen <s.jegen@gmail.com> Tested-by: Silvan Jegen <s.jegen@gmail.com> Signed-off-by: Vicki Pfau <vi@endrift.com> --- drivers/hid/Kconfig | 8 +- drivers/hid/hid-nintendo.c | 179 ++++++++++++++++++++++++++++++++++++- 2 files changed, 181 insertions(+), 6 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 19c77c323ec9..851eed76c236 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -859,10 +859,10 @@ config NINTENDO_FF depends on HID_NINTENDO select INPUT_FF_MEMLESS help - Say Y here if you have a Nintendo Switch controller and want to enable - force feedback support for it. This works for both joy-cons, the pro - controller, and the NSO N64 controller. For the pro controller, both - rumble motors can be controlled individually. + Say Y here if you have a Nintendo Switch or Switch 2 controller and want + to enable force feedback support for it. This works for Joy-Cons, the Pro + Controllers, and the NSO N64 and GameCube controller. For the Pro + Controller, both rumble motors can be controlled individually. config HID_NTI tristate "NTI keyboard adapters" diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 2193afcdefab..9801799a9075 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -2972,6 +2972,18 @@ struct switch2_stick_calibration { struct switch2_axis_calibration y; }; +struct switch2_hd_rumble { + uint16_t hi_freq : 10; + uint16_t hi_amp : 10; + uint16_t lo_freq : 10; + uint16_t lo_amp : 10; +} __packed; + +struct switch2_erm_rumble { + uint16_t error; + uint16_t amplitude; +}; + struct switch2_controller { struct hid_device *hdev; struct switch2_cfg_intf *cfg; @@ -2993,8 +3005,45 @@ struct switch2_controller { uint32_t player_id; struct led_classdev leds[4]; + +#if IS_ENABLED(CONFIG_NINTENDO_FF) + spinlock_t rumble_lock; + uint8_t rumble_seq; + union { + struct switch2_hd_rumble hd; + struct switch2_erm_rumble sd; + } rumble; + unsigned long last_rumble_work; + struct delayed_work rumble_work; + uint8_t rumble_buffer[64]; +#endif +}; + +enum gc_rumble { + GC_RUMBLE_OFF = 0, + GC_RUMBLE_ON = 1, + GC_RUMBLE_STOP = 2, }; +/* + * The highest rumble level for "HD Rumble" is strong enough to potentially damage the controller, + * and also leaves your hands feeling like melted jelly, so we set a semi-arbitrary scaling factor + * to artificially limit the maximum for safety and comfort. It is currently unknown if the Switch + * 2 itself does something similar, but it's quite likely. + * + * This value must be between 0 and 1024, otherwise the math below will overflow. + */ +#define RUMBLE_MAX 450u + +/* + * Semi-arbitrary values used to simulate the "rumble" sensation of an eccentric rotating + * mass type haptic motor on the Switch 2 controllers' linear resonant actuator type haptics. + * + * The units used are unknown, but the values must be between 0 and 1023. + */ +#define RUMBLE_HI_FREQ 0x187 +#define RUMBLE_LO_FREQ 0x112 + static DEFINE_MUTEX(switch2_controllers_lock); static LIST_HEAD(switch2_controllers); @@ -3084,9 +3133,12 @@ static const uint8_t switch2_init_cmd_data[] = { }; static const uint8_t switch2_one_data[] = { 0x01, 0x00, 0x00, 0x00 }; +#if IS_ENABLED(CONFIG_NINTENDO_FF) +static const uint8_t switch2_zero_data[] = { 0x00, 0x00, 0x00, 0x00 }; +#endif static const uint8_t switch2_feature_mask[] = { - NS2_FEATURE_BUTTONS | NS2_FEATURE_ANALOG | NS2_FEATURE_IMU, + NS2_FEATURE_BUTTONS | NS2_FEATURE_ANALOG | NS2_FEATURE_IMU | NS2_FEATURE_RUMBLE, 0x00, 0x00, 0x00 }; @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type) return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR; } +#if IS_ENABLED(CONFIG_NINTENDO_FF) +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5]) +{ + buffer[0] = rumble->hi_freq; + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2); + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4); + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6); + buffer[4] = rumble->lo_amp >> 2; +} + +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) +{ + struct switch2_controller *ns2 = input_get_drvdata(dev); + + if (effect->type != FF_RUMBLE) + return 0; + + guard(spinlock_irqsave)(&ns2->rumble_lock); + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) { + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude, + effect->u.rumble.weak_magnitude >> 1); + } else { + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16; + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16; + } + + if (ns2->hdev) + schedule_delayed_work(&ns2->rumble_work, 0); + + return 0; +} + +static void switch2_rumble_work(struct work_struct *work) +{ + struct switch2_controller *ns2 = container_of(to_delayed_work(work), + struct switch2_controller, rumble_work); + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64()); + unsigned long flags; + bool active; + int ret; + + spin_lock_irqsave(&ns2->rumble_lock, flags); + ns2->rumble_buffer[0x1] = 0x50 | ns2->rumble_seq; + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) { + ns2->rumble_buffer[0] = 3; + if (ns2->rumble.sd.amplitude == 0) { + ns2->rumble_buffer[2] = GC_RUMBLE_STOP; + ns2->rumble.sd.error = 0; + active = false; + } else { + if (ns2->rumble.sd.error < ns2->rumble.sd.amplitude) { + ns2->rumble_buffer[2] = GC_RUMBLE_ON; + ns2->rumble.sd.error += U16_MAX - ns2->rumble.sd.amplitude; + } else { + ns2->rumble_buffer[2] = GC_RUMBLE_OFF; + ns2->rumble.sd.error -= ns2->rumble.sd.amplitude; + } + active = true; + } + } else { + ns2->rumble_buffer[0] = 1; + switch2_encode_rumble(&ns2->rumble.hd, &ns2->rumble_buffer[0x2]); + active = ns2->rumble.hd.hi_amp || ns2->rumble.hd.lo_amp; + if (ns2->ctlr_type == NS2_CTLR_TYPE_PRO) { + /* + * The Pro Controller contains separate LRAs on each + * side that can be controlled individually. + */ + ns2->rumble_buffer[0] = 2; + ns2->rumble_buffer[0x11] = 0x50 | ns2->rumble_seq; + switch2_encode_rumble(&ns2->rumble.hd, &ns2->rumble_buffer[0x12]); + } + } + ns2->rumble_seq = (ns2->rumble_seq + 1) & 0xF; + + if (active) { + unsigned long interval = msecs_to_jiffies(4); + + if (!ns2->last_rumble_work) + ns2->last_rumble_work = current_ms; + else + ns2->last_rumble_work += interval; + schedule_delayed_work(&ns2->rumble_work, + ns2->last_rumble_work + interval - current_ms); + } else { + ns2->last_rumble_work = 0; + } + spin_unlock_irqrestore(&ns2->rumble_lock, flags); + + if (!ns2->hdev) { + cancel_delayed_work(&ns2->rumble_work); + ret = -ENODEV; + } else { + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64); + } + + if (ret < 0) + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret); +} +#endif + static int switch2_set_leds(struct switch2_controller *ns2) { int i; @@ -3226,6 +3379,15 @@ static int switch2_init_input(struct switch2_controller *ns2) return -EINVAL; } +#if IS_ENABLED(CONFIG_NINTENDO_FF) + input_set_capability(input, EV_FF, FF_RUMBLE); + ret = input_ff_create_memless(input, NULL, switch2_play_effect); + if (ret) { + input_free_device(input); + return ret; + } +#endif + hid_info(ns2->hdev, "Firmware version %u.%u.%u (type %i)\n", ns2->version.major, ns2->version.minor, ns2->version.patch, ns2->version.ctlr_type); if (ns2->version.dsp_type >= 0) @@ -3308,6 +3470,10 @@ static void switch2_controller_put(struct switch2_controller *ns2) if (input) input_unregister_device(input); +#if IS_ENABLED(CONFIG_NINTENDO_FF) + cancel_delayed_work_sync(&ns2->rumble_work); +#endif + if (do_free) { list_del_init(&ns2->entry); mutex_destroy(&ns2->lock); @@ -3667,7 +3833,8 @@ static int switch2_init_controller(struct switch2_controller *ns2) return ns2->cfg->send_command(NS2_CMD_FEATSEL, NS2_SUBCMD_FEATSEL_SET_MASK, switch2_feature_mask, sizeof(switch2_feature_mask), ns2->cfg); case NS2_INIT_ENABLE_FEATURES: - return switch2_features_enable(ns2, NS2_FEATURE_BUTTONS | NS2_FEATURE_ANALOG); + return switch2_features_enable(ns2, NS2_FEATURE_BUTTONS | + NS2_FEATURE_ANALOG | NS2_FEATURE_RUMBLE); case NS2_INIT_GRIP_BUTTONS: if (!switch2_ctlr_is_joycon(ns2->ctlr_type)) { switch2_init_step_done(ns2, ns2->init_step); @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id switch2_leds_create(ns2); +#if IS_ENABLED(CONFIG_NINTENDO_FF) + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) { + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ; + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ; + } + spin_lock_init(&ns2->rumble_lock); + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work); +#endif hid_set_drvdata(hdev, ns2); if (ns2->cfg) -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers 2026-05-12 20:00 ` [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers Vicki Pfau @ 2026-05-14 0:53 ` sashiko-bot 2026-05-30 20:03 ` Silvan Jegen 2026-06-19 4:17 ` Vicki Pfau 0 siblings, 2 replies; 15+ messages in thread From: sashiko-bot @ 2026-05-14 0:53 UTC (permalink / raw) To: Vicki Pfau; +Cc: dmitry.torokhov, linux-input Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble. - [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition. - [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers. - [Low] Implicit type promotion in the max() macro causes a compiler warning. - [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors. -- commit d72f0f8e0dadde2cacc27050b92088c2e5cbea0e Author: Vicki Pfau <vi@endrift.com> Subject: HID: nintendo: Add rumble support for Switch 2 controllers This commit introduces rumble support for Switch 2 controllers, handling both HD Rumble linear resonant actuators and eccentric rotating mass haptics. It adds state machines for rumble scheduling and output reporting via hdev. > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 26ef589f652c6..a7fe74d982475 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -850,10 +850,10 @@ config NINTENDO_FF > depends on HID_NINTENDO > select INPUT_FF_MEMLESS > help > - Say Y here if you have a Nintendo Switch controller and want to enable > - force feedback support for it. This works for both joy-cons, the pro > - controller, and the NSO N64 controller. For the pro controller, both > - rumble motors can be controlled individually. > + Say Y here if you have a Nintendo Switch or Switch 2 controller and want > + to enable force feedback support for it. This works for Joy-Cons, the Pro > + Controllers, and the NSO N64 and GameCube controller. For the Pro > + Controller, both rumble motors can be controlled individually. This isn't a bug, but is the help text accurate regarding individual motor control? The implementation in switch2_rumble_work() appears to encode the exact same rumble configuration to both the left and right motor offsets, which would make both motors vibrate identically. > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 2193afcdefabc..9801799a90758 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c [ ... ] > @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type) > return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR; > } > > +#if IS_ENABLED(CONFIG_NINTENDO_FF) > +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5]) > +{ > + buffer[0] = rumble->hi_freq; > + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2); > + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4); > + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6); > + buffer[4] = rumble->lo_amp >> 2; > +} > + > +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) > +{ > + struct switch2_controller *ns2 = input_get_drvdata(dev); > + > + if (effect->type != FF_RUMBLE) > + return 0; > + > + guard(spinlock_irqsave)(&ns2->rumble_lock); > + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) { > + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude, > + effect->u.rumble.weak_magnitude >> 1); Will this max calculation trigger a compiler warning? The strict type checking in the max macro might fail because effect->u.rumble.weak_magnitude >> 1 promotes to an int, while strong_magnitude is a u16. > + } else { > + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16; > + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16; > + } > + > + if (ns2->hdev) > + schedule_delayed_work(&ns2->rumble_work, 0); > + > + return 0; > +} > + > +static void switch2_rumble_work(struct work_struct *work) > +{ > + struct switch2_controller *ns2 = container_of(to_delayed_work(work), > + struct switch2_controller, rumble_work); > + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64()); > + unsigned long flags; > + bool active; > + int ret; [ ... ] > + if (active) { > + unsigned long interval = msecs_to_jiffies(4); > + > + if (!ns2->last_rumble_work) > + ns2->last_rumble_work = current_ms; > + else > + ns2->last_rumble_work += interval; > + schedule_delayed_work(&ns2->rumble_work, > + ns2->last_rumble_work + interval - current_ms); Does this code underflow the scheduled delay? The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(), but interval is calculated in jiffies using msecs_to_jiffies(). Since ns2->last_rumble_work is initialized to milliseconds but incremented by jiffies, subtracting current_ms could yield a negative value. On systems where HZ is not 1000, could this underflow the delay parameter and schedule the next work millions of jiffies in the future? > + } else { > + ns2->last_rumble_work = 0; > + } > + spin_unlock_irqrestore(&ns2->rumble_lock, flags); > + > + if (!ns2->hdev) { > + cancel_delayed_work(&ns2->rumble_work); > + ret = -ENODEV; > + } else { > + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64); > + } > + > + if (ret < 0) > + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret); If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call. Could this result in a NULL pointer dereference, as the hid_dbg macro passes ns2->hdev to dev_dbg()? Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there a time-of-check to time-of-use race where the compiler might reload the pointer between the check and hid_hw_output_report()? [ ... ] > @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id > > switch2_leds_create(ns2); > > +#if IS_ENABLED(CONFIG_NINTENDO_FF) > + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) { > + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ; > + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ; > + } Are we checking ns2->ctlr_type before it is dynamically initialized? During probe, ns2->ctlr_type appears to be 0 since the struct is freshly allocated and the type is discovered later during the init state machine. This would cause the high and low frequencies to always be written. Since hd and sd share a union, could this corrupt the sd.error and sd.amplitude fields for actual GameCube controllers? > + spin_lock_init(&ns2->rumble_lock); > + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work); > +#endif > hid_set_drvdata(hdev, ns2); -- Sashiko AI review · https://sashiko.dev/#/patchset/20260512200051.2534081-1-vi@endrift.com?part=2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers 2026-05-14 0:53 ` sashiko-bot @ 2026-05-30 20:03 ` Silvan Jegen 2026-06-19 4:17 ` Vicki Pfau 1 sibling, 0 replies; 15+ messages in thread From: Silvan Jegen @ 2026-05-30 20:03 UTC (permalink / raw) To: sashiko-reviews; +Cc: Vicki Pfau, dmitry.torokhov, linux-input sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: > - [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble. > - [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition. > - [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers. > - [Low] Implicit type promotion in the max() macro causes a compiler warning. > - [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors. > -- > > commit d72f0f8e0dadde2cacc27050b92088c2e5cbea0e > Author: Vicki Pfau <vi@endrift.com> > Subject: HID: nintendo: Add rumble support for Switch 2 controllers > > This commit introduces rumble support for Switch 2 controllers, handling both HD > Rumble linear resonant actuators and eccentric rotating mass haptics. It adds > state machines for rumble scheduling and output reporting via hdev. > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > index 26ef589f652c6..a7fe74d982475 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -850,10 +850,10 @@ config NINTENDO_FF > > depends on HID_NINTENDO > > select INPUT_FF_MEMLESS > > help > > - Say Y here if you have a Nintendo Switch controller and want to enable > > - force feedback support for it. This works for both joy-cons, the pro > > - controller, and the NSO N64 controller. For the pro controller, both > > - rumble motors can be controlled individually. > > + Say Y here if you have a Nintendo Switch or Switch 2 controller and want > > + to enable force feedback support for it. This works for Joy-Cons, the Pro > > + Controllers, and the NSO N64 and GameCube controller. For the Pro > > + Controller, both rumble motors can be controlled individually. > > This isn't a bug, but is the help text accurate regarding individual motor > control? The implementation in switch2_rumble_work() appears to encode the > exact same rumble configuration to both the left and right motor offsets, > which would make both motors vibrate identically. As pointers to different offsets into the rumble_buffer are passed, I assume that the vibrations will actually not be identical. > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > index 2193afcdefabc..9801799a90758 100644 > > --- a/drivers/hid/hid-nintendo.c > > +++ b/drivers/hid/hid-nintendo.c > > [ ... ] > > > @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type) > > return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR; > > } > > > > +#if IS_ENABLED(CONFIG_NINTENDO_FF) > > +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5]) > > +{ > > + buffer[0] = rumble->hi_freq; > > + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2); > > + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4); > > + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6); > > + buffer[4] = rumble->lo_amp >> 2; > > +} > > + > > +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) > > +{ > > + struct switch2_controller *ns2 = input_get_drvdata(dev); > > + > > + if (effect->type != FF_RUMBLE) > > + return 0; > > + > > + guard(spinlock_irqsave)(&ns2->rumble_lock); > > + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) { > > + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude, > > + effect->u.rumble.weak_magnitude >> 1); > > Will this max calculation trigger a compiler warning? The strict type checking > in the max macro might fail because effect->u.rumble.weak_magnitude >> 1 > promotes to an int, while strong_magnitude is a u16. Apparently it is not strict enough to emit a warning on my machine at least ... > > > + } else { > > + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16; > > + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16; > > + } > > + > > + if (ns2->hdev) > > + schedule_delayed_work(&ns2->rumble_work, 0); > > + > > + return 0; > > +} > > + > > +static void switch2_rumble_work(struct work_struct *work) > > +{ > > + struct switch2_controller *ns2 = container_of(to_delayed_work(work), > > + struct switch2_controller, rumble_work); > > + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64()); > > + unsigned long flags; > > + bool active; > > + int ret; > > [ ... ] > > > + if (active) { > > + unsigned long interval = msecs_to_jiffies(4); > > + > > + if (!ns2->last_rumble_work) > > + ns2->last_rumble_work = current_ms; > > + else > > + ns2->last_rumble_work += interval; > > + schedule_delayed_work(&ns2->rumble_work, > > + ns2->last_rumble_work + interval - current_ms); > > Does this code underflow the scheduled delay? > > The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(), > but interval is calculated in jiffies using msecs_to_jiffies(). Since > ns2->last_rumble_work is initialized to milliseconds but incremented by > jiffies, subtracting current_ms could yield a negative value. > > On systems where HZ is not 1000, could this underflow the delay parameter and > schedule the next work millions of jiffies in the future? It's definitely suspicious that we are adding jiffies to milliseconds. We should probably fix that. > > > + } else { > > + ns2->last_rumble_work = 0; > > + } > > + spin_unlock_irqrestore(&ns2->rumble_lock, flags); > > + > > + if (!ns2->hdev) { > > + cancel_delayed_work(&ns2->rumble_work); > > + ret = -ENODEV; > > + } else { > > + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64); > > + } > > + > > + if (ret < 0) > > + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret); > > If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call. > Could this result in a NULL pointer dereference, as the hid_dbg macro passes > ns2->hdev to dev_dbg()? Since we set `ret = -ENODEV` when ns2->hdev is NULL we would indeed end up calling hid_dbg() with a NULL argument. > > Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there > a time-of-check to time-of-use race where the compiler might reload the pointer > between the check and hid_hw_output_report()? > > [ ... ] > > > @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id > > > > switch2_leds_create(ns2); > > > > +#if IS_ENABLED(CONFIG_NINTENDO_FF) > > + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) { > > + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ; > > + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ; > > + } > > Are we checking ns2->ctlr_type before it is dynamically initialized? > > During probe, ns2->ctlr_type appears to be 0 since the struct is freshly > allocated and the type is discovered later during the init state machine. > This would cause the high and low frequencies to always be written. As switch2_init_controller is called from the other driver's probe function, this might be fine. To avoid writing these values if the initialisation has not happened yet, we should probably guard against a 0 value. Cheers, Silvan > > Since hd and sd share a union, could this corrupt the sd.error and > sd.amplitude fields for actual GameCube controllers? > > > + spin_lock_init(&ns2->rumble_lock); > > + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work); > > +#endif > > hid_set_drvdata(hdev, ns2); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers 2026-05-14 0:53 ` sashiko-bot 2026-05-30 20:03 ` Silvan Jegen @ 2026-06-19 4:17 ` Vicki Pfau 1 sibling, 0 replies; 15+ messages in thread From: Vicki Pfau @ 2026-06-19 4:17 UTC (permalink / raw) To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input Let's tackle some more hallucinations. On 5/13/26 5:53 PM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: > - [High] Mixing jiffies and milliseconds in delayed work scheduling logic causes an integer underflow, permanently stalling the haptic rumble. > - [High] A guaranteed NULL pointer dereference occurs in the error logging path of switch2_rumble_work, as well as a TOCTOU race condition. > - [Medium] `ns2->ctlr_type` is checked before it is dynamically initialized, causing union corruption for GameCube controllers. > - [Low] Implicit type promotion in the max() macro causes a compiler warning. > - [Low] Kconfig documentation claims individual motor control for the Pro Controller, but the implementation identicalizes the rumble data for both motors. > -- > > commit d72f0f8e0dadde2cacc27050b92088c2e5cbea0e > Author: Vicki Pfau <vi@endrift.com> > Subject: HID: nintendo: Add rumble support for Switch 2 controllers > > This commit introduces rumble support for Switch 2 controllers, handling both HD > Rumble linear resonant actuators and eccentric rotating mass haptics. It adds > state machines for rumble scheduling and output reporting via hdev. > >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >> index 26ef589f652c6..a7fe74d982475 100644 >> --- a/drivers/hid/Kconfig >> +++ b/drivers/hid/Kconfig >> @@ -850,10 +850,10 @@ config NINTENDO_FF >> depends on HID_NINTENDO >> select INPUT_FF_MEMLESS >> help >> - Say Y here if you have a Nintendo Switch controller and want to enable >> - force feedback support for it. This works for both joy-cons, the pro >> - controller, and the NSO N64 controller. For the pro controller, both >> - rumble motors can be controlled individually. >> + Say Y here if you have a Nintendo Switch or Switch 2 controller and want >> + to enable force feedback support for it. This works for Joy-Cons, the Pro >> + Controllers, and the NSO N64 and GameCube controller. For the Pro >> + Controller, both rumble motors can be controlled individually. > > This isn't a bug, but is the help text accurate regarding individual motor > control? The implementation in switch2_rumble_work() appears to encode the > exact same rumble configuration to both the left and right motor offsets, > which would make both motors vibrate identically. I didn't add this text, so I'm ingoring this. > >> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c >> index 2193afcdefabc..9801799a90758 100644 >> --- a/drivers/hid/hid-nintendo.c >> +++ b/drivers/hid/hid-nintendo.c > > [ ... ] > >> @@ -3103,6 +3155,107 @@ static inline bool switch2_ctlr_is_joycon(enum switch2_ctlr_type type) >> return type == NS2_CTLR_TYPE_JCL || type == NS2_CTLR_TYPE_JCR; >> } >> >> +#if IS_ENABLED(CONFIG_NINTENDO_FF) >> +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5]) >> +{ >> + buffer[0] = rumble->hi_freq; >> + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2); >> + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4); >> + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6); >> + buffer[4] = rumble->lo_amp >> 2; >> +} >> + >> +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) >> +{ >> + struct switch2_controller *ns2 = input_get_drvdata(dev); >> + >> + if (effect->type != FF_RUMBLE) >> + return 0; >> + >> + guard(spinlock_irqsave)(&ns2->rumble_lock); >> + if (ns2->ctlr_type == NS2_CTLR_TYPE_GC) { >> + ns2->rumble.sd.amplitude = max(effect->u.rumble.strong_magnitude, >> + effect->u.rumble.weak_magnitude >> 1); > > Will this max calculation trigger a compiler warning? The strict type checking > in the max macro might fail because effect->u.rumble.weak_magnitude >> 1 > promotes to an int, while strong_magnitude is a u16. It doesn't cause a warning locally, but my warnings seem lax for some reason. I'm not sure how to fix that, but I guess adding a cast here is harmless. > >> + } else { >> + ns2->rumble.hd.hi_amp = effect->u.rumble.weak_magnitude * RUMBLE_MAX >> 16; >> + ns2->rumble.hd.lo_amp = effect->u.rumble.strong_magnitude * RUMBLE_MAX >> 16; >> + } >> + >> + if (ns2->hdev) >> + schedule_delayed_work(&ns2->rumble_work, 0); >> + >> + return 0; >> +} >> + >> +static void switch2_rumble_work(struct work_struct *work) >> +{ >> + struct switch2_controller *ns2 = container_of(to_delayed_work(work), >> + struct switch2_controller, rumble_work); >> + unsigned long current_ms = jiffies_to_msecs(get_jiffies_64()); >> + unsigned long flags; >> + bool active; >> + int ret; > > [ ... ] > >> + if (active) { >> + unsigned long interval = msecs_to_jiffies(4); >> + >> + if (!ns2->last_rumble_work) >> + ns2->last_rumble_work = current_ms; >> + else >> + ns2->last_rumble_work += interval; >> + schedule_delayed_work(&ns2->rumble_work, >> + ns2->last_rumble_work + interval - current_ms); > > Does this code underflow the scheduled delay? > > The current_ms variable is retrieved in milliseconds using jiffies_to_msecs(), > but interval is calculated in jiffies using msecs_to_jiffies(). Since > ns2->last_rumble_work is initialized to milliseconds but incremented by > jiffies, subtracting current_ms could yield a negative value. > > On systems where HZ is not 1000, could this underflow the delay parameter and > schedule the next work millions of jiffies in the future? Fixing in v6. > >> + } else { >> + ns2->last_rumble_work = 0; >> + } >> + spin_unlock_irqrestore(&ns2->rumble_lock, flags); >> + >> + if (!ns2->hdev) { >> + cancel_delayed_work(&ns2->rumble_work); >> + ret = -ENODEV; >> + } else { >> + ret = hid_hw_output_report(ns2->hdev, ns2->rumble_buffer, 64); >> + } >> + >> + if (ret < 0) >> + hid_dbg(ns2->hdev, "Failed to send output report ret=%d\n", ret); > > If ns2->hdev is NULL, we set ret = -ENODEV and then reach this hid_dbg call. > Could this result in a NULL pointer dereference, as the hid_dbg macro passes > ns2->hdev to dev_dbg()? Fixing in v6 > > Also, since ns2->hdev is accessed locklessly and without READ_ONCE(), is there > a time-of-check to time-of-use race where the compiler might reload the pointer > between the check and hid_hw_output_report()? This can only get changed in switch2_remove, which is missing a cancel_delayed_work_sync. Fixing in v6. > > [ ... ] > >> @@ -3881,6 +4048,14 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id >> >> switch2_leds_create(ns2); >> >> +#if IS_ENABLED(CONFIG_NINTENDO_FF) >> + if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) { >> + ns2->rumble.hd.hi_freq = RUMBLE_HI_FREQ; >> + ns2->rumble.hd.lo_freq = RUMBLE_LO_FREQ; >> + } > > Are we checking ns2->ctlr_type before it is dynamically initialized? > > During probe, ns2->ctlr_type appears to be 0 since the struct is freshly > allocated and the type is discovered later during the init state machine. > This would cause the high and low frequencies to always be written. > > Since hd and sd share a union, could this corrupt the sd.error and > sd.amplitude fields for actual GameCube controllers? Really, really unimportant and technically "works by accident" territory but I'll move the logic anyway. > >> + spin_lock_init(&ns2->rumble_lock); >> + INIT_DELAYED_WORK(&ns2->rumble_work, switch2_rumble_work); >> +#endif >> hid_set_drvdata(hdev, ns2); > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 3/3] HID: nintendo: Add unified report format support 2026-05-12 20:00 [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Vicki Pfau 2026-05-12 20:00 ` [PATCH v5 1/3] " Vicki Pfau 2026-05-12 20:00 ` [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers Vicki Pfau @ 2026-05-12 20:00 ` Vicki Pfau 2026-05-14 1:06 ` sashiko-bot 2026-06-10 16:18 ` [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Jiri Kosina 3 siblings, 1 reply; 15+ messages in thread From: Vicki Pfau @ 2026-05-12 20:00 UTC (permalink / raw) To: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, linux-input Cc: Vicki Pfau, Silvan Jegen This adds support for the "unified" report format that all controllers also support, which has overlapping fields for like buttons and axes between them. Reviewed-by: Silvan Jegen <s.jegen@gmail.com> Tested-by: Silvan Jegen <s.jegen@gmail.com> Signed-off-by: Vicki Pfau <vi@endrift.com> --- drivers/hid/hid-nintendo.c | 148 +++++++++++++++++++++++++++++++++++-- 1 file changed, 143 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 9801799a9075..9e7eda7b70bf 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -2824,6 +2824,36 @@ static int joycon_suspend(struct hid_device *hdev, pm_message_t message) #define NS2_BTN3_SR BIT(6) #define NS2_BTN3_SL BIT(7) +#define NS2_BTN_U1_Y BIT(0) +#define NS2_BTN_U1_X BIT(1) +#define NS2_BTN_U1_B BIT(2) +#define NS2_BTN_U1_A BIT(3) +#define NS2_BTN_U1_SR BIT(4) +#define NS2_BTN_U1_SL BIT(5) +#define NS2_BTN_U1_R BIT(6) +#define NS2_BTN_U1_ZR BIT(7) + +#define NS2_BTN_U2_MINUS BIT(0) +#define NS2_BTN_U2_PLUS BIT(1) +#define NS2_BTN_U2_RS BIT(2) +#define NS2_BTN_U2_LS BIT(3) +#define NS2_BTN_U2_HOME BIT(4) +#define NS2_BTN_U2_CAPTURE BIT(5) +#define NS2_BTN_U2_C BIT(6) + +#define NS2_BTN_U3_DOWN BIT(0) +#define NS2_BTN_U3_UP BIT(1) +#define NS2_BTN_U3_RIGHT BIT(2) +#define NS2_BTN_U3_LEFT BIT(3) +#define NS2_BTN_U3_SR BIT(4) +#define NS2_BTN_U3_SL BIT(5) +#define NS2_BTN_U3_L BIT(6) +#define NS2_BTN_U3_ZL BIT(7) + +#define NS2_BTN_U4_GR BIT(0) +#define NS2_BTN_U4_GL BIT(1) +#define NS2_BTN_U4_HEADSET BIT(5) + #define NS2_BTN_JCR_HOME BIT(0) #define NS2_BTN_JCR_GR BIT(2) #define NS2_BTN_JCR_C NS2_BTN3_C @@ -3069,6 +3099,22 @@ static const struct switch2_ctlr_button_mapping ns2_left_joycon_button_mappings[ { /* sentinel */ }, }; +static const struct switch2_ctlr_button_mapping ns2_left_joycon_button_unified_mappings[] = { + { BTN_DPAD_LEFT, 2, NS2_BTN_U3_LEFT, }, + { BTN_DPAD_UP, 2, NS2_BTN_U3_UP, }, + { BTN_DPAD_DOWN, 2, NS2_BTN_U3_DOWN, }, + { BTN_DPAD_RIGHT, 2, NS2_BTN_U3_RIGHT, }, + { BTN_TL, 2, NS2_BTN_U3_L, }, + { BTN_TL2, 2, NS2_BTN_U3_ZL, }, + { BTN_SELECT, 1, NS2_BTN_U2_MINUS, }, + { BTN_THUMBL, 1, NS2_BTN_U2_LS, }, + { KEY_RECORD, 1, NS2_BTN_U2_CAPTURE, }, + { BTN_GRIPR, 2, NS2_BTN_U3_SL, }, + { BTN_GRIPR2, 2, NS2_BTN_U3_SR, }, + { BTN_GRIPL, 3, NS2_BTN_U4_GL, }, + { /* sentinel */ }, +}; + static const struct switch2_ctlr_button_mapping ns2_right_joycon_button_mappings[] = { { BTN_SOUTH, 0, NS2_BTNR_A, }, { BTN_EAST, 0, NS2_BTNR_B, }, @@ -3086,6 +3132,23 @@ static const struct switch2_ctlr_button_mapping ns2_right_joycon_button_mappings { /* sentinel */ }, }; +static const struct switch2_ctlr_button_mapping ns2_right_joycon_button_unified_mappings[] = { + { BTN_SOUTH, 0, NS2_BTN_U1_A, }, + { BTN_EAST, 0, NS2_BTN_U1_B, }, + { BTN_NORTH, 0, NS2_BTN_U1_X, }, + { BTN_WEST, 0, NS2_BTN_U1_Y, }, + { BTN_TR, 0, NS2_BTN_U1_R, }, + { BTN_TR2, 0, NS2_BTN_U1_ZR }, + { BTN_START, 1, NS2_BTN_U2_PLUS, }, + { BTN_THUMBR, 1, NS2_BTN_U2_RS, }, + { BTN_C, 1, NS2_BTN_U2_C, }, + { BTN_MODE, 1, NS2_BTN_U2_HOME, }, + { BTN_GRIPL2, 0, NS2_BTN_U1_SL, }, + { BTN_GRIPL, 0, NS2_BTN_U1_SR, }, + { BTN_GRIPR, 3, NS2_BTN_U4_GR, }, + { /* sentinel */ }, +}; + static const struct switch2_ctlr_button_mapping ns2_procon_mappings[] = { { BTN_SOUTH, 0, NS2_BTNR_A, }, { BTN_EAST, 0, NS2_BTNR_B, }, @@ -3107,6 +3170,27 @@ static const struct switch2_ctlr_button_mapping ns2_procon_mappings[] = { { /* sentinel */ }, }; +static const struct switch2_ctlr_button_mapping ns2_procon_unified_mappings[] = { + { BTN_SOUTH, 0, NS2_BTN_U1_A, }, + { BTN_EAST, 0, NS2_BTN_U1_B, }, + { BTN_NORTH, 0, NS2_BTN_U1_X, }, + { BTN_WEST, 0, NS2_BTN_U1_Y, }, + { BTN_TL, 2, NS2_BTN_U3_L, }, + { BTN_TR, 0, NS2_BTN_U1_R, }, + { BTN_TL2, 2, NS2_BTN_U3_ZL, }, + { BTN_TR2, 0, NS2_BTN_U1_ZR, }, + { BTN_SELECT, 1, NS2_BTN_U2_MINUS, }, + { BTN_START, 1, NS2_BTN_U2_PLUS, }, + { BTN_THUMBL, 1, NS2_BTN_U2_LS, }, + { BTN_THUMBR, 1, NS2_BTN_U2_RS, }, + { BTN_MODE, 1, NS2_BTN_U2_HOME }, + { KEY_RECORD, 1, NS2_BTN_U2_CAPTURE }, + { BTN_GRIPR, 3, NS2_BTN_U4_GR }, + { BTN_GRIPL, 3, NS2_BTN_U4_GL }, + { BTN_C, 1, NS2_BTN_U2_C }, + { /* sentinel */ }, +}; + static const struct switch2_ctlr_button_mapping ns2_gccon_mappings[] = { { BTN_SOUTH, 0, NS2_BTNR_A, }, { BTN_EAST, 0, NS2_BTNR_B, }, @@ -3124,6 +3208,23 @@ static const struct switch2_ctlr_button_mapping ns2_gccon_mappings[] = { { /* sentinel */ }, }; +static const struct switch2_ctlr_button_mapping ns2_gccon_unified_mappings[] = { + { BTN_SOUTH, 0, NS2_BTN_U1_A, }, + { BTN_EAST, 0, NS2_BTN_U1_B, }, + { BTN_NORTH, 0, NS2_BTN_U1_X, }, + { BTN_WEST, 0, NS2_BTN_U1_Y, }, + { BTN_TL2, 2, NS2_BTN_U3_L, }, + { BTN_TR2, 0, NS2_BTN_U1_R, }, + { BTN_TL, 2, NS2_BTN_U3_ZL }, + { BTN_TR, 0, NS2_BTN_U1_ZR }, + { BTN_SELECT, 1, NS2_BTN_U2_MINUS, }, + { BTN_START, 1, NS2_BTN_U2_PLUS, }, + { BTN_MODE, 1, NS2_BTN_U2_HOME }, + { KEY_RECORD, 1, NS2_BTN_U2_CAPTURE }, + { BTN_C, 1, NS2_BTN_U2_C }, + { /* sentinel */ }, +}; + static const uint8_t switch2_init_cmd_data[] = { /* * The last 6 bytes of this packet are the MAC address of @@ -3690,11 +3791,48 @@ static int switch2_event(struct hid_device *hdev, struct hid_report *report, uin switch (report->id) { case NS2_REPORT_UNIFIED: - /* - * TODO - * This won't be sent unless the report type gets changed via command - * 03-0A, but we should support it at some point regardless. - */ + switch (ns2->ctlr_type) { + case NS2_CTLR_TYPE_JCL: + switch2_report_stick(input, &ns2->stick_calib[0], + ABS_X, false, ABS_Y, true, &raw_data[11]); + switch2_report_buttons(input, &raw_data[5], + ns2_left_joycon_button_unified_mappings); + break; + case NS2_CTLR_TYPE_JCR: + switch2_report_stick(input, &ns2->stick_calib[0], + ABS_X, false, ABS_Y, true, &raw_data[14]); + switch2_report_buttons(input, &raw_data[5], + ns2_right_joycon_button_unified_mappings); + break; + case NS2_CTLR_TYPE_GC: + input_report_abs(input, ABS_HAT0X, + !!(raw_data[7] & NS2_BTN_U3_RIGHT) - + !!(raw_data[7] & NS2_BTN_U3_LEFT)); + input_report_abs(input, ABS_HAT0Y, + !!(raw_data[7] & NS2_BTN_U3_DOWN) - + !!(raw_data[7] & NS2_BTN_U3_UP)); + switch2_report_buttons(input, &raw_data[5], ns2_gccon_unified_mappings); + switch2_report_stick(input, &ns2->stick_calib[0], + ABS_X, false, ABS_Y, true, &raw_data[11]); + switch2_report_stick(input, &ns2->stick_calib[1], + ABS_RX, false, ABS_RY, true, &raw_data[14]); + switch2_report_trigger(input, ns2->lt_zero, ABS_Z, raw_data[0x3d]); + switch2_report_trigger(input, ns2->rt_zero, ABS_RZ, raw_data[0x3e]); + break; + case NS2_CTLR_TYPE_PRO: + input_report_abs(input, ABS_HAT0X, + !!(raw_data[7] & NS2_BTN_U3_RIGHT) - + !!(raw_data[7] & NS2_BTN_U3_LEFT)); + input_report_abs(input, ABS_HAT0Y, + !!(raw_data[7] & NS2_BTN_U3_DOWN) - + !!(raw_data[7] & NS2_BTN_U3_UP)); + switch2_report_buttons(input, &raw_data[5], ns2_procon_unified_mappings); + switch2_report_stick(input, &ns2->stick_calib[0], + ABS_X, false, ABS_Y, true, &raw_data[11]); + switch2_report_stick(input, &ns2->stick_calib[1], + ABS_RX, false, ABS_RY, true, &raw_data[14]); + break; + } break; case NS2_REPORT_JCL: switch2_report_stick(input, &ns2->stick_calib[0], ABS_X, false, -- 2.54.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 3/3] HID: nintendo: Add unified report format support 2026-05-12 20:00 ` [PATCH v5 3/3] HID: nintendo: Add unified report format support Vicki Pfau @ 2026-05-14 1:06 ` sashiko-bot 2026-05-31 17:57 ` Silvan Jegen 0 siblings, 1 reply; 15+ messages in thread From: sashiko-bot @ 2026-05-14 1:06 UTC (permalink / raw) To: Vicki Pfau; +Cc: linux-input, dmitry.torokhov Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Insufficient bounds checking for NS2_REPORT_UNIFIED payload leads to out-of-bounds reads. - [High] Divide-by-zero kernel panic in switch2_report_trigger() caused by attacker-controlled calibration data. -- commit 6e45b564fde8014fc6ade8452a1b793097b3bee4 Author: Vicki Pfau <vi@endrift.com> Subject: HID: nintendo: Add unified report format support This commit adds support for the "unified" report format to the nintendo hid driver. It introduces new mappings for overlapping button and axis fields across different controller types. > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 9801799a90758..9e7eda7b70bfb 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c [ ... ] > @@ -3690,11 +3791,48 @@ static int switch2_event(struct hid_device *hdev, struct hid_report *report, uin > > switch (report->id) { > case NS2_REPORT_UNIFIED: > - /* > - * TODO > - * This won't be sent unless the report type gets changed via command > - * 03-0A, but we should support it at some point regardless. > - */ > + switch (ns2->ctlr_type) { > + case NS2_CTLR_TYPE_JCL: > + switch2_report_stick(input, &ns2->stick_calib[0], > + ABS_X, false, ABS_Y, true, &raw_data[11]); > + switch2_report_buttons(input, &raw_data[5], > + ns2_left_joycon_button_unified_mappings); > + break; > + case NS2_CTLR_TYPE_JCR: > + switch2_report_stick(input, &ns2->stick_calib[0], > + ABS_X, false, ABS_Y, true, &raw_data[14]); Is this safe if the report size is exactly 15 bytes? Earlier in switch2_event(), there is a global size check: if (size < 15) return -EINVAL; However, switch2_report_stick() reads up to 3 bytes from the provided data pointer (indices 0, 1, and 2). Passing &raw_data[14] means it will read raw_data[14], raw_data[15], and raw_data[16]. Could this cause an out-of-bounds read if an undersized report is sent? > + switch2_report_buttons(input, &raw_data[5], > + ns2_right_joycon_button_unified_mappings); > + break; > + case NS2_CTLR_TYPE_GC: > + input_report_abs(input, ABS_HAT0X, > + !!(raw_data[7] & NS2_BTN_U3_RIGHT) - > + !!(raw_data[7] & NS2_BTN_U3_LEFT)); > + input_report_abs(input, ABS_HAT0Y, > + !!(raw_data[7] & NS2_BTN_U3_DOWN) - > + !!(raw_data[7] & NS2_BTN_U3_UP)); > + switch2_report_buttons(input, &raw_data[5], ns2_gccon_unified_mappings); > + switch2_report_stick(input, &ns2->stick_calib[0], > + ABS_X, false, ABS_Y, true, &raw_data[11]); > + switch2_report_stick(input, &ns2->stick_calib[1], > + ABS_RX, false, ABS_RY, true, &raw_data[14]); > + switch2_report_trigger(input, ns2->lt_zero, ABS_Z, raw_data[0x3d]); > + switch2_report_trigger(input, ns2->rt_zero, ABS_RZ, raw_data[0x3e]); Similarly, does this code read past the end of the buffer? Accessing raw_data[0x3d] (index 61) and raw_data[0x3e] (index 62) requires the report size to be at least 63 bytes, but the global check only guarantees 15 bytes. Additionally, could this new path trigger a divide-by-zero panic? While this issue pre-dates this commit, switch2_report_trigger() contains the following calculation: int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); The zero parameter here comes from the device's factory calibration data (ns2->lt_zero and ns2->rt_zero). If a device advertises 232 as its zero calibration point, this division evaluates to 0. Since this patch exposes a new way to call switch2_report_trigger(), might it be worth adding validation to prevent a zero divisor? > + break; > + case NS2_CTLR_TYPE_PRO: [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/20260512200051.2534081-1-vi@endrift.com?part=3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 3/3] HID: nintendo: Add unified report format support 2026-05-14 1:06 ` sashiko-bot @ 2026-05-31 17:57 ` Silvan Jegen 0 siblings, 0 replies; 15+ messages in thread From: Silvan Jegen @ 2026-05-31 17:57 UTC (permalink / raw) To: sashiko-reviews; +Cc: Vicki Pfau, linux-input, dmitry.torokhov sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > - [High] Insufficient bounds checking for NS2_REPORT_UNIFIED payload leads to out-of-bounds reads. > - [High] Divide-by-zero kernel panic in switch2_report_trigger() caused by attacker-controlled calibration data. > -- > > commit 6e45b564fde8014fc6ade8452a1b793097b3bee4 > Author: Vicki Pfau <vi@endrift.com> > Subject: HID: nintendo: Add unified report format support > > This commit adds support for the "unified" report format to the nintendo hid > driver. It introduces new mappings for overlapping button and axis fields > across different controller types. > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > index 9801799a90758..9e7eda7b70bfb 100644 > > --- a/drivers/hid/hid-nintendo.c > > +++ b/drivers/hid/hid-nintendo.c > [ ... ] > > @@ -3690,11 +3791,48 @@ static int switch2_event(struct hid_device *hdev, struct hid_report *report, uin > > > > switch (report->id) { > > case NS2_REPORT_UNIFIED: > > - /* > > - * TODO > > - * This won't be sent unless the report type gets changed via command > > - * 03-0A, but we should support it at some point regardless. > > - */ > > + switch (ns2->ctlr_type) { > > + case NS2_CTLR_TYPE_JCL: > > + switch2_report_stick(input, &ns2->stick_calib[0], > > + ABS_X, false, ABS_Y, true, &raw_data[11]); > > + switch2_report_buttons(input, &raw_data[5], > > + ns2_left_joycon_button_unified_mappings); > > + break; > > + case NS2_CTLR_TYPE_JCR: > > + switch2_report_stick(input, &ns2->stick_calib[0], > > + ABS_X, false, ABS_Y, true, &raw_data[14]); > > Is this safe if the report size is exactly 15 bytes? > > Earlier in switch2_event(), there is a global size check: > > if (size < 15) > return -EINVAL; > > However, switch2_report_stick() reads up to 3 bytes from the provided data > pointer (indices 0, 1, and 2). Passing &raw_data[14] means it will read > raw_data[14], raw_data[15], and raw_data[16]. > > Could this cause an out-of-bounds read if an undersized report is sent? Yes, I assume we would have to check for a bigger size here. > > > + switch2_report_buttons(input, &raw_data[5], > > + ns2_right_joycon_button_unified_mappings); > > + break; > > + case NS2_CTLR_TYPE_GC: > > + input_report_abs(input, ABS_HAT0X, > > + !!(raw_data[7] & NS2_BTN_U3_RIGHT) - > > + !!(raw_data[7] & NS2_BTN_U3_LEFT)); > > + input_report_abs(input, ABS_HAT0Y, > > + !!(raw_data[7] & NS2_BTN_U3_DOWN) - > > + !!(raw_data[7] & NS2_BTN_U3_UP)); > > + switch2_report_buttons(input, &raw_data[5], ns2_gccon_unified_mappings); > > + switch2_report_stick(input, &ns2->stick_calib[0], > > + ABS_X, false, ABS_Y, true, &raw_data[11]); > > + switch2_report_stick(input, &ns2->stick_calib[1], > > + ABS_RX, false, ABS_RY, true, &raw_data[14]); > > + switch2_report_trigger(input, ns2->lt_zero, ABS_Z, raw_data[0x3d]); > > + switch2_report_trigger(input, ns2->rt_zero, ABS_RZ, raw_data[0x3e]); > > Similarly, does this code read past the end of the buffer? > > Accessing raw_data[0x3d] (index 61) and raw_data[0x3e] (index 62) requires > the report size to be at least 63 bytes, but the global check only guarantees > 15 bytes. Same here. > > Additionally, could this new path trigger a divide-by-zero panic? > > While this issue pre-dates this commit, switch2_report_trigger() contains the > following calculation: > > int value = (NS2_TRIGGER_RANGE + 1) * (data - zero) / (232 - zero); > > The zero parameter here comes from the device's factory calibration data > (ns2->lt_zero and ns2->rt_zero). If a device advertises 232 as its zero > calibration point, this division evaluates to 0. > > Since this patch exposes a new way to call switch2_report_trigger(), might it > be worth adding validation to prevent a zero divisor? This one has been reported already in one of the earlier emails. Cheers, Silvan > > > + break; > > + case NS2_CTLR_TYPE_PRO: > [ ... ] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver 2026-05-12 20:00 [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Vicki Pfau ` (2 preceding siblings ...) 2026-05-12 20:00 ` [PATCH v5 3/3] HID: nintendo: Add unified report format support Vicki Pfau @ 2026-06-10 16:18 ` Jiri Kosina 3 siblings, 0 replies; 15+ messages in thread From: Jiri Kosina @ 2026-06-10 16:18 UTC (permalink / raw) To: Vicki Pfau; +Cc: Dmitry Torokhov, Benjamin Tissoires, linux-input On Tue, 12 May 2026, Vicki Pfau wrote: > This series adds preliminary support for Switch 2 controllers using the > same split-driver model as previous versions. This is a minor iteration on > v4, fixing some leaked input_dev objects in error paths as well as fixing a > merge/rebase error when CONFIG_PM ifdefs got removed in 7.0. > > Vicki Pfau (3): > HID: nintendo: Add preliminary Switch 2 controller driver > HID: nintendo: Add rumble support for Switch 2 controllers > HID: nintendo: Add unified report format support > > MAINTAINERS | 1 + > drivers/hid/Kconfig | 19 +- > drivers/hid/hid-ids.h | 4 + > drivers/hid/hid-nintendo.c | 1504 ++++++++++++++++- > drivers/hid/hid-nintendo.h | 72 + > drivers/input/joystick/Kconfig | 11 + > drivers/input/joystick/Makefile | 1 + > drivers/input/joystick/nintendo-switch2-usb.c | 353 ++++ > 8 files changed, 1951 insertions(+), 14 deletions(-) > create mode 100644 drivers/hid/hid-nintendo.h > create mode 100644 drivers/input/joystick/nintendo-switch2-usb.c Dmitry, do you have any review comments to the changes under drivers/input/joystick, please? Thanks, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-06-22 9:53 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-12 20:00 [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Vicki Pfau 2026-05-12 20:00 ` [PATCH v5 1/3] " Vicki Pfau 2026-05-14 0:15 ` sashiko-bot 2026-05-23 8:57 ` Silvan Jegen 2026-06-19 4:02 ` Vicki Pfau 2026-06-19 3:56 ` Vicki Pfau 2026-06-22 9:47 ` [PATCH] HID: nintendo: fix clang build of switch2_probe guard/goto Ryan McClelland 2026-05-12 20:00 ` [PATCH v5 2/3] HID: nintendo: Add rumble support for Switch 2 controllers Vicki Pfau 2026-05-14 0:53 ` sashiko-bot 2026-05-30 20:03 ` Silvan Jegen 2026-06-19 4:17 ` Vicki Pfau 2026-05-12 20:00 ` [PATCH v5 3/3] HID: nintendo: Add unified report format support Vicki Pfau 2026-05-14 1:06 ` sashiko-bot 2026-05-31 17:57 ` Silvan Jegen 2026-06-10 16:18 ` [PATCH v5 0/3] HID: nintendo: Add preliminary Switch 2 controller driver Jiri Kosina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox