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

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

On 22-07-11 01:36:39, Xu Yang wrote:
> Hi Peter,
> 
> > -----Original Message-----
> > From: Peter Chen <peter.chen@kernel.org>
> > Sent: Saturday, July 9, 2022 6:00 PM
> > 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: Re: [EXT] Re: [PATCH] usb: chipidea: udc: check request status
> > before setting device address
> > 
> > Caution: EXT Email
> > 
> > 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?
> 
> Yes, still not sure the root cause of this werid behavior. Will dig it out later.
> 
> > 
> > > or received other setup token rather IN token.
> > That's possible. Have you found that at bus analyzer?
> 
> Not yet.
> 
> > 
> > 
> > > 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?
> 
> This patch has been received by Greg. Do I still need to send patch again?
> 

That's ok. Thanks for clarifying above.

Peter

> Xu Yang
> 
> > 
> > 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
> 

-- 

Thanks,
Peter Chen