| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240905002611.rxlv66zsker2h5w2@synopsys.com>
Date: Thu, 5 Sep 2024 00:26:19 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Selvarasu Ganesan <selvarasu.g@...sung.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jh0801.jung@...sung.com" <jh0801.jung@...sung.com>,
"dh10.jung@...sung.com" <dh10.jung@...sung.com>,
"naushad@...sung.com" <naushad@...sung.com>,
"akash.m5@...sung.com" <akash.m5@...sung.com>,
"rc93.raju@...sung.com" <rc93.raju@...sung.com>,
"taehyun.cho@...sung.com" <taehyun.cho@...sung.com>,
"hongpooh.kim@...sung.com" <hongpooh.kim@...sung.com>,
"eomji.oh@...sung.com" <eomji.oh@...sung.com>,
"shijie.cai@...sung.com" <shijie.cai@...sung.com>
Subject: Re: [PATCH] usb: dwc3: Potential fix of possible dwc3 interrupt storm
On Wed, Sep 04, 2024, Selvarasu Ganesan wrote:
>
> On 9/4/2024 6:33 AM, Thinh Nguyen wrote:
> > On Mon, Sep 02, 2024, Selvarasu Ganesan wrote:
> >> I would like to reconfirm from our end that in our failure scenario, we
> >> observe that DWC3_EVENT_PENDING is set in evt->flags when the dwc3
> >> resume sequence is executed, and the dwc->pending_events flag is not
> >> being set.
> >>
> > If the controller is stopped, no event is generated until it's restarted
> > again. (ie, you should not see GEVNTCOUNT updated after clearing
> > DCTL.run_stop). If there's no event, no interrupt assertion should come
> > from the controller.
> >
> > If the pending_events is not set and you still see this failure, then
> > likely that the controller had started, and the interrupt is generated
> > from the controller event. This occurs along with the interrupt
> > generated from your connection notification from your setup.
>
>
> I completely agree. My discussion revolves around the handling of the
> DWC3_EVENT_PENDING flag in all situations. The purpose of using this
> flag is to prevent the processing of new events if an existing event is
> still being processed. This flag is set in the top-half interrupt
> handler and cleared at the end of the bottom-half handler.
>
> Now, let's consider scenarios where the bottom half is not scheduled,
> and a USB reconnect occurs. In this case, there is a possibility that
> the interrupt line is unmasked in dwc3_event_buffers_setup, and the USB
> controller begins posting new events. The top-half interrupt handler
> checks for the DWC3_EVENT_PENDING flag and returns IRQ_HANDLED without
> processing any new events. However, the USB controller continues to post
> interrupts until they are acknowledged.
>
> Please review the complete sequence once with DWC3_EVENT_PENDING flag.
>
> My proposal is to clear or reset the DWC3_EVENT_PENDING flag when
> unmasking the interrupt line dwc3_event_buffers_setup, apart from
> bottom-half handler. Clearing the DWC3_EVENT_PENDING flag in
> dwc3_event_buffers_setup does not cause any harm, as we have implemented
> a temporary workaround in our test setup to prevent IRQ storms.
>
>
>
> Working scenarios:
> ==================
> 1. Top-half handler:
> a. if (evt->flags & DWC3_EVENT_PENDING)
> return IRQ_HANDLED;
> b. Set DWC3_EVENT_PENDING flag
> c. Masking interrupt line
>
> 2. Bottom-half handler:
> a. Un-masking interrupt line
> b. Clear DWC3_EVENT_PENDING flag
>
> Failure scenarios:
> ==================
> 1. Top-half handler:
> a. if (evt->flags & DWC3_EVENT_PENDING)
> return IRQ_HANDLED;
> b. Set DWC3_EVENT_PENDING flag
> c. Masking interrupt line
For DWC3_EVENT_PENDING flag to be set at this point (before we start the
controller), that means that the GEVNTCOUNT was not 0 after
soft-disconnect and that the pm_runtime_suspended() must be false.
>
> 2. No Bottom-half scheduled:
Why is the bottom-half not scheduled? Or do you mean it hasn't woken up
yet before the next top-half coming?
>
> 3. USB reconnect: dwc3_event_buffers_setup
> a. Un-masking interrupt line
Do we know that the GEVNTCOUNT is non-zero before starting the
controller again?
>
> 4. Continuous interrupts : Top-half handler:
> a. if (evt->flags & DWC3_EVENT_PENDING)
> return IRQ_HANDLED;
>
> a. if (evt->flags & DWC3_EVENT_PENDING)
> return IRQ_HANDLED;
>
> a. if (evt->flags & DWC3_EVENT_PENDING)
> return IRQ_HANDLED;
> .....
>
> .....
>
> .....
>
I don't want the driver to process any stale event after a
soft-disconnect. Can we try this instead:
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index ccc3895dbd7f..a1e6ba92e808 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -544,6 +544,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length)
int dwc3_event_buffers_setup(struct dwc3 *dwc)
{
struct dwc3_event_buffer *evt;
+ u32 reg;
if (!dwc->ev_buf)
return 0;
@@ -556,7 +557,10 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc)
upper_32_bits(evt->dma));
dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
DWC3_GEVNTSIZ_SIZE(evt->length));
- dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0);
+
+ /* Clear any stale event */
+ reg = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
+ dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), reg);
return 0;
}
@@ -2484,7 +2488,11 @@ static int dwc3_runtime_resume(struct device *dev)
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
- dwc3_gadget_process_pending_events(dwc);
+ if (dwc->pending_events) {
+ pm_runtime_put(dwc->dev);
+ dwc->pending_events = false;
+ enable_irq(dwc->irq_gadget);
+ }
break;
case DWC3_GCTL_PRTCAP_HOST:
default:
@@ -2574,6 +2582,12 @@ static void dwc3_complete(struct device *dev)
static const struct dev_pm_ops dwc3_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
.complete = dwc3_complete,
+
+ /*
+ * Runtime suspend halts the controller on disconnection. It replies on
+ * platforms with custom connection notification to start the controller
+ * again.
+ */
SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
dwc3_runtime_idle)
};
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1e561fd8b86e..2fa3fd55eb32 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1673,7 +1673,6 @@ static inline void dwc3_otg_host_init(struct dwc3 *dwc)
#if !IS_ENABLED(CONFIG_USB_DWC3_HOST)
int dwc3_gadget_suspend(struct dwc3 *dwc);
int dwc3_gadget_resume(struct dwc3 *dwc);
-void dwc3_gadget_process_pending_events(struct dwc3 *dwc);
#else
static inline int dwc3_gadget_suspend(struct dwc3 *dwc)
{
@@ -1684,10 +1683,6 @@ static inline int dwc3_gadget_resume(struct dwc3 *dwc)
{
return 0;
}
-
-static inline void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
-{
-}
#endif /* !IS_ENABLED(CONFIG_USB_DWC3_HOST) */
#if IS_ENABLED(CONFIG_USB_DWC3_ULPI)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 89fc690fdf34..a525f7ea5886 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4490,6 +4490,17 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
return IRQ_HANDLED;
count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
+
+ /*
+ * If the controller is halted, the event count is stale/invalid. Ignore
+ * them. This happens if the interrupt assertion is from an out-of-band
+ * resume notification.
+ */
+ if (!dwc->pullups_connected && count) {
+ dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
+ return IRQ_HANDLED;
+ }
+
count &= DWC3_GEVNTCOUNT_MASK;
if (!count)
return IRQ_NONE;
@@ -4735,14 +4746,3 @@ int dwc3_gadget_resume(struct dwc3 *dwc)
return dwc3_gadget_soft_connect(dwc);
}
-
-void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
-{
- if (dwc->pending_events) {
- dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
- dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
- pm_runtime_put(dwc->dev);
- dwc->pending_events = false;
- enable_irq(dwc->irq_gadget);
- }
-}
---
Thanks,
Thinh
Powered by blists - more mailing lists