| Message ID | 1595408575-13150-1-git-send-email-jun.li@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5bde3f020a150aa16f396ce22b6a778627dd4484 |
| Headers | show |
| Series | usb: dwc3: debugfs: do not queue work if try to change mode on non-drd | expand |
Hi, Li Jun wrote: > Do not try to queue a drd work for change mode if the port is not a drd, > this is to avoid below kernel dump: Are you talking about OTG or DRD? This patch seems to be for OTG. If you need to debug and manually do role switch for DRD from userspace, enable and use role class sysfs attribute. BR, Thinh > [ 60.115529] ------------[ cut here ]------------ > [ 60.120166] WARNING: CPU: 1 PID: 627 at kernel/workqueue.c:1473 > __queue_work+0x46c/0x520 > [ 60.128254] Modules linked in: > [ 60.131313] CPU: 1 PID: 627 Comm: sh Not tainted > 5.7.0-rc4-00022-g914a586-dirty #135 > [ 60.139054] Hardware name: NXP i.MX8MQ EVK (DT) > [ 60.143585] pstate: a0000085 (NzCv daIf -PAN -UAO) > [ 60.148376] pc : __queue_work+0x46c/0x520 > [ 60.152385] lr : __queue_work+0x314/0x520 > [ 60.156393] sp : ffff8000124ebc40 > [ 60.159705] x29: ffff8000124ebc40 x28: ffff800011808018 > [ 60.165018] x27: ffff800011819ef8 x26: ffff800011d39980 > [ 60.170331] x25: ffff800011808018 x24: 0000000000000100 > [ 60.175643] x23: 0000000000000013 x22: 0000000000000001 > [ 60.180955] x21: ffff0000b7c08e00 x20: ffff0000b6c31080 > [ 60.186267] x19: ffff0000bb99bc00 x18: 0000000000000000 > [ 60.191579] x17: 0000000000000000 x16: 0000000000000000 > [ 60.196891] x15: 0000000000000000 x14: 0000000000000000 > [ 60.202202] x13: 0000000000000000 x12: 0000000000000000 > [ 60.207515] x11: 0000000000000000 x10: 0000000000000040 > [ 60.212827] x9 : ffff800011d55460 x8 : ffff800011d55458 > [ 60.218138] x7 : ffff0000b7800028 x6 : 0000000000000000 > [ 60.223450] x5 : ffff0000b7800000 x4 : 0000000000000000 > [ 60.228762] x3 : ffff0000bb997cc0 x2 : 0000000000000001 > [ 60.234074] x1 : 0000000000000000 x0 : ffff0000b6c31088 > [ 60.239386] Call trace: > [ 60.241834] __queue_work+0x46c/0x520 > [ 60.245496] queue_work_on+0x6c/0x90 > [ 60.249075] dwc3_set_mode+0x48/0x58 > [ 60.252651] dwc3_mode_write+0xf8/0x150 > [ 60.256489] full_proxy_write+0x5c/0xa8 > [ 60.260327] __vfs_write+0x18/0x40 > [ 60.263729] vfs_write+0xdc/0x1c8 > [ 60.267045] ksys_write+0x68/0xf0 > [ 60.270360] __arm64_sys_write+0x18/0x20 > [ 60.274286] el0_svc_common.constprop.0+0x68/0x160 > [ 60.279077] do_el0_svc+0x20/0x80 > [ 60.282394] el0_sync_handler+0x10c/0x178 > [ 60.286403] el0_sync+0x140/0x180 > [ 60.289716] ---[ end trace 70b155582e2b7988 ]--- > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/dwc3/debugfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c > index 6d9de33..99e7f53 100644 > --- a/drivers/usb/dwc3/debugfs.c > +++ b/drivers/usb/dwc3/debugfs.c > @@ -428,6 +428,9 @@ static ssize_t dwc3_mode_write(struct file *file, > if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count))) > return -EFAULT; > > + if (dwc->dr_mode != USB_DR_MODE_OTG) > + return count; > + > if (!strncmp(buf, "host", 4)) > mode = DWC3_GCTL_PRTCAP_HOST; >
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Thursday, July 23, 2020 4:36 AM > To: Jun Li <jun.li@nxp.com>; balbi@kernel.org > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org > Subject: Re: [PATCH] usb: dwc3: debugfs: do not queue work if try to change mode > on non-drd > > Hi, > > Li Jun wrote: > > Do not try to queue a drd work for change mode if the port is not a > > drd, this is to avoid below kernel dump: > > Are you talking about OTG or DRD? This patch seems to be for OTG. If you need to > debug and manually do role switch for DRD from userspace, enable and use role class > sysfs attribute. There is no problem in normal cases, I triggered this warning when try to change the role for a single role port via this debugfs file, I think nothing should stop user to use this file for debug purpose. There is condition check of USB_DR_MODE_OTG when init drd_work, but no this check when queue work, so maybe a better change is to move the condition check from __dwc3_set_mode() into dwc3_set_mode() diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 6adaa7d..2fa50ec 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -120,9 +120,6 @@ static void __dwc3_set_mode(struct work_struct *work) unsigned long flags; int ret; - if (dwc->dr_mode != USB_DR_MODE_OTG) - return; - pm_runtime_get_sync(dwc->dev); if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG) @@ -205,6 +202,9 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode) { unsigned long flags; + if (dwc->dr_mode != USB_DR_MODE_OTG) + return; + spin_lock_irqsave(&dwc->lock, flags); dwc->desired_dr_role = mode; spin_unlock_irqrestore(&dwc->lock, flags); Thanks Li Jun > > BR, > Thinh > > > [ 60.115529] ------------[ cut here ]------------ > > [ 60.120166] WARNING: CPU: 1 PID: 627 at kernel/workqueue.c:1473 > > __queue_work+0x46c/0x520 > > [ 60.128254] Modules linked in: > > [ 60.131313] CPU: 1 PID: 627 Comm: sh Not tainted > > 5.7.0-rc4-00022-g914a586-dirty #135 > > [ 60.139054] Hardware name: NXP i.MX8MQ EVK (DT) > > [ 60.143585] pstate: a0000085 (NzCv daIf -PAN -UAO) > > [ 60.148376] pc : __queue_work+0x46c/0x520 > > [ 60.152385] lr : __queue_work+0x314/0x520 > > [ 60.156393] sp : ffff8000124ebc40 > > [ 60.159705] x29: ffff8000124ebc40 x28: ffff800011808018 > > [ 60.165018] x27: ffff800011819ef8 x26: ffff800011d39980 > > [ 60.170331] x25: ffff800011808018 x24: 0000000000000100 > > [ 60.175643] x23: 0000000000000013 x22: 0000000000000001 > > [ 60.180955] x21: ffff0000b7c08e00 x20: ffff0000b6c31080 > > [ 60.186267] x19: ffff0000bb99bc00 x18: 0000000000000000 > > [ 60.191579] x17: 0000000000000000 x16: 0000000000000000 > > [ 60.196891] x15: 0000000000000000 x14: 0000000000000000 > > [ 60.202202] x13: 0000000000000000 x12: 0000000000000000 > > [ 60.207515] x11: 0000000000000000 x10: 0000000000000040 > > [ 60.212827] x9 : ffff800011d55460 x8 : ffff800011d55458 > > [ 60.218138] x7 : ffff0000b7800028 x6 : 0000000000000000 > > [ 60.223450] x5 : ffff0000b7800000 x4 : 0000000000000000 > > [ 60.228762] x3 : ffff0000bb997cc0 x2 : 0000000000000001 > > [ 60.234074] x1 : 0000000000000000 x0 : ffff0000b6c31088 > > [ 60.239386] Call trace: > > [ 60.241834] __queue_work+0x46c/0x520 > > [ 60.245496] queue_work_on+0x6c/0x90 > > [ 60.249075] dwc3_set_mode+0x48/0x58 > > [ 60.252651] dwc3_mode_write+0xf8/0x150 > > [ 60.256489] full_proxy_write+0x5c/0xa8 > > [ 60.260327] __vfs_write+0x18/0x40 > > [ 60.263729] vfs_write+0xdc/0x1c8 > > [ 60.267045] ksys_write+0x68/0xf0 > > [ 60.270360] __arm64_sys_write+0x18/0x20 > > [ 60.274286] el0_svc_common.constprop.0+0x68/0x160 > > [ 60.279077] do_el0_svc+0x20/0x80 > > [ 60.282394] el0_sync_handler+0x10c/0x178 > > [ 60.286403] el0_sync+0x140/0x180 > > [ 60.289716] ---[ end trace 70b155582e2b7988 ]--- > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/dwc3/debugfs.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c > > index 6d9de33..99e7f53 100644 > > --- a/drivers/usb/dwc3/debugfs.c > > +++ b/drivers/usb/dwc3/debugfs.c > > @@ -428,6 +428,9 @@ static ssize_t dwc3_mode_write(struct file *file, > > if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count))) > > return -EFAULT; > > > > + if (dwc->dr_mode != USB_DR_MODE_OTG) > > + return count; > > + > > if (!strncmp(buf, "host", 4)) > > mode = DWC3_GCTL_PRTCAP_HOST; > >
Jun Li wrote: > >> -----Original Message----- >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Sent: Thursday, July 23, 2020 4:36 AM >> To: Jun Li <jun.li@nxp.com>; balbi@kernel.org >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org >> Subject: Re: [PATCH] usb: dwc3: debugfs: do not queue work if try to change mode >> on non-drd >> >> Hi, >> >> Li Jun wrote: >>> Do not try to queue a drd work for change mode if the port is not a >>> drd, this is to avoid below kernel dump: >> Are you talking about OTG or DRD? This patch seems to be for OTG. If you need to >> debug and manually do role switch for DRD from userspace, enable and use role class >> sysfs attribute. > There is no problem in normal cases, I triggered this warning > when try to change the role for a single role port via this debugfs > file, I think nothing should stop user to use this file for debug > purpose. Ok. > > There is condition check of USB_DR_MODE_OTG when init drd_work, but > no this check when queue work, so maybe a better change is to move > the condition check from __dwc3_set_mode() into dwc3_set_mode() Yeah, this looks better. > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 6adaa7d..2fa50ec 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -120,9 +120,6 @@ static void __dwc3_set_mode(struct work_struct *work) > unsigned long flags; > int ret; > > - if (dwc->dr_mode != USB_DR_MODE_OTG) > - return; > - > pm_runtime_get_sync(dwc->dev); > > if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG) > @@ -205,6 +202,9 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode) > { > unsigned long flags; > > + if (dwc->dr_mode != USB_DR_MODE_OTG) > + return; > + > spin_lock_irqsave(&dwc->lock, flags); > dwc->desired_dr_role = mode; > spin_unlock_irqrestore(&dwc->lock, flags); > BR, Thinh
Hi > -----Original Message----- > From: Jun Li <jun.li@nxp.com> > Sent: Wednesday, July 22, 2020 5:07 PM > To: balbi@kernel.org > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org > Subject: [PATCH] usb: dwc3: debugfs: do not queue work if try to change mode on > non-drd > > Do not try to queue a drd work for change mode if the port is not a drd, this is > to avoid below kernel dump: > [ 60.115529] ------------[ cut here ]------------ > [ 60.120166] WARNING: CPU: 1 PID: 627 at kernel/workqueue.c:1473 > __queue_work+0x46c/0x520 > [ 60.128254] Modules linked in: > [ 60.131313] CPU: 1 PID: 627 Comm: sh Not tainted > 5.7.0-rc4-00022-g914a586-dirty #135 > [ 60.139054] Hardware name: NXP i.MX8MQ EVK (DT) > [ 60.143585] pstate: a0000085 (NzCv daIf -PAN -UAO) > [ 60.148376] pc : __queue_work+0x46c/0x520 > [ 60.152385] lr : __queue_work+0x314/0x520 > [ 60.156393] sp : ffff8000124ebc40 > [ 60.159705] x29: ffff8000124ebc40 x28: ffff800011808018 > [ 60.165018] x27: ffff800011819ef8 x26: ffff800011d39980 > [ 60.170331] x25: ffff800011808018 x24: 0000000000000100 > [ 60.175643] x23: 0000000000000013 x22: 0000000000000001 > [ 60.180955] x21: ffff0000b7c08e00 x20: ffff0000b6c31080 > [ 60.186267] x19: ffff0000bb99bc00 x18: 0000000000000000 > [ 60.191579] x17: 0000000000000000 x16: 0000000000000000 > [ 60.196891] x15: 0000000000000000 x14: 0000000000000000 > [ 60.202202] x13: 0000000000000000 x12: 0000000000000000 > [ 60.207515] x11: 0000000000000000 x10: 0000000000000040 > [ 60.212827] x9 : ffff800011d55460 x8 : ffff800011d55458 > [ 60.218138] x7 : ffff0000b7800028 x6 : 0000000000000000 > [ 60.223450] x5 : ffff0000b7800000 x4 : 0000000000000000 > [ 60.228762] x3 : ffff0000bb997cc0 x2 : 0000000000000001 > [ 60.234074] x1 : 0000000000000000 x0 : ffff0000b6c31088 > [ 60.239386] Call trace: > [ 60.241834] __queue_work+0x46c/0x520 > [ 60.245496] queue_work_on+0x6c/0x90 > [ 60.249075] dwc3_set_mode+0x48/0x58 > [ 60.252651] dwc3_mode_write+0xf8/0x150 > [ 60.256489] full_proxy_write+0x5c/0xa8 > [ 60.260327] __vfs_write+0x18/0x40 > [ 60.263729] vfs_write+0xdc/0x1c8 > [ 60.267045] ksys_write+0x68/0xf0 > [ 60.270360] __arm64_sys_write+0x18/0x20 > [ 60.274286] el0_svc_common.constprop.0+0x68/0x160 > [ 60.279077] do_el0_svc+0x20/0x80 > [ 60.282394] el0_sync_handler+0x10c/0x178 > [ 60.286403] el0_sync+0x140/0x180 > [ 60.289716] ---[ end trace 70b155582e2b7988 ]--- > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/dwc3/debugfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c index > 6d9de33..99e7f53 100644 > --- a/drivers/usb/dwc3/debugfs.c > +++ b/drivers/usb/dwc3/debugfs.c > @@ -428,6 +428,9 @@ static ssize_t dwc3_mode_write(struct file *file, > if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count))) > return -EFAULT; > > + if (dwc->dr_mode != USB_DR_MODE_OTG) > + return count; > + > if (!strncmp(buf, "host", 4)) > mode = DWC3_GCTL_PRTCAP_HOST; > > -- > 2.7.4 A gentle ping... Li Jun
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c index 6d9de33..99e7f53 100644 --- a/drivers/usb/dwc3/debugfs.c +++ b/drivers/usb/dwc3/debugfs.c @@ -428,6 +428,9 @@ static ssize_t dwc3_mode_write(struct file *file, if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count))) return -EFAULT; + if (dwc->dr_mode != USB_DR_MODE_OTG) + return count; + if (!strncmp(buf, "host", 4)) mode = DWC3_GCTL_PRTCAP_HOST;
Do not try to queue a drd work for change mode if the port is not a drd, this is to avoid below kernel dump: [ 60.115529] ------------[ cut here ]------------ [ 60.120166] WARNING: CPU: 1 PID: 627 at kernel/workqueue.c:1473 __queue_work+0x46c/0x520 [ 60.128254] Modules linked in: [ 60.131313] CPU: 1 PID: 627 Comm: sh Not tainted 5.7.0-rc4-00022-g914a586-dirty #135 [ 60.139054] Hardware name: NXP i.MX8MQ EVK (DT) [ 60.143585] pstate: a0000085 (NzCv daIf -PAN -UAO) [ 60.148376] pc : __queue_work+0x46c/0x520 [ 60.152385] lr : __queue_work+0x314/0x520 [ 60.156393] sp : ffff8000124ebc40 [ 60.159705] x29: ffff8000124ebc40 x28: ffff800011808018 [ 60.165018] x27: ffff800011819ef8 x26: ffff800011d39980 [ 60.170331] x25: ffff800011808018 x24: 0000000000000100 [ 60.175643] x23: 0000000000000013 x22: 0000000000000001 [ 60.180955] x21: ffff0000b7c08e00 x20: ffff0000b6c31080 [ 60.186267] x19: ffff0000bb99bc00 x18: 0000000000000000 [ 60.191579] x17: 0000000000000000 x16: 0000000000000000 [ 60.196891] x15: 0000000000000000 x14: 0000000000000000 [ 60.202202] x13: 0000000000000000 x12: 0000000000000000 [ 60.207515] x11: 0000000000000000 x10: 0000000000000040 [ 60.212827] x9 : ffff800011d55460 x8 : ffff800011d55458 [ 60.218138] x7 : ffff0000b7800028 x6 : 0000000000000000 [ 60.223450] x5 : ffff0000b7800000 x4 : 0000000000000000 [ 60.228762] x3 : ffff0000bb997cc0 x2 : 0000000000000001 [ 60.234074] x1 : 0000000000000000 x0 : ffff0000b6c31088 [ 60.239386] Call trace: [ 60.241834] __queue_work+0x46c/0x520 [ 60.245496] queue_work_on+0x6c/0x90 [ 60.249075] dwc3_set_mode+0x48/0x58 [ 60.252651] dwc3_mode_write+0xf8/0x150 [ 60.256489] full_proxy_write+0x5c/0xa8 [ 60.260327] __vfs_write+0x18/0x40 [ 60.263729] vfs_write+0xdc/0x1c8 [ 60.267045] ksys_write+0x68/0xf0 [ 60.270360] __arm64_sys_write+0x18/0x20 [ 60.274286] el0_svc_common.constprop.0+0x68/0x160 [ 60.279077] do_el0_svc+0x20/0x80 [ 60.282394] el0_sync_handler+0x10c/0x178 [ 60.286403] el0_sync+0x140/0x180 [ 60.289716] ---[ end trace 70b155582e2b7988 ]--- Signed-off-by: Li Jun <jun.li@nxp.com> --- drivers/usb/dwc3/debugfs.c | 3 +++ 1 file changed, 3 insertions(+)