[PATCH] USB: ci: gadget: Panic if priming when gadget off

[John Ernberg]

---
 drivers/usb/chipidea/udc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 2fea263a5e30..544aa4fa2d1d 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -203,8 +203,10 @@ static int hw_ep_prime(struct ci_hdrc *ci, int num, in=
t dir, int is_ctrl)
=20
    hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n));
=20
-   while (hw_read(ci, OP_ENDPTPRIME, BIT(n)))
+   while (hw_read(ci, OP_ENDPTPRIME, BIT(n))) {
        cpu_relax();
+       BUG_ON(dir =3D=3D TX && !hw_read(ci, OP_ENDPTCTRL + num, ENDPTCTRL_=
TXE));
+   }
    if (is_ctrl && dir =3D=3D RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)=
))
        return -EAGAIN;
=20
----------------->8------------------

On the iMX8QXP you may additionally run into asychronous aborts and SError
due to resource being disabled.

> >=20
> > ---8<--------------------
> >=20
> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > index 8a9b31fd5c89..72329a7eac4d 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -2374,6 +2374,9 @@ static void udc_suspend(struct ci_hdrc *ci)
> >          */
> >         if (hw_read(ci, OP_ENDPTLISTADDR, ~0) =3D=3D 0)
> >                 hw_write(ci, OP_ENDPTLISTADDR, ~0, ~0);
> > +
> > +       if (ci->driver && ci->vbus_active && (ci->gadget.state !=3D USB=
_STATE_SUSPENDED))
> > +               usb_gadget_disconnect(&ci->gadget);
> >  }
> > =20
> >  static void udc_resume(struct ci_hdrc *ci, bool power_lost)
> > @@ -2384,6 +2387,9 @@ static void udc_resume(struct ci_hdrc *ci, bool p=
ower_lost)
> >                                         OTGSC_BSVIS | OTGSC_BSVIE);
> >                 if (ci->vbus_active)
> >                         usb_gadget_vbus_disconnect(&ci->gadget);
> > +       } else {
> > +               if (ci->driver && ci->vbus_active)
> > +                       usb_gadget_connect(&ci->gadget);
> >         }
> > =20
> >         /* Restore value 0 if it was set for power lost check */
> >=20
> > ---->8------------------
>=20
> During the scp process, the usb host won't put usb device to suspend stat=
e.
> In current design, then the ether driver doesn't know the system has
> suspended after echo mem. The root cause is that ether driver is still tr=
ing
> to queue usb request after usb controller has suspended where usb clock i=
s off,
> then the system hang.
>=20
> With the above changes, I think the ether driver will fail to eth_start_x=
mit()=20
> at an ealier stage, so the issue can't be triggered.
>=20
> I think the ether driver needs call gether_suspend() accordingly, to do t=
his,
> the controller driver need explicitly call suspend() function when it's g=
oing
> to be suspended. Could you check whether below patch fix the issue?
>=20
>  ---8<--------------------
>=20
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 8a9b31fd5c89..27a7674ed62c 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -2367,6 +2367,8 @@ static void udc_id_switch_for_host(struct ci_hdrc *=
ci)
>  #ifdef CONFIG_PM_SLEEP
>  static void udc_suspend(struct ci_hdrc *ci)
>  {
> +       ci->driver->suspend(&ci->gadget);
> +
>         /*
>          * Set OP_ENDPTLISTADDR to be non-zero for
>          * checking if controller resume from power lost
> @@ -2389,6 +2391,8 @@ static void udc_resume(struct ci_hdrc *ci, bool pow=
er_lost)
>         /* Restore value 0 if it was set for power lost check */
>         if (hw_read(ci, OP_ENDPTLISTADDR, ~0) =3D=3D 0xFFFFFFFF)
>                 hw_write(ci, OP_ENDPTLISTADDR, ~0, 0);
> +
> +       ci->driver->resume(&ci->gadget);
>  }
>  #endif
>=20
>  ---->8------------------

I tested this during my debugging and it doesn't work because suspend/resum=
e
callbacks on the gadgets are designed for USB triggered suspend/resume and
not system triggered suspend/resume. Meaning that the link will just be
woken up again by the next USB transfer.

>=20
> Thanks,
> Xu Yang
>=20
> >=20
> > But it's unclear to me why the hangup happens and how the change above
> > fix the problem.  Do you guys have any insight here?o
> >=20
> > Shawn
> >=20
> > [1] https://github.com/reMarkable/linux-imx/commit/0791d25578cb0e46fd93=
ae7a3c36ff7a424f3547
> >=20

I didn't find the missing lines of code that Shawn found and instead ended
up looking at why the UDC core isn't suspending the gadgets when the system
is going to suspend. Because to me it feels like a job of UDC core.

I ended up with the monstrosity below that I have been intended to send as
an RFC when I'm done thinking about it. It currently applies on 6.12.20.

But since Shawn also ran into the problem I'm including it for the sake of
discussion about what the correct path of solving it is.

Best regards // John Ernberg

----------------->8------------------