[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240904053052.GA8@f902c60e4871>
Date: Wed, 4 Sep 2024 07:30:52 +0200
From: Tomas Marek <tomas.marek@...est.cz>
To: Sergey Shtylyov <s.shtylyov@....ru>
Cc: hminas@...opsys.com, gregkh@...uxfoundation.org,
Arthur.Petrosyan@...opsys.com, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, oleg.karfich@...o.com
Subject: Re: [PATCH] usb: dwc2: drd: fix clock gating on USB role switch
Hi Sergey,
thank you for your review.
On Tue, Sep 03, 2024 at 04:20:26PM +0300, Sergey Shtylyov wrote:
> On 9/3/24 12:41 PM, Tomas Marek wrote:
>
> > The dwc2_handle_usb_suspend_intr() function disables gadget clocks in USB
> > peripheral mode when no other power-down mode is available (introduced by
> > commit 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")).
> > However, the dwc2_drd_role_sw_set() USB role update handler attempts to
> > read DWC2 registers if the USB role has changed while the USB is in suspend
> > mode (when the clocks are gated). This causes the system to hang.
> >
> > Release gadget clocks before handling the USB role update.
> >
> > Fixes: 0112b7ce68ea ("usb: dwc2: Update dwc2_handle_usb_suspend_intr function.")
> >
> > Signed-off-by: Tomas Marek <tomas.marek@...est.cz>
> > ---
> > drivers/usb/dwc2/drd.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> > index a8605b02115b..ccb33cd1f04b 100644
> > --- a/drivers/usb/dwc2/drd.c
> > +++ b/drivers/usb/dwc2/drd.c
> > @@ -127,6 +127,18 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
> > role = USB_ROLE_DEVICE;
> > }
> >
> > +#if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
> > + IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
> > + if (dwc2_is_device_mode(hsotg)) {
>
> Why not avoid #ifdef by doing e.g.:
>
> if ((IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
> IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) &&
> dwc2_is_device_mode(hsotg)) {
>
> > + if (hsotg->lx_state == DWC2_L2) {
> > + if (hsotg->params.power_down ==
> > + DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended &&
> > + !hsotg->params.no_clock_gating)
>
> Well, these *if*s are mergeable too... :-)
>
OK, why not. In the end all conditions could be merged into one if statement like:
if ((IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) ||
IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)) &&
dwc2_is_device_mode(hsotg) &&
hsotg->lx_state == DWC2_L2 &&
hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE &&
hsotg->bus_suspended &&
!hsotg->params.no_clock_gating)
dwc2_gadget_exit_clock_gating(hsotg, 0);
I’ll submit a new patch version with this modification.
Best regards
Tomas
> > + dwc2_gadget_exit_clock_gating(hsotg, 0);
> > + }
> > + }
> > +#endif
> > +
> [...]
>
> MBR, Sergey
Powered by blists - more mailing lists