| Message ID | 20220623030242.41796-1-xu.yang_2@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: chipidea: udc: check request status before setting device address | expand |
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 >
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 >
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
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 >
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;
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(+)