| Message ID | 20240618031918.2585799-1-Meng.Li@windriver.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7838de15bb700c2898a7d741db9b1f3cbc86c136 |
| Headers | show |
| Series | [v2] usb: dwc3: core: remove lock of otg mode during gadget suspend/resume to avoid deadlock | expand |
On Tue, Jun 18, 2024, Meng Li wrote: > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger system > to enter suspend status with below command: > echo mem > /sys/power/state > There will be a deadlock issue occurring. Detailed invoking path as > below: > dwc3_suspend_common() > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > dwc3_gadget_suspend(dwc); > dwc3_gadget_soft_disconnect(dwc); > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd > This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: Fix > NULL pointer dereference in dwc3_gadget_suspend") that removes the code > of checking whether dwc->gadget_driver is NULL or not. It causes the > following code is executed and deadlock occurs when trying to get the > spinlock. In fact, the root cause is the commit 5265397f9442("usb: dwc3: > Remove DWC3 locking during gadget suspend/resume") that forgot to remove > the lock of otg mode. So, remove the redundant lock of otg mode during > gadget suspend/resume. > > Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget suspend/resume") > Cc: Xu Yang <xu.yang_2@nxp.com> > Cc: stable@vger.kernel.org > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > --- > v1->v2: > - improve commit log, correct the Fixes commit id. > > --- > drivers/usb/dwc3/core.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 7ee61a89520b..9d47c3aa5777 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > - unsigned long flags; > u32 reg; > int i; > > @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > break; > > if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > - spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_suspend(dwc); > - spin_unlock_irqrestore(&dwc->lock, flags); > synchronize_irq(dwc->irq_gadget); > } > > @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > { > - unsigned long flags; > int ret; > u32 reg; > int i; > @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { > dwc3_otg_host_init(dwc); > } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > - spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_resume(dwc); > - spin_unlock_irqrestore(&dwc->lock, flags); > } > > break; > -- > 2.34.1 > Did you use "git format-patch -v"? The $subject version prefix order is different. In any case, Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
Hi On Tue, Jun 18, 2024 at 11:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Tue, Jun 18, 2024, Meng Li wrote: > > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger system > > to enter suspend status with below command: > > echo mem > /sys/power/state > > There will be a deadlock issue occurring. Detailed invoking path as > > below: > > dwc3_suspend_common() > > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > > dwc3_gadget_suspend(dwc); > > dwc3_gadget_soft_disconnect(dwc); > > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd In resume path the spinlock was taken too and I see that there is a call dwc3_gadget_soft_connect that call the dwc3_core_soft_reset that can poll using msleep for time. I don't know if this is a valid fix but seems does not fix only commit c7ebd8149ee5 Michael > > This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: Fix > > NULL pointer dereference in dwc3_gadget_suspend") that removes the code > > of checking whether dwc->gadget_driver is NULL or not. It causes the > > following code is executed and deadlock occurs when trying to get the > > spinlock. In fact, the root cause is the commit 5265397f9442("usb: dwc3: > > Remove DWC3 locking during gadget suspend/resume") that forgot to remove > > the lock of otg mode. So, remove the redundant lock of otg mode during > > gadget suspend/resume. > > > > Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget suspend/resume") > > Cc: Xu Yang <xu.yang_2@nxp.com> > > Cc: stable@vger.kernel.org > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > > > --- > > v1->v2: > > - improve commit log, correct the Fixes commit id. > > > > --- > > drivers/usb/dwc3/core.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 7ee61a89520b..9d47c3aa5777 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) > > > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > { > > - unsigned long flags; > > u32 reg; > > int i; > > > > @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > break; > > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > - spin_lock_irqsave(&dwc->lock, flags); > > dwc3_gadget_suspend(dwc); > > - spin_unlock_irqrestore(&dwc->lock, flags); > > synchronize_irq(dwc->irq_gadget); > > } > > > > @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > > static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > { > > - unsigned long flags; > > int ret; > > u32 reg; > > int i; > > @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { > > dwc3_otg_host_init(dwc); > > } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > - spin_lock_irqsave(&dwc->lock, flags); > > dwc3_gadget_resume(dwc); > > - spin_unlock_irqrestore(&dwc->lock, flags); > > } > > > > break; > > -- > > 2.34.1 > > > > Did you use "git format-patch -v"? The $subject version prefix order is > different. In any case, > > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Thanks, > Thinh
Hi, On Wed, Jun 19, 2024, Michael Nazzareno Trimarchi wrote: > Hi > > On Tue, Jun 18, 2024 at 11:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > On Tue, Jun 18, 2024, Meng Li wrote: > > > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger system > > > to enter suspend status with below command: > > > echo mem > /sys/power/state > > > There will be a deadlock issue occurring. Detailed invoking path as > > > below: > > > dwc3_suspend_common() > > > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > > > dwc3_gadget_suspend(dwc); > > > dwc3_gadget_soft_disconnect(dwc); > > > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd > > > In resume path the spinlock was taken too and I see that there is a call > dwc3_gadget_soft_connect that call the dwc3_core_soft_reset that can poll > using msleep for time. I don't know if this is a valid fix but seems > does not fix > only commit c7ebd8149ee5 > Meng did take the resume path into account. Can you clarify what you meant with "does not fix only commit c7ebd8149ee5"? Thanks, Thinh > > > > This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: Fix > > > NULL pointer dereference in dwc3_gadget_suspend") that removes the code > > > of checking whether dwc->gadget_driver is NULL or not. It causes the > > > following code is executed and deadlock occurs when trying to get the > > > spinlock. In fact, the root cause is the commit 5265397f9442("usb: dwc3: > > > Remove DWC3 locking during gadget suspend/resume") that forgot to remove > > > the lock of otg mode. So, remove the redundant lock of otg mode during > > > gadget suspend/resume. > > > > > > Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget suspend/resume") > > > Cc: Xu Yang <xu.yang_2@nxp.com> > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > > > > > --- > > > v1->v2: > > > - improve commit log, correct the Fixes commit id. > > > > > > --- > > > drivers/usb/dwc3/core.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 7ee61a89520b..9d47c3aa5777 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) > > > > > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > { > > > - unsigned long flags; > > > u32 reg; > > > int i; > > > > > > @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > break; > > > > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > > - spin_lock_irqsave(&dwc->lock, flags); > > > dwc3_gadget_suspend(dwc); > > > - spin_unlock_irqrestore(&dwc->lock, flags); > > > synchronize_irq(dwc->irq_gadget); > > > } > > > > > > @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > > > > static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > > { > > > - unsigned long flags; > > > int ret; > > > u32 reg; > > > int i; > > > @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { > > > dwc3_otg_host_init(dwc); > > > } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > > - spin_lock_irqsave(&dwc->lock, flags); > > > dwc3_gadget_resume(dwc); > > > - spin_unlock_irqrestore(&dwc->lock, flags); > > > } > > > > > > break; > > > -- > > > 2.34.1 > > > > > > > Did you use "git format-patch -v"? The $subject version prefix order is > > different. In any case, > > > > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > Thanks, > > Thinh
Hi On Wed, Jun 19, 2024 at 12:42 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > Hi, > > On Wed, Jun 19, 2024, Michael Nazzareno Trimarchi wrote: > > Hi > > > > On Tue, Jun 18, 2024 at 11:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > > On Tue, Jun 18, 2024, Meng Li wrote: > > > > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger system > > > > to enter suspend status with below command: > > > > echo mem > /sys/power/state > > > > There will be a deadlock issue occurring. Detailed invoking path as > > > > below: > > > > dwc3_suspend_common() > > > > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > > > > dwc3_gadget_suspend(dwc); > > > > dwc3_gadget_soft_disconnect(dwc); > > > > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd > > > > > > In resume path the spinlock was taken too and I see that there is a call > > dwc3_gadget_soft_connect that call the dwc3_core_soft_reset that can poll > > using msleep for time. I don't know if this is a valid fix but seems > > does not fix > > only commit c7ebd8149ee5 > > > > Meng did take the resume path into account. Can you clarify what you > meant with "does not fix only commit c7ebd8149ee5"? I have seen that is suppose to fix something introduced by that commit but I think that resume path was already broken before spinlock_irqsave() dwc3_gadget_resume -> dwc3_gadget_soft_connect -> dwc3_core_soft_reset (can sleep) Michael > > Thanks, > Thinh > > > > > > > This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: Fix > > > > NULL pointer dereference in dwc3_gadget_suspend") that removes the code > > > > of checking whether dwc->gadget_driver is NULL or not. It causes the > > > > following code is executed and deadlock occurs when trying to get the > > > > spinlock. In fact, the root cause is the commit 5265397f9442("usb: dwc3: > > > > Remove DWC3 locking during gadget suspend/resume") that forgot to remove > > > > the lock of otg mode. So, remove the redundant lock of otg mode during > > > > gadget suspend/resume. > > > > > > > > Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget suspend/resume") > > > > Cc: Xu Yang <xu.yang_2@nxp.com> > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > > > > > > > --- > > > > v1->v2: > > > > - improve commit log, correct the Fixes commit id. > > > > > > > > --- > > > > drivers/usb/dwc3/core.c | 6 ------ > > > > 1 file changed, 6 deletions(-) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 7ee61a89520b..9d47c3aa5777 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) > > > > > > > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > > { > > > > - unsigned long flags; > > > > u32 reg; > > > > int i; > > > > > > > > @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > > break; > > > > > > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > > > - spin_lock_irqsave(&dwc->lock, flags); > > > > dwc3_gadget_suspend(dwc); > > > > - spin_unlock_irqrestore(&dwc->lock, flags); > > > > synchronize_irq(dwc->irq_gadget); > > > > } > > > > > > > > @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > > > > > > > static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > > > { > > > > - unsigned long flags; > > > > int ret; > > > > u32 reg; > > > > int i; > > > > @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { > > > > dwc3_otg_host_init(dwc); > > > > } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > > > - spin_lock_irqsave(&dwc->lock, flags); > > > > dwc3_gadget_resume(dwc); > > > > - spin_unlock_irqrestore(&dwc->lock, flags); > > > > } > > > > > > > > break; > > > > -- > > > > 2.34.1 > > > > > > > > > > Did you use "git format-patch -v"? The $subject version prefix order is > > > different. In any case, > > > > > > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > > > Thanks, > > > Thinh
On Wed, Jun 19, 2024, Michael Nazzareno Trimarchi wrote: > Hi > > On Wed, Jun 19, 2024 at 12:42 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > Hi, > > > > On Wed, Jun 19, 2024, Michael Nazzareno Trimarchi wrote: > > > Hi > > > > > > On Tue, Jun 18, 2024 at 11:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > > > > On Tue, Jun 18, 2024, Meng Li wrote: > > > > > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger system > > > > > to enter suspend status with below command: > > > > > echo mem > /sys/power/state > > > > > There will be a deadlock issue occurring. Detailed invoking path as > > > > > below: > > > > > dwc3_suspend_common() > > > > > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > > > > > dwc3_gadget_suspend(dwc); > > > > > dwc3_gadget_soft_disconnect(dwc); > > > > > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd > > > > > > > > > In resume path the spinlock was taken too and I see that there is a call > > > dwc3_gadget_soft_connect that call the dwc3_core_soft_reset that can poll > > > using msleep for time. I don't know if this is a valid fix but seems > > > does not fix > > > only commit c7ebd8149ee5 > > > > > > > Meng did take the resume path into account. Can you clarify what you > > meant with "does not fix only commit c7ebd8149ee5"? > > I have seen that is suppose to fix something introduced by that commit > but I think > that resume path was already broken before > > spinlock_irqsave() > dwc3_gadget_resume -> dwc3_gadget_soft_connect -> dwc3_core_soft_reset > (can sleep) > The code path for OTG should not hit that sleep. The sleep is only applicable to DWC_usb31 and DWC_usb32. Technically, it wasn't broken before. But we should remove this spinlock on resume as it is done here. BR, Thinh
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Wednesday, June 19, 2024 5:54 AM > To: Li, Meng <Meng.Li@windriver.com> > Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; > gregkh@linuxfoundation.org; quic_uaggarwa@quicinc.com; linux- > usb@vger.kernel.org; linux-kernel@vger.kernel.org; xu.yang_2@nxp.com > Subject: Re: [v2 PATCH] usb: dwc3: core: remove lock of otg mode during > gadget suspend/resume to avoid deadlock > > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and > know the content is safe. > > On Tue, Jun 18, 2024, Meng Li wrote: > > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger > system > > to enter suspend status with below command: > > echo mem > /sys/power/state > > There will be a deadlock issue occurring. Detailed invoking path as > > below: > > dwc3_suspend_common() > > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > > dwc3_gadget_suspend(dwc); > > dwc3_gadget_soft_disconnect(dwc); > > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd > > This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: Fix > > NULL pointer dereference in dwc3_gadget_suspend") that removes the > > code of checking whether dwc->gadget_driver is NULL or not. It causes > > the following code is executed and deadlock occurs when trying to get > > the spinlock. In fact, the root cause is the commit 5265397f9442("usb: > dwc3: > > Remove DWC3 locking during gadget suspend/resume") that forgot to > > remove the lock of otg mode. So, remove the redundant lock of otg mode > > during gadget suspend/resume. > > > > Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget > > suspend/resume") > > Cc: Xu Yang <xu.yang_2@nxp.com> > > Cc: stable@vger.kernel.org > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > > > --- > > v1->v2: > > - improve commit log, correct the Fixes commit id. > > > > --- > > drivers/usb/dwc3/core.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index > > 7ee61a89520b..9d47c3aa5777 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct > dwc3 > > *dwc) > > > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > > - unsigned long flags; > > u32 reg; > > int i; > > > > @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 > *dwc, pm_message_t msg) > > break; > > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > - spin_lock_irqsave(&dwc->lock, flags); > > dwc3_gadget_suspend(dwc); > > - spin_unlock_irqrestore(&dwc->lock, flags); > > synchronize_irq(dwc->irq_gadget); > > } > > > > @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 > *dwc, > > pm_message_t msg) > > > > static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) { > > - unsigned long flags; > > int ret; > > u32 reg; > > int i; > > @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 > *dwc, pm_message_t msg) > > if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { > > dwc3_otg_host_init(dwc); > > } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > - spin_lock_irqsave(&dwc->lock, flags); > > dwc3_gadget_resume(dwc); > > - spin_unlock_irqrestore(&dwc->lock, flags); > > } > > > > break; > > -- > > 2.34.1 > > > > Did you use "git format-patch -v"? The $subject version prefix order is > different. In any case, > Sorry! I use --subject-prefix="v2 PATCH". I will correct it in next patch pull request. Thanks, Limeng > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Thanks, > Thinh
> -----Original Message----- > From: Li, Meng > Sent: Wednesday, June 19, 2024 9:49 AM > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Cc: gregkh@linuxfoundation.org; quic_uaggarwa@quicinc.com; linux- > usb@vger.kernel.org; linux-kernel@vger.kernel.org; xu.yang_2@nxp.com > Subject: RE: [v2 PATCH] usb: dwc3: core: remove lock of otg mode during > gadget suspend/resume to avoid deadlock > > > > > -----Original Message----- > > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Sent: Wednesday, June 19, 2024 5:54 AM > > To: Li, Meng <Meng.Li@windriver.com> > > Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; > > gregkh@linuxfoundation.org; quic_uaggarwa@quicinc.com; linux- > > usb@vger.kernel.org; linux-kernel@vger.kernel.org; xu.yang_2@nxp.com > > Subject: Re: [v2 PATCH] usb: dwc3: core: remove lock of otg mode > > during gadget suspend/resume to avoid deadlock > > > > CAUTION: This email comes from a non Wind River email account! > > Do not click links or open attachments unless you recognize the sender > > and know the content is safe. > > > > On Tue, Jun 18, 2024, Meng Li wrote: > > > When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger > > system > > > to enter suspend status with below command: > > > echo mem > /sys/power/state > > > There will be a deadlock issue occurring. Detailed invoking path as > > > below: > > > dwc3_suspend_common() > > > spin_lock_irqsave(&dwc->lock, flags); <-- 1st > > > dwc3_gadget_suspend(dwc); > > > dwc3_gadget_soft_disconnect(dwc); > > > spin_lock_irqsave(&dwc->lock, flags); <-- 2nd > > > This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: > > > Fix NULL pointer dereference in dwc3_gadget_suspend") that removes > > > the code of checking whether dwc->gadget_driver is NULL or not. It > > > causes the following code is executed and deadlock occurs when > > > trying to get the spinlock. In fact, the root cause is the commit > 5265397f9442("usb: > > dwc3: > > > Remove DWC3 locking during gadget suspend/resume") that forgot to > > > remove the lock of otg mode. So, remove the redundant lock of otg > > > mode during gadget suspend/resume. > > > > > > Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget > > > suspend/resume") > > > Cc: Xu Yang <xu.yang_2@nxp.com> > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Meng Li <Meng.Li@windriver.com> > > > > > > --- > > > v1->v2: > > > - improve commit log, correct the Fixes commit id. > > > > > > --- > > > drivers/usb/dwc3/core.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index > > > 7ee61a89520b..9d47c3aa5777 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct > > dwc3 > > > *dwc) > > > > > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t > msg) > > { > > > - unsigned long flags; > > > u32 reg; > > > int i; > > > > > > @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 > > *dwc, pm_message_t msg) > > > break; > > > > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > > - spin_lock_irqsave(&dwc->lock, flags); > > > dwc3_gadget_suspend(dwc); > > > - spin_unlock_irqrestore(&dwc->lock, flags); > > > synchronize_irq(dwc->irq_gadget); > > > } > > > > > > @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 > > *dwc, > > > pm_message_t msg) > > > > > > static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > { > > > - unsigned long flags; > > > int ret; > > > u32 reg; > > > int i; > > > @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 > > *dwc, pm_message_t msg) > > > if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { > > > dwc3_otg_host_init(dwc); > > > } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { > > > - spin_lock_irqsave(&dwc->lock, flags); > > > dwc3_gadget_resume(dwc); > > > - spin_unlock_irqrestore(&dwc->lock, flags); > > > } > > > > > > break; > > > -- > > > 2.34.1 > > > > > > > Did you use "git format-patch -v"? The $subject version prefix order > > is different. In any case, > > > > Sorry! I use --subject-prefix="v2 PATCH". > I will correct it in next patch pull request. > > Thanks, > Limeng > Do I need to send another v3 pull request? Thanks, LImeng > > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > Thanks, > > Thinh
On Wed, Jun 19, 2024, Li, Meng wrote: > > Do I need to send another v3 pull request? > No. Greg already picked this up. Thanks, Thinh
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Saturday, June 22, 2024 7:15 AM > To: Li, Meng <Meng.Li@windriver.com> > Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; > gregkh@linuxfoundation.org; quic_uaggarwa@quicinc.com; linux- > usb@vger.kernel.org; linux-kernel@vger.kernel.org; xu.yang_2@nxp.com > Subject: Re: [v2 PATCH] usb: dwc3: core: remove lock of otg mode during > gadget suspend/resume to avoid deadlock > > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and > know the content is safe. > > On Wed, Jun 19, 2024, Li, Meng wrote: > > > > Do I need to send another v3 pull request? > > > > No. Greg already picked this up. > Thanks for reviewing and accepting my patch. You have given me great enthusiasm to contribute to the community regards, Limeng > Thanks, > Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 7ee61a89520b..9d47c3aa5777 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -2250,7 +2250,6 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) { - unsigned long flags; u32 reg; int i; @@ -2293,9 +2292,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) break; if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { - spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_suspend(dwc); - spin_unlock_irqrestore(&dwc->lock, flags); synchronize_irq(dwc->irq_gadget); } @@ -2312,7 +2309,6 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) { - unsigned long flags; int ret; u32 reg; int i; @@ -2366,9 +2362,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) if (dwc->current_otg_role == DWC3_OTG_ROLE_HOST) { dwc3_otg_host_init(dwc); } else if (dwc->current_otg_role == DWC3_OTG_ROLE_DEVICE) { - spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_resume(dwc); - spin_unlock_irqrestore(&dwc->lock, flags); } break;
When config CONFIG_USB_DWC3_DUAL_ROLE is selected, and trigger system to enter suspend status with below command: echo mem > /sys/power/state There will be a deadlock issue occurring. Detailed invoking path as below: dwc3_suspend_common() spin_lock_irqsave(&dwc->lock, flags); <-- 1st dwc3_gadget_suspend(dwc); dwc3_gadget_soft_disconnect(dwc); spin_lock_irqsave(&dwc->lock, flags); <-- 2nd This issue is exposed by commit c7ebd8149ee5 ("usb: dwc3: gadget: Fix NULL pointer dereference in dwc3_gadget_suspend") that removes the code of checking whether dwc->gadget_driver is NULL or not. It causes the following code is executed and deadlock occurs when trying to get the spinlock. In fact, the root cause is the commit 5265397f9442("usb: dwc3: Remove DWC3 locking during gadget suspend/resume") that forgot to remove the lock of otg mode. So, remove the redundant lock of otg mode during gadget suspend/resume. Fixes: 5265397f9442 ("usb: dwc3: Remove DWC3 locking during gadget suspend/resume") Cc: Xu Yang <xu.yang_2@nxp.com> Cc: stable@vger.kernel.org Signed-off-by: Meng Li <Meng.Li@windriver.com> --- v1->v2: - improve commit log, correct the Fixes commit id. --- drivers/usb/dwc3/core.c | 6 ------ 1 file changed, 6 deletions(-)