| Message ID | 20240916224543.187-1-selvarasu.g@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: dwc3: core: Stop processing of pending events if controller is halted | expand |
On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: > This commit addresses an issue where events were being processed when > the controller was in a halted state. To fix this issue by stop > processing the events as the event count was considered stale or > invalid when the controller was halted. > > Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") > Cc: stable <stable@kernel.org> Checkpatch doesn't like that format. Fix the Cc stable tag to below: Cc: stable@kernel.org > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > > Changes in v2: > - Updated the code changes that suggested by reviewer (Thinh Nguyen). > - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/lkml/20240719110100.329-1-selvarasu.g@samsung.com/__;!!A4F2R9G_pg!bSbBiyBT0X0C8o_Q7P_-KRqpOSZyigMiFe3hDcu78KnopsRHQrLIuOPVU3TZT-jqNHAn2z-U6c6SVEcmr3faZNMd3vU$ > --- > drivers/usb/dwc3/core.c | 22 +++++++++++++++++++--- > drivers/usb/dwc3/core.h | 4 ---- > drivers/usb/dwc3/gadget.c | 11 ----------- > 3 files changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 9eb085f359ce..65ae5203b89e 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,8 +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; > } > > @@ -584,7 +587,10 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) > dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(0), 0); > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), DWC3_GEVNTSIZ_INTMASK > | DWC3_GEVNTSIZ_SIZE(0)); > - 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); > } > > static void dwc3_core_num_eps(struct dwc3 *dwc) > @@ -2499,7 +2505,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: > @@ -2589,6 +2599,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 Fix replies -> relies > + * 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 c71240e8f7c7..9c508e0c5cdf 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1675,7 +1675,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) > { > @@ -1687,9 +1686,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 291bc549935b..10178e5eda5a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -4728,14 +4728,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); > - } > -} > -- > 2.17.1 > After the minor fixes, you can include this: Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On Mon, Sep 16, 2024 at 11:00:30PM +0000, Thinh Nguyen wrote: > On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: > > This commit addresses an issue where events were being processed when > > the controller was in a halted state. To fix this issue by stop > > processing the events as the event count was considered stale or > > invalid when the controller was halted. > > > > Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") > > Cc: stable <stable@kernel.org> > > Checkpatch doesn't like that format. Fix the Cc stable tag to below: > > Cc: stable@kernel.org What? Why? That should be fine, exactly what is the warning that this gives? That should be fine, as it's what my scripts put into patches that I create :) thanks, greg k-h
On Tue, Sep 17, 2024, gregkh@linuxfoundation.org wrote: > On Mon, Sep 16, 2024 at 11:00:30PM +0000, Thinh Nguyen wrote: > > On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: > > > This commit addresses an issue where events were being processed when > > > the controller was in a halted state. To fix this issue by stop > > > processing the events as the event count was considered stale or > > > invalid when the controller was halted. > > > > > > Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") > > > Cc: stable <stable@kernel.org> > > > > Checkpatch doesn't like that format. Fix the Cc stable tag to below: > > > > Cc: stable@kernel.org > > What? Why? That should be fine, exactly what is the warning that this > gives? That should be fine, as it's what my scripts put into patches > that I create :) > This is what checkpatch complains: WARNING:BAD_STABLE_ADDRESS_STYLE: Invalid email format for stable: 'stable <stable@kernel.org>', prefer 'stable@kernel.org' #23: Cc: stable <stable@kernel.org> total: 0 errors, 1 warnings, 0 checks, 72 lines checked BR, Thinh
On Tue, Sep 17, 2024 at 05:47:05AM +0000, Thinh Nguyen wrote: > On Tue, Sep 17, 2024, gregkh@linuxfoundation.org wrote: > > On Mon, Sep 16, 2024 at 11:00:30PM +0000, Thinh Nguyen wrote: > > > On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: > > > > This commit addresses an issue where events were being processed when > > > > the controller was in a halted state. To fix this issue by stop > > > > processing the events as the event count was considered stale or > > > > invalid when the controller was halted. > > > > > > > > Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") > > > > Cc: stable <stable@kernel.org> > > > > > > Checkpatch doesn't like that format. Fix the Cc stable tag to below: > > > > > > Cc: stable@kernel.org > > > > What? Why? That should be fine, exactly what is the warning that this > > gives? That should be fine, as it's what my scripts put into patches > > that I create :) > > > > This is what checkpatch complains: > > WARNING:BAD_STABLE_ADDRESS_STYLE: Invalid email format for stable: 'stable <stable@kernel.org>', prefer 'stable@kernel.org' > #23: > Cc: stable <stable@kernel.org> > > total: 0 errors, 1 warnings, 0 checks, 72 lines checked Ugh, that's wrong, whatever you want to do here is fine. Someone should send a patch for checkpatch... thanks, greg k-h
On 9/17/2024 12:38 PM, gregkh@linuxfoundation.org wrote: > On Tue, Sep 17, 2024 at 05:47:05AM +0000, Thinh Nguyen wrote: >> On Tue, Sep 17, 2024, gregkh@linuxfoundation.org wrote: >>> On Mon, Sep 16, 2024 at 11:00:30PM +0000, Thinh Nguyen wrote: >>>> On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: >>>>> This commit addresses an issue where events were being processed when >>>>> the controller was in a halted state. To fix this issue by stop >>>>> processing the events as the event count was considered stale or >>>>> invalid when the controller was halted. >>>>> >>>>> Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") >>>>> Cc: stable <stable@kernel.org> >>>> Checkpatch doesn't like that format. Fix the Cc stable tag to below: >>>> >>>> Cc: stable@kernel.org >>> What? Why? That should be fine, exactly what is the warning that this >>> gives? That should be fine, as it's what my scripts put into patches >>> that I create :) >>> >> This is what checkpatch complains: >> >> WARNING:BAD_STABLE_ADDRESS_STYLE: Invalid email format for stable: 'stable <stable@kernel.org>', prefer 'stable@kernel.org' >> #23: >> Cc: stable <stable@kernel.org> >> >> total: 0 errors, 1 warnings, 0 checks, 72 lines checked > Ugh, that's wrong, whatever you want to do here is fine. > > Someone should send a patch for checkpatch... > > thanks, > > greg k-h Hi Greg, However I already posted new version with address a typo mistake as well. Kindly review the same. https://lore.kernel.org/lkml/20240916231813.206-1-selvarasu.g@samsung.com/ Thanks, Selva >
On Tue, Sep 17, 2024 at 09:52:00PM +0530, Selvarasu Ganesan wrote: > > On 9/17/2024 12:38 PM, gregkh@linuxfoundation.org wrote: > > On Tue, Sep 17, 2024 at 05:47:05AM +0000, Thinh Nguyen wrote: > >> On Tue, Sep 17, 2024, gregkh@linuxfoundation.org wrote: > >>> On Mon, Sep 16, 2024 at 11:00:30PM +0000, Thinh Nguyen wrote: > >>>> On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: > >>>>> This commit addresses an issue where events were being processed when > >>>>> the controller was in a halted state. To fix this issue by stop > >>>>> processing the events as the event count was considered stale or > >>>>> invalid when the controller was halted. > >>>>> > >>>>> Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") > >>>>> Cc: stable <stable@kernel.org> > >>>> Checkpatch doesn't like that format. Fix the Cc stable tag to below: > >>>> > >>>> Cc: stable@kernel.org > >>> What? Why? That should be fine, exactly what is the warning that this > >>> gives? That should be fine, as it's what my scripts put into patches > >>> that I create :) > >>> > >> This is what checkpatch complains: > >> > >> WARNING:BAD_STABLE_ADDRESS_STYLE: Invalid email format for stable: 'stable <stable@kernel.org>', prefer 'stable@kernel.org' > >> #23: > >> Cc: stable <stable@kernel.org> > >> > >> total: 0 errors, 1 warnings, 0 checks, 72 lines checked > > Ugh, that's wrong, whatever you want to do here is fine. > > > > Someone should send a patch for checkpatch... > > > > thanks, > > > > greg k-h > > Hi Greg, > > However I already posted new version with address a typo mistake as > well. Kindly review the same. > > https://lore.kernel.org/lkml/20240916231813.206-1-selvarasu.g@samsung.com/ It's the middle of the merge window, I'll look at it after -rc1 is out, thanks. greg k-h
++Joe Perches, Dwaipayan Ray On Tue, Sep 17, 2024, gregkh@linuxfoundation.org wrote: > On Tue, Sep 17, 2024 at 05:47:05AM +0000, Thinh Nguyen wrote: > > On Tue, Sep 17, 2024, gregkh@linuxfoundation.org wrote: > > > On Mon, Sep 16, 2024 at 11:00:30PM +0000, Thinh Nguyen wrote: > > > > On Tue, Sep 17, 2024, Selvarasu Ganesan wrote: > > > > > This commit addresses an issue where events were being processed when > > > > > the controller was in a halted state. To fix this issue by stop > > > > > processing the events as the event count was considered stale or > > > > > invalid when the controller was halted. > > > > > > > > > > Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") > > > > > Cc: stable <stable@kernel.org> > > > > > > > > Checkpatch doesn't like that format. Fix the Cc stable tag to below: > > > > > > > > Cc: stable@kernel.org > > > > > > What? Why? That should be fine, exactly what is the warning that this > > > gives? That should be fine, as it's what my scripts put into patches > > > that I create :) > > > > > > > This is what checkpatch complains: > > > > WARNING:BAD_STABLE_ADDRESS_STYLE: Invalid email format for stable: 'stable <stable@kernel.org>', prefer 'stable@kernel.org' > > #23: > > Cc: stable <stable@kernel.org> > > > > total: 0 errors, 1 warnings, 0 checks, 72 lines checked > > Ugh, that's wrong, whatever you want to do here is fine. > > Someone should send a patch for checkpatch... > I included the maintainer Joe and Dwaipayan base on the below related change: fccaebf00e60 ("checkpatch: improve email parsing") Seems checkpatch doesn't like stable address that begins with a name? Hopefully they can help take a look. Thanks, Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 9eb085f359ce..65ae5203b89e 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,8 +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; } @@ -584,7 +587,10 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(0), 0); dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), DWC3_GEVNTSIZ_INTMASK | DWC3_GEVNTSIZ_SIZE(0)); - 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); } static void dwc3_core_num_eps(struct dwc3 *dwc) @@ -2499,7 +2505,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: @@ -2589,6 +2599,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 c71240e8f7c7..9c508e0c5cdf 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1675,7 +1675,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) { @@ -1687,9 +1686,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 291bc549935b..10178e5eda5a 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -4728,14 +4728,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); - } -}
This commit addresses an issue where events were being processed when the controller was in a halted state. To fix this issue by stop processing the events as the event count was considered stale or invalid when the controller was halted. Fixes: fc8bb91bc83e ("usb: dwc3: implement runtime PM") Cc: stable <stable@kernel.org> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- Changes in v2: - Updated the code changes that suggested by reviewer (Thinh Nguyen). - Link to v1: https://lore.kernel.org/lkml/20240719110100.329-1-selvarasu.g@samsung.com/ --- drivers/usb/dwc3/core.c | 22 +++++++++++++++++++--- drivers/usb/dwc3/core.h | 4 ---- drivers/usb/dwc3/gadget.c | 11 ----------- 3 files changed, 19 insertions(+), 18 deletions(-)