| Message ID | aEZxmlHmjeWcXiF3@dragon |
|---|---|
| State | New |
| Headers | show |
| Series | i.MX kernel hangup caused by chipidea USB gadget driver | expand |
On Mon, Jun 09, 2025 at 07:53:22PM +0800, Xu Yang wrote: > Hi Shawn, > > Thanks for your reports! > > On Mon, Jun 09, 2025 at 01:31:06PM +0800, Shawn Guo wrote: > > Hi Xu, Peter, > > > > I'm seeing a kernel hangup on imx8mm-evk board. It happens when: > > > > - USB gadget is enabled as Ethernet > > - There is data transfer over USB Ethernet > > - Device is going in/out suspend > During the scp process, the usb host won't put usb device to suspend state. > In current design, then the ether driver doesn't know the system has > suspended after echo mem. The root cause is that ether driver is still tring > to queue usb request after usb controller has suspended where usb clock is off, > then the system hang. > > With the above changes, I think the ether driver will fail to eth_start_xmit() > at an ealier stage, so the issue can't be triggered. > > I think the ether driver needs call gether_suspend() accordingly, to do this, > the controller driver need explicitly call suspend() function when it's going > to be suspended. Could you check whether below patch fix the issue? The situation is more complicated than this. In general, a USB gadget cannot allow itself to be suspended while the USB bus it is connected to remains active. Not unless it can be set to wake up when a USB packet arrives, and even that probably won't work because the wakeup sequence would take too long and the USB transfer would time out on the host. The best way to fix this problem is for the gadget to disconnect itself from the USB bus whenever it goes into suspend, and to reconnect when it resumes. Alan Stern
Hi Shawn, Xu, On Mon, Jun 09, 2025 at 07:53:22PM +0800, Xu Yang wrote: > Hi Shawn, > > Thanks for your reports! > > On Mon, Jun 09, 2025 at 01:31:06PM +0800, Shawn Guo wrote: > > Hi Xu, Peter, > > > > I'm seeing a kernel hangup on imx8mm-evk board. It happens when: > > > > - USB gadget is enabled as Ethernet > > - There is data transfer over USB Ethernet > > - Device is going in/out suspend > > > > A simple way to reproduce the issue could be: > > > > 1. Copy a big file (like 500MB) from host PC to device with scp > > > > 2. While the file copy is ongoing, suspend & resume the device like: > > > > $ echo +3 > /sys/class/rtc/rtc0/wakealarm; echo mem > /sys/power/state > > > > 3. The device will hang up there > > > > I reproduced on the following kernels: > > > > - Mainline kernel > > - NXP kernel lf-6.6.y > > - NXP kernel lf-6.12.y > > > > But NXP kernel lf-6.1.y doesn't have this problem. I tracked it down to > > Peter's commit [1] on lf-6.1.y, and found that the gadget disconnect & > > connect calls got lost from suspend & resume hooks, when the commit were > > split and pushed upstream. I confirm that adding the calls back fixes > > the hangup. We probably ran into the same problem trying to bring onboard 6.12, going from 6.1, on iMX8QXP. I managed to trace the hang to EP priming through a combination of debug tracing and BUG_ON experiments. See if it starts splatin with the below change. ----------------->8------------------
Hi John, On Mon, Jun 09, 2025 at 02:17:30PM +0000, John Ernberg wrote: <snip> > We probably ran into the same problem trying to bring onboard 6.12, going > from 6.1, on iMX8QXP. I managed to trace the hang to EP priming through a > combination of debug tracing and BUG_ON experiments. See if it starts > splatin with the below change. > > ----------------->8------------------ > > From 092599ab6f9e20412a7ca1eb118dd2be80cd18ff Mon Sep 17 00:00:00 2001 > From: John Ernberg <john.ernberg@actia.se> > Date: Mon, 5 May 2025 09:09:01 +0200 > Subject: [PATCH] USB: ci: gadget: Panic if priming when gadget off > > --- > drivers/usb/chipidea/udc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 2fea263a5e30..544aa4fa2d1d 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -203,8 +203,10 @@ static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl) > > hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n)); > > - while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) > + while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) { > cpu_relax(); > + BUG_ON(dir == TX && !hw_read(ci, OP_ENDPTCTRL + num, ENDPTCTRL_TXE)); > + } > if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) > return -EAGAIN; > > ----------------->8------------------ Hmm, I just tested the change on i.MX8MM but didn't see the splatting. Maybe we are running into a slightly different problems? Shawn
Hi Alan, On Mon, Jun 09, 2025 at 09:54:45AM -0400, Alan Stern wrote: > On Mon, Jun 09, 2025 at 07:53:22PM +0800, Xu Yang wrote: > > Hi Shawn, > > > > Thanks for your reports! > > > > On Mon, Jun 09, 2025 at 01:31:06PM +0800, Shawn Guo wrote: > > > Hi Xu, Peter, > > > > > > I'm seeing a kernel hangup on imx8mm-evk board. It happens when: > > > > > > - USB gadget is enabled as Ethernet > > > - There is data transfer over USB Ethernet > > > - Device is going in/out suspend > > > During the scp process, the usb host won't put usb device to suspend state. > > In current design, then the ether driver doesn't know the system has > > suspended after echo mem. The root cause is that ether driver is still tring > > to queue usb request after usb controller has suspended where usb clock is off, > > then the system hang. > > > > With the above changes, I think the ether driver will fail to eth_start_xmit() > > at an ealier stage, so the issue can't be triggered. > > > > I think the ether driver needs call gether_suspend() accordingly, to do this, > > the controller driver need explicitly call suspend() function when it's going > > to be suspended. Could you check whether below patch fix the issue? > > The situation is more complicated than this. > > In general, a USB gadget cannot allow itself to be suspended while the > USB bus it is connected to remains active. Not unless it can be set to > wake up when a USB packet arrives, and even that probably won't work > because the wakeup sequence would take too long and the USB transfer > would time out on the host. > > The best way to fix this problem is for the gadget to disconnect itself > from the USB bus whenever it goes into suspend, and to reconnect when it > resumes. Thank you so much for the insight! It matches my testing pretty well. The disconnect/reconnect gadget in udc suspend/resume is more reliable than suspend/resume gadget. Shawn
On Tue, Jun 10, 2025 at 10:12:43AM +0800, Peter Chen (CIX) wrote: > Like Alan and Xu's suggestion, there are probably two problems here: > - When the system enters the suspend, the USB bus may neither at suspend > nor disconnect state if USB controller/phy's power is not off and VBUS > is there. So, the host still considers the device is active, it could > trigger transfer any time. If the transfer occurs during system resume, > the USB controller triggers interrupt to CPU, and USB's interrupt handler > is triggered. If the USB's hardware is still at low power mode (or clock > is gated off), it may cause system hang (CPU gets error response from USB) > after access register. > > With Shawn's change, it pulls D+ down during the suspend, and the host > is notified of disconnection, so the host will not trigger transfer > until D+ is pulled up by calling usb_gadget_connect. The USB leaves > low power mode (and turn clock on) before that, the access register > will not cause system hang. Thanks for the input, Peter! It's very helpful and well explaining what I'm seeing here. > - The current chipidea driver doesn't notify gadget driver when it > enters system suspend routine. In fact, during system suspend/resume, > the controller driver may not respond middle layer's (network) request > well due to it enters low power mode, so calling usb_gadget_driver-> > disconnect (composite_disconnect) is needed during controller's suspend > routine, it calls function->disable for USB function driver and > ends/stop middle layer process. This problem can also be addressed by Xu's suggestion, i.e. calling gadget driver's suspend hook from udc_suspend()? Shawn
Hi Alan, On Mon, Jun 09, 2025 at 09:54:45AM -0400, Alan Stern wrote: > On Mon, Jun 09, 2025 at 07:53:22PM +0800, Xu Yang wrote: > > Hi Shawn, > > > > Thanks for your reports! > > > > On Mon, Jun 09, 2025 at 01:31:06PM +0800, Shawn Guo wrote: > > > Hi Xu, Peter, > > > > > > I'm seeing a kernel hangup on imx8mm-evk board. It happens when: > > > > > > - USB gadget is enabled as Ethernet > > > - There is data transfer over USB Ethernet > > > - Device is going in/out suspend > > > During the scp process, the usb host won't put usb device to suspend state. > > In current design, then the ether driver doesn't know the system has > > suspended after echo mem. The root cause is that ether driver is still tring > > to queue usb request after usb controller has suspended where usb clock is off, > > then the system hang. > > > > With the above changes, I think the ether driver will fail to eth_start_xmit() > > at an ealier stage, so the issue can't be triggered. > > > > I think the ether driver needs call gether_suspend() accordingly, to do this, > > the controller driver need explicitly call suspend() function when it's going > > to be suspended. Could you check whether below patch fix the issue? > > The situation is more complicated than this. > > In general, a USB gadget cannot allow itself to be suspended while the > USB bus it is connected to remains active. Not unless it can be set to > wake up when a USB packet arrives, and even that probably won't work > because the wakeup sequence would take too long and the USB transfer > would time out on the host. Agree with you. I always see such wakeup issue on some customer usecase. Then I need to improve the system wakeup time to satisfy usb requirements. > > The best way to fix this problem is for the gadget to disconnect itself > from the USB bus whenever it goes into suspend, and to reconnect when it > resumes. Okay, thanks for your suggestion! This should be the best solution. I will do this when wakeup is not needed and will also take care of wakeup case due to some user indeed need it. Thanks, Xu Yang > > Alan Stern
On Tue, Jun 10, 2025 at 10:12:43AM +0800, Peter Chen (CIX) wrote: > On 25-06-09 14:17:30, John Ernberg wrote: > > Hi Shawn, Xu, > > > > On Mon, Jun 09, 2025 at 07:53:22PM +0800, Xu Yang wrote: > > > Hi Shawn, > > > > > > Thanks for your reports! > > > > > > On Mon, Jun 09, 2025 at 01:31:06PM +0800, Shawn Guo wrote: > > > > Hi Xu, Peter, > > > > > > > > I'm seeing a kernel hangup on imx8mm-evk board. It happens when: > > > > > > > > - USB gadget is enabled as Ethernet > > > > - There is data transfer over USB Ethernet > > > > - Device is going in/out suspend > > > > > > > > A simple way to reproduce the issue could be: > > > > > > > > 1. Copy a big file (like 500MB) from host PC to device with scp > > > > > > > > 2. While the file copy is ongoing, suspend & resume the device like: > > > > > > > > $ echo +3 > /sys/class/rtc/rtc0/wakealarm; echo mem > /sys/power/state > > > > > > > > 3. The device will hang up there > > > > > > > > I reproduced on the following kernels: > > > > > > > > - Mainline kernel > > > > - NXP kernel lf-6.6.y > > > > - NXP kernel lf-6.12.y > > > > > > > > But NXP kernel lf-6.1.y doesn't have this problem. I tracked it down to > > > > Peter's commit [1] on lf-6.1.y, and found that the gadget disconnect & > > > > connect calls got lost from suspend & resume hooks, when the commit were > > > > split and pushed upstream. I confirm that adding the calls back fixes > > > > the hangup. > > > > We probably ran into the same problem trying to bring onboard 6.12, going > > from 6.1, on iMX8QXP. I managed to trace the hang to EP priming through a > > combination of debug tracing and BUG_ON experiments. See if it starts > > splatin with the below change. > > Hi John and Shawn, > > Like Alan and Xu's suggestion, there are probably two problems here: > - When the system enters the suspend, the USB bus may neither at suspend > nor disconnect state if USB controller/phy's power is not off and VBUS > is there. So, the host still considers the device is active, it could > trigger transfer any time. If the transfer occurs during system resume, > the USB controller triggers interrupt to CPU, and USB's interrupt handler > is triggered. If the USB's hardware is still at low power mode (or clock > is gated off), it may cause system hang (CPU gets error response from USB) > after access register. > > With Shawn's change, it pulls D+ down during the suspend, and the host > is notified of disconnection, so the host will not trigger transfer > until D+ is pulled up by calling usb_gadget_connect. The USB leaves > low power mode (and turn clock on) before that, the access register > will not cause system hang. > > - The current chipidea driver doesn't notify gadget driver when it > enters system suspend routine. In fact, during system suspend/resume, > the controller driver may not respond middle layer's (network) request > well due to it enters low power mode, so calling usb_gadget_driver-> > disconnect (composite_disconnect) is needed during controller's suspend > routine, it calls function->disable for USB function driver and > ends/stop middle layer process. Thank you Peter! I'll improve this part later. Best Regards, Xu Yang > > Peter > > > > > ----------------->8------------------ > > > > From 092599ab6f9e20412a7ca1eb118dd2be80cd18ff Mon Sep 17 00:00:00 2001 > > From: John Ernberg <john.ernberg@actia.se> > > Date: Mon, 5 May 2025 09:09:01 +0200 > > Subject: [PATCH] USB: ci: gadget: Panic if priming when gadget off > > > > --- > > drivers/usb/chipidea/udc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > index 2fea263a5e30..544aa4fa2d1d 100644 > > --- a/drivers/usb/chipidea/udc.c > > +++ b/drivers/usb/chipidea/udc.c > > @@ -203,8 +203,10 @@ static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl) > > > > hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n)); > > > > - while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) > > + while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) { > > cpu_relax(); > > + BUG_ON(dir == TX && !hw_read(ci, OP_ENDPTCTRL + num, ENDPTCTRL_TXE)); > > + } > > if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) > > return -EAGAIN; > > > > ----------------->8------------------ > > > > On the iMX8QXP you may additionally run into asychronous aborts and SError > > due to resource being disabled. > > > > > > > > > > ---8<-------------------- > > > > > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > > index 8a9b31fd5c89..72329a7eac4d 100644 > > > > --- a/drivers/usb/chipidea/udc.c > > > > +++ b/drivers/usb/chipidea/udc.c > > > > @@ -2374,6 +2374,9 @@ static void udc_suspend(struct ci_hdrc *ci) > > > > */ > > > > if (hw_read(ci, OP_ENDPTLISTADDR, ~0) == 0) > > > > hw_write(ci, OP_ENDPTLISTADDR, ~0, ~0); > > > > + > > > > + if (ci->driver && ci->vbus_active && (ci->gadget.state != USB_STATE_SUSPENDED)) > > > > + usb_gadget_disconnect(&ci->gadget); > > > > } > > > > > > > > static void udc_resume(struct ci_hdrc *ci, bool power_lost) > > > > @@ -2384,6 +2387,9 @@ static void udc_resume(struct ci_hdrc *ci, bool power_lost) > > > > OTGSC_BSVIS | OTGSC_BSVIE); > > > > if (ci->vbus_active) > > > > usb_gadget_vbus_disconnect(&ci->gadget); > > > > + } else { > > > > + if (ci->driver && ci->vbus_active) > > > > + usb_gadget_connect(&ci->gadget); > > > > } > > > > > > > > /* Restore value 0 if it was set for power lost check */ > > > > > > > > ---->8------------------ > > > > > > During the scp process, the usb host won't put usb device to suspend state. > > > In current design, then the ether driver doesn't know the system has > > > suspended after echo mem. The root cause is that ether driver is still tring > > > to queue usb request after usb controller has suspended where usb clock is off, > > > then the system hang. > > > > > > With the above changes, I think the ether driver will fail to eth_start_xmit() > > > at an ealier stage, so the issue can't be triggered. > > > > > > I think the ether driver needs call gether_suspend() accordingly, to do this, > > > the controller driver need explicitly call suspend() function when it's going > > > to be suspended. Could you check whether below patch fix the issue? > > > > > > ---8<-------------------- > > > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > index 8a9b31fd5c89..27a7674ed62c 100644 > > > --- a/drivers/usb/chipidea/udc.c > > > +++ b/drivers/usb/chipidea/udc.c > > > @@ -2367,6 +2367,8 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci) > > > #ifdef CONFIG_PM_SLEEP > > > static void udc_suspend(struct ci_hdrc *ci) > > > { > > > + ci->driver->suspend(&ci->gadget); > > > + > > > /* > > > * Set OP_ENDPTLISTADDR to be non-zero for > > > * checking if controller resume from power lost > > > @@ -2389,6 +2391,8 @@ static void udc_resume(struct ci_hdrc *ci, bool power_lost) > > > /* Restore value 0 if it was set for power lost check */ > > > if (hw_read(ci, OP_ENDPTLISTADDR, ~0) == 0xFFFFFFFF) > > > hw_write(ci, OP_ENDPTLISTADDR, ~0, 0); > > > + > > > + ci->driver->resume(&ci->gadget); > > > } > > > #endif > > > > > > ---->8------------------ > > > > I tested this during my debugging and it doesn't work because suspend/resume > > callbacks on the gadgets are designed for USB triggered suspend/resume and > > not system triggered suspend/resume. Meaning that the link will just be > > woken up again by the next USB transfer. > > > > > > > > Thanks, > > > Xu Yang > > > > > > > > > > > But it's unclear to me why the hangup happens and how the change above > > > > fix the problem. Do you guys have any insight here?o > > > > > > > > Shawn > > > > > > > > [1] https://github.com/reMarkable/linux-imx/commit/0791d25578cb0e46fd93ae7a3c36ff7a424f3547 > > > > > > > > I didn't find the missing lines of code that Shawn found and instead ended > > up looking at why the UDC core isn't suspending the gadgets when the system > > is going to suspend. Because to me it feels like a job of UDC core. > > > > I ended up with the monstrosity below that I have been intended to send as > > an RFC when I'm done thinking about it. It currently applies on 6.12.20. > > > > But since Shawn also ran into the problem I'm including it for the sake of > > discussion about what the correct path of solving it is. > > > > Best regards // John Ernberg > > > > ----------------->8------------------ > > > > From 3c1d167f1eff0bd010b797530e3d03f6939db322 Mon Sep 17 00:00:00 2001 > > From: John Ernberg <john.ernberg@actia.se> > > Date: Mon, 5 May 2025 09:09:50 +0200 > > Subject: [PATCH] WIP: Suspend getherlink on system suspend > > > > --- > > drivers/usb/gadget/composite.c | 68 +++++++++++++++++++++++++++ > > drivers/usb/gadget/configfs.c | 53 +++++++++++++++++++++ > > drivers/usb/gadget/function/f_ecm.c | 22 +++++++++ > > drivers/usb/gadget/function/u_ether.c | 34 ++++++++++++++ > > drivers/usb/gadget/function/u_ether.h | 2 + > > drivers/usb/gadget/udc/core.c | 29 ++++++++++++ > > include/linux/usb/composite.h | 4 ++ > > include/linux/usb/gadget.h | 2 + > > 8 files changed, 214 insertions(+) > > > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > > index 8402a86176f4..f1ed1db1e1d0 100644 > > --- a/drivers/usb/gadget/composite.c > > +++ b/drivers/usb/gadget/composite.c > > @@ -2669,6 +2669,72 @@ void composite_resume(struct usb_gadget *gadget) > > cdev->suspended = 0; > > } > > > > +int composite_system_suspend(struct usb_gadget *gadget) > > +{ > > + struct usb_composite_dev *cdev = get_gadget_data(gadget); > > + struct usb_function *f; > > + int ret; > > + > > + DBG(cdev, "system suspend\n"); > > + if (cdev->config) { > > + list_for_each_entry(f, &cdev->config->functions, list) { > > + if (f->system_suspend) { > > + ret = f->system_suspend(f); > > + if (ret) > > + return ret; > > + } > > + } > > + } > > + > > + if (cdev->config && > > + cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER) > > + usb_gadget_set_selfpowered(gadget); > > + > > + usb_gadget_vbus_draw(gadget, 2); > > + > > + return 0; > > +} > > + > > +int composite_system_resume(struct usb_gadget *gadget) > > +{ > > + struct usb_composite_dev *cdev = get_gadget_data(gadget); > > + struct usb_function *f; > > + unsigned maxpower; > > + int ret; > > + > > + DBG(cdev, "system resume\n"); > > + if (cdev->config) { > > + list_for_each_entry(f, &cdev->config->functions, list) { > > + if (f->system_resume) { > > + ret = f->system_resume(f); > > + if (ret) > > + return ret; > > + } > > + } > > + > > + maxpower = cdev->config->MaxPower ? > > + cdev->config->MaxPower : CONFIG_USB_GADGET_VBUS_DRAW; > > + if (gadget->speed < USB_SPEED_SUPER) > > + maxpower = min(maxpower, 500U); > > + else > > + maxpower = min(maxpower, 900U); > > + > > + if (maxpower > USB_SELF_POWER_VBUS_MAX_DRAW || > > + !(cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER)) > > + usb_gadget_clear_selfpowered(gadget); > > + else > > + usb_gadget_set_selfpowered(gadget); > > + > > + usb_gadget_vbus_draw(gadget, maxpower); > > + } else { > > + maxpower = CONFIG_USB_GADGET_VBUS_DRAW; > > + maxpower = min(maxpower, 100U); > > + usb_gadget_vbus_draw(gadget, maxpower); > > + } > > + > > + return 0; > > +} > > + > > /*-------------------------------------------------------------------------*/ > > > > static const struct usb_gadget_driver composite_driver_template = { > > @@ -2681,6 +2747,8 @@ static const struct usb_gadget_driver composite_driver_template = { > > > > .suspend = composite_suspend, > > .resume = composite_resume, > > + .system_suspend = composite_system_suspend, > > + .system_resume = composite_system_resume, > > > > .driver = { > > .owner = THIS_MODULE, > > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c > > index 29390d573e23..e0d2f0998e86 100644 > > --- a/drivers/usb/gadget/configfs.c > > +++ b/drivers/usb/gadget/configfs.c > > @@ -1962,6 +1962,57 @@ static void configfs_composite_resume(struct usb_gadget *gadget) > > spin_unlock_irqrestore(&gi->spinlock, flags); > > } > > > > +static int configfs_composite_system_suspend(struct usb_gadget *gadget) > > +{ > > + struct usb_composite_dev *cdev; > > + struct gadget_info *gi; > > + unsigned long flags; > > + int ret; > > + > > + cdev = get_gadget_data(gadget); > > + if (!cdev) > > + return 0; > > + > > + gi = container_of(cdev, struct gadget_info, cdev); > > + spin_lock_irqsave(&gi->spinlock, flags); > > + cdev = get_gadget_data(gadget); > > + if (!cdev || gi->unbind) { > > + spin_unlock_irqrestore(&gi->spinlock, flags); > > + return 0; > > + } > > + > > + ret = composite_system_suspend(gadget); > > + spin_unlock_irqrestore(&gi->spinlock, flags); > > + > > + return ret; > > +} > > + > > +static int configfs_composite_system_resume(struct usb_gadget *gadget) > > +{ > > + struct usb_composite_dev *cdev; > > + struct gadget_info *gi; > > + unsigned long flags; > > + int ret; > > + > > + cdev = get_gadget_data(gadget); > > + if (!cdev) > > + return 0; > > + > > + gi = container_of(cdev, struct gadget_info, cdev); > > + spin_lock_irqsave(&gi->spinlock, flags); > > + cdev = get_gadget_data(gadget); > > + if (!cdev || gi->unbind) { > > + spin_unlock_irqrestore(&gi->spinlock, flags); > > + return 0; > > + } > > + > > + ret = composite_system_resume(gadget); > > + spin_unlock_irqrestore(&gi->spinlock, flags); > > + > > + return ret; > > +} > > + > > + > > static const struct usb_gadget_driver configfs_driver_template = { > > .bind = configfs_composite_bind, > > .unbind = configfs_composite_unbind, > > @@ -1972,6 +2023,8 @@ static const struct usb_gadget_driver configfs_driver_template = { > > > > .suspend = configfs_composite_suspend, > > .resume = configfs_composite_resume, > > + .system_suspend = configfs_composite_system_suspend, > > + .system_resume = configfs_composite_system_resume, > > > > .max_speed = USB_SPEED_SUPER_PLUS, > > .driver = { > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c > > index 6cb7771e8a69..4df67d5ee0fa 100644 > > --- a/drivers/usb/gadget/function/f_ecm.c > > +++ b/drivers/usb/gadget/function/f_ecm.c > > @@ -892,6 +892,26 @@ static void ecm_resume(struct usb_function *f) > > gether_resume(&ecm->port); > > } > > > > +static int ecm_system_suspend(struct usb_function *f) > > +{ > > + struct f_ecm *ecm = func_to_ecm(f); > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; > > + > > + DBG(cdev, "ECM System Suspend\n"); > > + > > + return gether_system_suspend(&ecm->port); > > +} > > + > > +static int ecm_system_resume(struct usb_function *f) > > +{ > > + struct f_ecm *ecm = func_to_ecm(f); > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; > > + > > + DBG(cdev, "ECM System Resume\n"); > > + > > + return gether_system_resume(&ecm->port); > > +} > > + > > static void ecm_free(struct usb_function *f) > > { > > struct f_ecm *ecm; > > @@ -961,6 +981,8 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi) > > ecm->port.func.free_func = ecm_free; > > ecm->port.func.suspend = ecm_suspend; > > ecm->port.func.resume = ecm_resume; > > + ecm->port.func.system_suspend = ecm_system_suspend; > > + ecm->port.func.system_resume = ecm_system_resume; > > > > return &ecm->port.func; > > } > > diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c > > index f58590bf5e02..d4f0e28ffd4d 100644 > > --- a/drivers/usb/gadget/function/u_ether.c > > +++ b/drivers/usb/gadget/function/u_ether.c > > @@ -1078,6 +1078,40 @@ void gether_resume(struct gether *link) > > } > > EXPORT_SYMBOL_GPL(gether_resume); > > > > +int gether_system_suspend(struct gether *link) > > +{ > > + struct eth_dev *dev = link->ioport; > > + struct net_device *ndev = dev->net; > > + > > + rtnl_lock(); > > + if (netif_running(ndev)) { > > + netif_tx_lock_bh(ndev); > > + netif_device_detach(ndev); > > + netif_tx_unlock_bh(ndev); > > + } > > + rtnl_unlock(); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(gether_system_suspend); > > + > > +int gether_system_resume(struct gether *link) > > +{ > > + struct eth_dev *dev = link->ioport; > > + struct net_device *ndev = dev->net; > > + > > + rtnl_lock(); > > + if (netif_running(ndev)) { > > + netif_tx_lock_bh(ndev); > > + netif_device_attach(ndev); > > + netif_tx_unlock_bh(ndev); > > + } > > + rtnl_unlock(); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(gether_system_resume); > > + > > /* > > * gether_cleanup - remove Ethernet-over-USB device > > * Context: may sleep > > diff --git a/drivers/usb/gadget/function/u_ether.h b/drivers/usb/gadget/function/u_ether.h > > index 34be220cef77..ffd023b7be7b 100644 > > --- a/drivers/usb/gadget/function/u_ether.h > > +++ b/drivers/usb/gadget/function/u_ether.h > > @@ -261,6 +261,8 @@ void gether_cleanup(struct eth_dev *dev); > > > > void gether_suspend(struct gether *link); > > void gether_resume(struct gether *link); > > +int gether_system_suspend(struct gether *link); > > +int gether_system_resume(struct gether *link); > > > > /* connect/disconnect is handled by individual functions */ > > struct net_device *gether_connect(struct gether *); > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index 4b3d5075621a..1e4ee5ffcfbf 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -1683,6 +1683,30 @@ static void gadget_unbind_driver(struct device *dev) > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > } > > > > +static int gadget_suspend_driver(struct device *dev) > > +{ > > + struct usb_gadget *gadget = dev_to_usb_gadget(dev); > > + struct usb_udc *udc = gadget->udc; > > + struct usb_gadget_driver *driver = udc->driver; > > + > > + if (driver->system_suspend) > > + return driver->system_suspend(gadget); > > + > > + return 0; > > +} > > + > > +static int gadget_resume_driver(struct device *dev) > > +{ > > + struct usb_gadget *gadget = dev_to_usb_gadget(dev); > > + struct usb_udc *udc = gadget->udc; > > + struct usb_gadget_driver *driver = udc->driver; > > + > > + if (driver->system_resume) > > + return driver->system_resume(gadget); > > + > > + return 0; > > +} > > + > > /* ------------------------------------------------------------------------- */ > > > > int usb_gadget_register_driver_owner(struct usb_gadget_driver *driver, > > @@ -1896,11 +1920,16 @@ static const struct class udc_class = { > > .dev_uevent = usb_udc_uevent, > > }; > > > > +static const struct dev_pm_ops gadget_bus_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(gadget_suspend_driver, gadget_resume_driver) > > +}; > > + > > static const struct bus_type gadget_bus_type = { > > .name = "gadget", > > .probe = gadget_bind_driver, > > .remove = gadget_unbind_driver, > > .match = gadget_match_driver, > > + .pm = &gadget_bus_pm_ops, > > }; > > > > static int __init usb_udc_init(void) > > diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h > > index 6e38fb9d2117..f42ba1cfd181 100644 > > --- a/include/linux/usb/composite.h > > +++ b/include/linux/usb/composite.h > > @@ -226,6 +226,8 @@ struct usb_function { > > bool config0); > > void (*suspend)(struct usb_function *); > > void (*resume)(struct usb_function *); > > + int (*system_suspend)(struct usb_function *); > > + int (*system_resume)(struct usb_function *); > > > > /* USB 3.0 additions */ > > int (*get_status)(struct usb_function *); > > @@ -522,6 +524,8 @@ extern int composite_setup(struct usb_gadget *gadget, > > const struct usb_ctrlrequest *ctrl); > > extern void composite_suspend(struct usb_gadget *gadget); > > extern void composite_resume(struct usb_gadget *gadget); > > +extern int composite_system_suspend(struct usb_gadget *gadget); > > +extern int composite_system_resume(struct usb_gadget *gadget); > > > > /* > > * Some systems will need runtime overrides for the product identifiers > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > > index df33333650a0..8cdfdece1561 100644 > > --- a/include/linux/usb/gadget.h > > +++ b/include/linux/usb/gadget.h > > @@ -744,6 +744,8 @@ struct usb_gadget_driver { > > void (*disconnect)(struct usb_gadget *); > > void (*suspend)(struct usb_gadget *); > > void (*resume)(struct usb_gadget *); > > + int (*system_suspend)(struct usb_gadget *); > > + int (*system_resume)(struct usb_gadget *); > > void (*reset)(struct usb_gadget *); > > > > /* FIXME support safe rmmod */ > > -- > > Best regards, > Peter
Hi Xu, On 6/10/25 1:30 PM, Xu Yang wrote: > Hi John, > > On Mon, Jun 09, 2025 at 02:17:30PM +0000, John Ernberg wrote: >> Hi Shawn, Xu, >> >> On Mon, Jun 09, 2025 at 07:53:22PM +0800, Xu Yang wrote: >>> Hi Shawn, >>> >>> Thanks for your reports! >>> >>> On Mon, Jun 09, 2025 at 01:31:06PM +0800, Shawn Guo wrote: >>>> Hi Xu, Peter, >>>> >>>> I'm seeing a kernel hangup on imx8mm-evk board. It happens when: >>>> >>>> - USB gadget is enabled as Ethernet >>>> - There is data transfer over USB Ethernet >>>> - Device is going in/out suspend >>>> >>>> A simple way to reproduce the issue could be: >>>> >>>> 1. Copy a big file (like 500MB) from host PC to device with scp >>>> >>>> 2. While the file copy is ongoing, suspend & resume the device like: >>>> >>>> $ echo +3 > /sys/class/rtc/rtc0/wakealarm; echo mem > /sys/power/state >>>> >>>> 3. The device will hang up there >>>> >>>> I reproduced on the following kernels: >>>> >>>> - Mainline kernel >>>> - NXP kernel lf-6.6.y >>>> - NXP kernel lf-6.12.y >>>> >>>> But NXP kernel lf-6.1.y doesn't have this problem. I tracked it down to >>>> Peter's commit [1] on lf-6.1.y, and found that the gadget disconnect & >>>> connect calls got lost from suspend & resume hooks, when the commit were >>>> split and pushed upstream. I confirm that adding the calls back fixes >>>> the hangup. >> >> We probably ran into the same problem trying to bring onboard 6.12, going >> from 6.1, on iMX8QXP. I managed to trace the hang to EP priming through a >> combination of debug tracing and BUG_ON experiments. See if it starts >> splatin with the below change. >> >> ----------------->8------------------ >> >> >From 092599ab6f9e20412a7ca1eb118dd2be80cd18ff Mon Sep 17 00:00:00 2001 >> From: John Ernberg <john.ernberg@actia.se> >> Date: Mon, 5 May 2025 09:09:01 +0200 >> Subject: [PATCH] USB: ci: gadget: Panic if priming when gadget off >> >> --- >> drivers/usb/chipidea/udc.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> index 2fea263a5e30..544aa4fa2d1d 100644 >> --- a/drivers/usb/chipidea/udc.c >> +++ b/drivers/usb/chipidea/udc.c >> @@ -203,8 +203,10 @@ static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl) >> >> hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n)); >> >> - while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) >> + while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) { >> cpu_relax(); >> + BUG_ON(dir == TX && !hw_read(ci, OP_ENDPTCTRL + num, ENDPTCTRL_TXE)); >> + } >> if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) >> return -EAGAIN; >> >> ----------------->8------------------ >> >> On the iMX8QXP you may additionally run into asychronous aborts and SError >> due to resource being disabled. >> >>>> >>>> ---8<-------------------- >>>> >>>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >>>> index 8a9b31fd5c89..72329a7eac4d 100644 >>>> --- a/drivers/usb/chipidea/udc.c >>>> +++ b/drivers/usb/chipidea/udc.c >>>> @@ -2374,6 +2374,9 @@ static void udc_suspend(struct ci_hdrc *ci) >>>> */ >>>> if (hw_read(ci, OP_ENDPTLISTADDR, ~0) == 0) >>>> hw_write(ci, OP_ENDPTLISTADDR, ~0, ~0); >>>> + >>>> + if (ci->driver && ci->vbus_active && (ci->gadget.state != USB_STATE_SUSPENDED)) >>>> + usb_gadget_disconnect(&ci->gadget); >>>> } >>>> >>>> static void udc_resume(struct ci_hdrc *ci, bool power_lost) >>>> @@ -2384,6 +2387,9 @@ static void udc_resume(struct ci_hdrc *ci, bool power_lost) >>>> OTGSC_BSVIS | OTGSC_BSVIE); >>>> if (ci->vbus_active) >>>> usb_gadget_vbus_disconnect(&ci->gadget); >>>> + } else { >>>> + if (ci->driver && ci->vbus_active) >>>> + usb_gadget_connect(&ci->gadget); >>>> } >>>> >>>> /* Restore value 0 if it was set for power lost check */ >>>> >>>> ---->8------------------ > > Does above change work for you? I hope to allocate some time to test this in the next few days. Best regards // John Ernberg
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 8a9b31fd5c89..72329a7eac4d 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -2374,6 +2374,9 @@ static void udc_suspend(struct ci_hdrc *ci) */ if (hw_read(ci, OP_ENDPTLISTADDR, ~0) == 0) hw_write(ci, OP_ENDPTLISTADDR, ~0, ~0); + + if (ci->driver && ci->vbus_active && (ci->gadget.state != USB_STATE_SUSPENDED)) + usb_gadget_disconnect(&ci->gadget); } static void udc_resume(struct ci_hdrc *ci, bool power_lost) @@ -2384,6 +2387,9 @@ static void udc_resume(struct ci_hdrc *ci, bool power_lost) OTGSC_BSVIS | OTGSC_BSVIE); if (ci->vbus_active) usb_gadget_vbus_disconnect(&ci->gadget); + } else { + if (ci->driver && ci->vbus_active) + usb_gadget_connect(&ci->gadget); } /* Restore value 0 if it was set for power lost check */
Hi Xu, Peter, I'm seeing a kernel hangup on imx8mm-evk board. It happens when: - USB gadget is enabled as Ethernet - There is data transfer over USB Ethernet - Device is going in/out suspend A simple way to reproduce the issue could be: 1. Copy a big file (like 500MB) from host PC to device with scp 2. While the file copy is ongoing, suspend & resume the device like: $ echo +3 > /sys/class/rtc/rtc0/wakealarm; echo mem > /sys/power/state 3. The device will hang up there I reproduced on the following kernels: - Mainline kernel - NXP kernel lf-6.6.y - NXP kernel lf-6.12.y But NXP kernel lf-6.1.y doesn't have this problem. I tracked it down to Peter's commit [1] on lf-6.1.y, and found that the gadget disconnect & connect calls got lost from suspend & resume hooks, when the commit were split and pushed upstream. I confirm that adding the calls back fixes the hangup. ---8<-------------------- ---->8------------------ But it's unclear to me why the hangup happens and how the change above fix the problem. Do you guys have any insight here? Shawn [1] https://github.com/reMarkable/linux-imx/commit/0791d25578cb0e46fd93ae7a3c36ff7a424f3547