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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] usb: chipidea: udc: check request status before setting device address
@ 2022-06-23  3:02 Xu Yang
  2022-07-03  0:42 ` Peter Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Xu Yang @ 2022-06-23  3:02 UTC (permalink / raw)
  To: peter.chen; +Cc: gregkh, linux-usb, jun.li, linux-imx

The complete() function may be called even though request is not
completed. 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] usb: chipidea: udc: check request status before setting device address
  2022-06-23  3:02 [PATCH] usb: chipidea: udc: check request status before setting device address Xu Yang
@ 2022-07-03  0:42 ` Peter Chen
  2022-07-04  1:59   ` [EXT] " Xu Yang
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Chen @ 2022-07-03  0:42 UTC (permalink / raw)
  To: Xu Yang; +Cc: gregkh, linux-usb, jun.li, linux-imx

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?

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXT] Re: [PATCH] usb: chipidea: udc: check request status before setting device address
  2022-07-03  0:42 ` Peter Chen
@ 2022-07-04  1:59   ` Xu Yang
  2022-07-09 10:00     ` Peter Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Xu Yang @ 2022-07-04  1:59 UTC (permalink / raw)
  To: Peter Chen; +Cc: gregkh, linux-usb, Jun Li, dl-linux-imx

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 or received other setup token rather IN token.
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);

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [EXT] Re: [PATCH] usb: chipidea: udc: check request status before setting device address
  2022-07-04  1:59   ` [EXT] " Xu Yang
@ 2022-07-09 10:00     ` Peter Chen
  2022-07-11  1:36       ` Xu Yang
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Chen @ 2022-07-09 10:00 UTC (permalink / raw)
  To: Xu Yang; +Cc: gregkh, linux-usb, Jun Li, dl-linux-imx

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXT] Re: [PATCH] usb: chipidea: udc: check request status before setting device address
  2022-07-09 10:00     ` Peter Chen
@ 2022-07-11  1:36       ` Xu Yang
  2022-07-12  1:22         ` Peter Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Xu Yang @ 2022-07-11  1:36 UTC (permalink / raw)
  To: Peter Chen; +Cc: gregkh, linux-usb, Jun Li, dl-linux-imx

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?

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [EXT] Re: [PATCH] usb: chipidea: udc: check request status before setting device address
  2022-07-11  1:36       ` Xu Yang
@ 2022-07-12  1:22         ` Peter Chen
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Chen @ 2022-07-12  1:22 UTC (permalink / raw)
  To: Xu Yang; +Cc: gregkh, linux-usb, Jun Li, dl-linux-imx

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-07-12  1:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-23  3:02 [PATCH] usb: chipidea: udc: check request status before setting device address Xu Yang
2022-07-03  0:42 ` Peter Chen
2022-07-04  1:59   ` [EXT] " Xu Yang
2022-07-09 10:00     ` Peter Chen
2022-07-11  1:36       ` Xu Yang
2022-07-12  1:22         ` Peter Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.