Re: [EXT] Re: [PATCH] usb: chipidea: udc: check request status before setting device address

[Peter Chen <peter.chen@kernel.org>]

On 22-07-04 01:59:45, Xu Yang wrote:
> Hi Peter,
> 
> > -----Original Message-----
> > From: Peter Chen <peter.chen@kernel.org>
> > Sent: Sunday, July 3, 2022 8:42 AM
> > To: Xu Yang <xu.yang_2@nxp.com>
> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; Jun Li
> > <jun.li@nxp.com>; dl-linux-imx <linux-imx@nxp.com>
> > Subject: [EXT] Re: [PATCH] usb: chipidea: udc: check request status before
> > setting device address
> > 
> > Caution: EXT Email
> > 
> > On 22-06-23 11:02:42, Xu Yang wrote:
> > > The complete() function may be called even though request is not
> > > completed.
> > 
> > Would you please explain more when the complete() is called but the
> > request has not completed, and this happens before set_address has
> > finished?
> 
> The following sequence will be a case:
> 1. the HOST sent SET_ADDRESS control message to DEVICE.
> 2. the DEVICE acked this message and queued a usb_request (0 data length) for future IN transfer.
> 3. somehow the setup interrupt is not cleared by USB controller


You mean even software clear the OP_ENDPTSETUPSTAT bits, but controller
itself doesn't clear its interrupt, and trigger the same
interrupt handler again?

> or received other setup token rather IN token.
That's possible. Have you found that at bus analyzer?


> 4. the driver will go through below functions:
> 
> isr_setup_packet_handler(ci);
>   _ep_nuke(ci->ep0in);
>     usb_gadget_giveback_request(&hwep->ep, &hwreq->req);
>       req->complete(ep, req);
>         isr_setup_status_complete();
>           hw_usb_set_address(ci, ci->address);
>             usb_gadget_set_state(&ci->gadget, USB_STATE_ADDRESS);
> 

Would you please add your above description and send patch again?

Peter
> Xu Yang
> 
> > 
> > Peter
> > 
> > > In this case, it's necessary to check request status so as not to set
> > > device address wrongly.
> > >
> > > Fixes: 10775eb17bee ("usb: chipidea: udc: update gadget states
> > > according to ch9")
> > > cc: <stable@vger.kernel.org>
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > ---
> > >  drivers/usb/chipidea/udc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > > index 0c9ae9768a67..8c3e3a635ac2 100644
> > > --- a/drivers/usb/chipidea/udc.c
> > > +++ b/drivers/usb/chipidea/udc.c
> > > @@ -1048,6 +1048,9 @@ isr_setup_status_complete(struct usb_ep *ep,
> > struct usb_request *req)
> > >       struct ci_hdrc *ci = req->context;
> > >       unsigned long flags;
> > >
> > > +     if (req->status < 0)
> > > +             return;
> > > +
> > >       if (ci->setaddr) {
> > >               hw_usb_set_address(ci, ci->address);
> > >               ci->setaddr = false;
> > > --
> > > 2.25.1
> > >
> > 
> > --
> > 
> > Thanks,
> > Peter Chen
> 

-- 

Thanks,
Peter Chen