| Message ID | 20230906102507.15504-2-tinozzo123@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,RESEND] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend | expand |
On Wed, Sep 6, 2023 at 6:25 AM Martino Fontana <tinozzo123@gmail.com> wrote: > > When suspending the computer, a Switch Pro Controller connected via USB will > lose its internal status. However, because the USB connection was technically > never lost, when resuming the computer, the driver will attempt to communicate > with the controller as if nothing happened (and fail). > Because of this, the user was forced to manually disconnect the controller > (or to press the sync button on the controller to power it off), so that it > can be re-initialized. > > With this patch, the controller will be automatically re-initialized after > resuming from suspend. > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=216233 > > Signed-off-by: Martino Fontana <tinozzo123@gmail.com> Hi Martino, Thanks for the patch. This has been on the backlog for a long time, so it's great to see that USB resume is handled now. Have you tested how this behaves for bluetooth controllers? Does the pm resume hook always result in error logs for bluetooth controllers, or has the kernel already removed the device before resume? > --- > drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++--------------- > 1 file changed, 106 insertions(+), 72 deletions(-) > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 250f5d2f8..a5ebe857a 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c > @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr) > struct joycon_input_report *report; > > req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO; > + mutex_lock(&ctlr->output_mutex); > ret = joycon_send_subcmd(ctlr, &req, 0, HZ); > + mutex_unlock(&ctlr->output_mutex); > if (ret) { > hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret); > return ret; > @@ -2117,6 +2119,88 @@ static int joycon_read_info(struct joycon_ctlr *ctlr) > return 0; > } > > +static int joycon_init(struct hid_device *hdev) > +{ > + struct joycon_ctlr *ctlr = hid_get_drvdata(hdev); > + int ret = 0; > + > + mutex_lock(&ctlr->output_mutex); > + /* if handshake command fails, assume ble pro controller */ > + if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) && > + !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) { > + hid_dbg(hdev, "detected USB controller\n"); > + /* set baudrate for improved latency */ > + ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ); > + if (ret) { > + hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret); > + goto err_mutex; > + } > + /* handshake */ > + ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ); > + if (ret) { > + hid_err(hdev, "Failed handshake; ret=%d\n", ret); > + goto err_mutex; > + } > + /* > + * Set no timeout (to keep controller in USB mode). > + * This doesn't send a response, so ignore the timeout. > + */ > + joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10); > + } else if (jc_type_is_chrggrip(ctlr)) { > + hid_err(hdev, "Failed charging grip handshake\n"); > + ret = -ETIMEDOUT; > + goto err_mutex; > + } > + > + /* get controller calibration data, and parse it */ > + ret = joycon_request_calibration(ctlr); > + if (ret) { > + /* > + * We can function with default calibration, but it may be > + * inaccurate. Provide a warning, and continue on. > + */ > + hid_warn(hdev, "Analog stick positions may be inaccurate\n"); > + } > + > + /* get IMU calibration data, and parse it */ > + ret = joycon_request_imu_calibration(ctlr); > + if (ret) { > + /* > + * We can function with default calibration, but it may be > + * inaccurate. Provide a warning, and continue on. > + */ > + hid_warn(hdev, "Unable to read IMU calibration data\n"); > + } > + > + /* Set the reporting mode to 0x30, which is the full report mode */ > + ret = joycon_set_report_mode(ctlr); > + if (ret) { > + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret); > + goto err_mutex; > + } > + > + /* Enable rumble */ > + ret = joycon_enable_rumble(ctlr); > + if (ret) { > + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret); > + goto err_mutex; > + } > + > + /* Enable the IMU */ > + ret = joycon_enable_imu(ctlr); > + if (ret) { > + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret); > + goto err_mutex; > + } > + > + mutex_unlock(&ctlr->output_mutex); > + return 0; If desired, you could remove the above two lines and allow flow to continue through err_mutex even in the success case. > + > +err_mutex: > + mutex_unlock(&ctlr->output_mutex); > + return ret; > +} > + > /* Common handler for parsing inputs */ > static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data, > int size) > @@ -2248,85 +2332,19 @@ static int nintendo_hid_probe(struct hid_device *hdev, > > hid_device_io_start(hdev); > > - /* Initialize the controller */ > - mutex_lock(&ctlr->output_mutex); > - /* if handshake command fails, assume ble pro controller */ > - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) && > - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) { > - hid_dbg(hdev, "detected USB controller\n"); > - /* set baudrate for improved latency */ > - ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ); > - if (ret) { > - hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret); > - goto err_mutex; > - } > - /* handshake */ > - ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ); > - if (ret) { > - hid_err(hdev, "Failed handshake; ret=%d\n", ret); > - goto err_mutex; > - } > - /* > - * Set no timeout (to keep controller in USB mode). > - * This doesn't send a response, so ignore the timeout. > - */ > - joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10); > - } else if (jc_type_is_chrggrip(ctlr)) { > - hid_err(hdev, "Failed charging grip handshake\n"); > - ret = -ETIMEDOUT; > - goto err_mutex; > - } > - > - /* get controller calibration data, and parse it */ > - ret = joycon_request_calibration(ctlr); > - if (ret) { > - /* > - * We can function with default calibration, but it may be > - * inaccurate. Provide a warning, and continue on. > - */ > - hid_warn(hdev, "Analog stick positions may be inaccurate\n"); > - } > - > - /* get IMU calibration data, and parse it */ > - ret = joycon_request_imu_calibration(ctlr); > - if (ret) { > - /* > - * We can function with default calibration, but it may be > - * inaccurate. Provide a warning, and continue on. > - */ > - hid_warn(hdev, "Unable to read IMU calibration data\n"); > - } > - > - /* Set the reporting mode to 0x30, which is the full report mode */ > - ret = joycon_set_report_mode(ctlr); > - if (ret) { > - hid_err(hdev, "Failed to set report mode; ret=%d\n", ret); > - goto err_mutex; > - } > - > - /* Enable rumble */ > - ret = joycon_enable_rumble(ctlr); > - if (ret) { > - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret); > - goto err_mutex; > - } > - > - /* Enable the IMU */ > - ret = joycon_enable_imu(ctlr); > + ret = joycon_init(hdev); > if (ret) { > - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret); > - goto err_mutex; > + hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret); > + goto err_close; > } > > ret = joycon_read_info(ctlr); > if (ret) { > hid_err(hdev, "Failed to retrieve controller info; ret=%d\n", > ret); > - goto err_mutex; > + goto err_close; > } > > - mutex_unlock(&ctlr->output_mutex); > - > /* Initialize the leds */ > ret = joycon_leds_create(ctlr); > if (ret) { > @@ -2352,8 +2370,6 @@ static int nintendo_hid_probe(struct hid_device *hdev, > hid_dbg(hdev, "probe - success\n"); > return 0; > > -err_mutex: > - mutex_unlock(&ctlr->output_mutex); > err_close: > hid_hw_close(hdev); > err_stop: > @@ -2383,6 +2399,20 @@ static void nintendo_hid_remove(struct hid_device *hdev) > hid_hw_stop(hdev); > } > > +#ifdef CONFIG_PM > + > +static int nintendo_hid_resume(struct hid_device *hdev) > +{ > + int ret = joycon_init(hdev); > + > + if (ret) > + hid_err(hdev, "Failed to restore controller after resume"); > + > + return ret; > +} > + > +#endif > + > static const struct hid_device_id nintendo_hid_devices[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, > USB_DEVICE_ID_NINTENDO_PROCON) }, > @@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = { > .probe = nintendo_hid_probe, > .remove = nintendo_hid_remove, > .raw_event = nintendo_hid_event, > + > +#ifdef CONFIG_PM > + .resume = nintendo_hid_resume, > +#endif Something else we should do is add a suspend hook to power off the bluetooth-connected controllers. As of now, they remain powered on during suspend. > }; > module_hid_driver(nintendo_hid_driver); > > -- > 2.41.0 > Thanks, Daniel
On Fri, 8 Sept 2023 at 03:11, Daniel Ogorchock <djogorchock@gmail.com> wrote: > > Have you tested how this behaves for bluetooth controllers? Does the > pm resume hook always result in error logs for bluetooth controllers, > or has the kernel already removed the device before resume? Tested on a Bluetooth connection, it behaves like it did before: the controller disconnects as the computer sleeps, and you can have to reconnect it by pressing a button on the controller. I also don't see any log from that wasn't there before on journalctl. > > + mutex_unlock(&ctlr->output_mutex); > > + return 0; > > If desired, you could remove the above two lines and allow flow to > continue through err_mutex even in the success case. > > > + > > +err_mutex: > > + mutex_unlock(&ctlr->output_mutex); > > + return ret; > > +} > > + Do I make a patch v3 for that, or is that not necessary? (Asking because this is my first Linux kernel patch) > > static const struct hid_device_id nintendo_hid_devices[] = { > > { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, > > USB_DEVICE_ID_NINTENDO_PROCON) }, > > @@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = { > > .probe = nintendo_hid_probe, > > .remove = nintendo_hid_remove, > > .raw_event = nintendo_hid_event, > > + > > +#ifdef CONFIG_PM > > + .resume = nintendo_hid_resume, > > +#endif > > Something else we should do is add a suspend hook to power off the > bluetooth-connected controllers. As of now, they remain powered on > during suspend. No, they power off when you suspend the computer. If you press a button, the controller will attempt to pair, just like if you disconnected it manually. As I said before, Bluetooth behavior isn't changed. Regards, Martino
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 250f5d2f8..a5ebe857a 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr) struct joycon_input_report *report; req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO; + mutex_lock(&ctlr->output_mutex); ret = joycon_send_subcmd(ctlr, &req, 0, HZ); + mutex_unlock(&ctlr->output_mutex); if (ret) { hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret); return ret; @@ -2117,6 +2119,88 @@ static int joycon_read_info(struct joycon_ctlr *ctlr) return 0; } +static int joycon_init(struct hid_device *hdev) +{ + struct joycon_ctlr *ctlr = hid_get_drvdata(hdev); + int ret = 0; + + mutex_lock(&ctlr->output_mutex); + /* if handshake command fails, assume ble pro controller */ + if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) && + !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) { + hid_dbg(hdev, "detected USB controller\n"); + /* set baudrate for improved latency */ + ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ); + if (ret) { + hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret); + goto err_mutex; + } + /* handshake */ + ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ); + if (ret) { + hid_err(hdev, "Failed handshake; ret=%d\n", ret); + goto err_mutex; + } + /* + * Set no timeout (to keep controller in USB mode). + * This doesn't send a response, so ignore the timeout. + */ + joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10); + } else if (jc_type_is_chrggrip(ctlr)) { + hid_err(hdev, "Failed charging grip handshake\n"); + ret = -ETIMEDOUT; + goto err_mutex; + } + + /* get controller calibration data, and parse it */ + ret = joycon_request_calibration(ctlr); + if (ret) { + /* + * We can function with default calibration, but it may be + * inaccurate. Provide a warning, and continue on. + */ + hid_warn(hdev, "Analog stick positions may be inaccurate\n"); + } + + /* get IMU calibration data, and parse it */ + ret = joycon_request_imu_calibration(ctlr); + if (ret) { + /* + * We can function with default calibration, but it may be + * inaccurate. Provide a warning, and continue on. + */ + hid_warn(hdev, "Unable to read IMU calibration data\n"); + } + + /* Set the reporting mode to 0x30, which is the full report mode */ + ret = joycon_set_report_mode(ctlr); + if (ret) { + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret); + goto err_mutex; + } + + /* Enable rumble */ + ret = joycon_enable_rumble(ctlr); + if (ret) { + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret); + goto err_mutex; + } + + /* Enable the IMU */ + ret = joycon_enable_imu(ctlr); + if (ret) { + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret); + goto err_mutex; + } + + mutex_unlock(&ctlr->output_mutex); + return 0; + +err_mutex: + mutex_unlock(&ctlr->output_mutex); + return ret; +} + /* Common handler for parsing inputs */ static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data, int size) @@ -2248,85 +2332,19 @@ static int nintendo_hid_probe(struct hid_device *hdev, hid_device_io_start(hdev); - /* Initialize the controller */ - mutex_lock(&ctlr->output_mutex); - /* if handshake command fails, assume ble pro controller */ - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) && - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) { - hid_dbg(hdev, "detected USB controller\n"); - /* set baudrate for improved latency */ - ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ); - if (ret) { - hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret); - goto err_mutex; - } - /* handshake */ - ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ); - if (ret) { - hid_err(hdev, "Failed handshake; ret=%d\n", ret); - goto err_mutex; - } - /* - * Set no timeout (to keep controller in USB mode). - * This doesn't send a response, so ignore the timeout. - */ - joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10); - } else if (jc_type_is_chrggrip(ctlr)) { - hid_err(hdev, "Failed charging grip handshake\n"); - ret = -ETIMEDOUT; - goto err_mutex; - } - - /* get controller calibration data, and parse it */ - ret = joycon_request_calibration(ctlr); - if (ret) { - /* - * We can function with default calibration, but it may be - * inaccurate. Provide a warning, and continue on. - */ - hid_warn(hdev, "Analog stick positions may be inaccurate\n"); - } - - /* get IMU calibration data, and parse it */ - ret = joycon_request_imu_calibration(ctlr); - if (ret) { - /* - * We can function with default calibration, but it may be - * inaccurate. Provide a warning, and continue on. - */ - hid_warn(hdev, "Unable to read IMU calibration data\n"); - } - - /* Set the reporting mode to 0x30, which is the full report mode */ - ret = joycon_set_report_mode(ctlr); - if (ret) { - hid_err(hdev, "Failed to set report mode; ret=%d\n", ret); - goto err_mutex; - } - - /* Enable rumble */ - ret = joycon_enable_rumble(ctlr); - if (ret) { - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret); - goto err_mutex; - } - - /* Enable the IMU */ - ret = joycon_enable_imu(ctlr); + ret = joycon_init(hdev); if (ret) { - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret); - goto err_mutex; + hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret); + goto err_close; } ret = joycon_read_info(ctlr); if (ret) { hid_err(hdev, "Failed to retrieve controller info; ret=%d\n", ret); - goto err_mutex; + goto err_close; } - mutex_unlock(&ctlr->output_mutex); - /* Initialize the leds */ ret = joycon_leds_create(ctlr); if (ret) { @@ -2352,8 +2370,6 @@ static int nintendo_hid_probe(struct hid_device *hdev, hid_dbg(hdev, "probe - success\n"); return 0; -err_mutex: - mutex_unlock(&ctlr->output_mutex); err_close: hid_hw_close(hdev); err_stop: @@ -2383,6 +2399,20 @@ static void nintendo_hid_remove(struct hid_device *hdev) hid_hw_stop(hdev); } +#ifdef CONFIG_PM + +static int nintendo_hid_resume(struct hid_device *hdev) +{ + int ret = joycon_init(hdev); + + if (ret) + hid_err(hdev, "Failed to restore controller after resume"); + + return ret; +} + +#endif + static const struct hid_device_id nintendo_hid_devices[] = { { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_PROCON) }, @@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = { .probe = nintendo_hid_probe, .remove = nintendo_hid_remove, .raw_event = nintendo_hid_event, + +#ifdef CONFIG_PM + .resume = nintendo_hid_resume, +#endif }; module_hid_driver(nintendo_hid_driver);
When suspending the computer, a Switch Pro Controller connected via USB will lose its internal status. However, because the USB connection was technically never lost, when resuming the computer, the driver will attempt to communicate with the controller as if nothing happened (and fail). Because of this, the user was forced to manually disconnect the controller (or to press the sync button on the controller to power it off), so that it can be re-initialized. With this patch, the controller will be automatically re-initialized after resuming from suspend. Closes: https://bugzilla.kernel.org/show_bug.cgi?id=216233 Signed-off-by: Martino Fontana <tinozzo123@gmail.com> --- drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++--------------- 1 file changed, 106 insertions(+), 72 deletions(-)