| Message ID | 20211207091838.39572-1-pawell@gli-login.cadence.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: cdnsp: Fix incorrect status for control request | expand |
On 21-12-09 12:57:31, Pawel Laszczak wrote: > >> From: Pawel Laszczak <pawell@cadence.com> > >> > >> Patch fixes incorrect status for control request. > >> Without this fix all usb_request objects were returned to upper drivers > >> with usb_reqest->status field set to -EINPROGRESS. > >> > >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") > >> Reported-by: Ken (Jian) He <jianhe@ambarella.com> > >> cc: <stable@vger.kernel.org> > >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> > >> --- > >> drivers/usb/cdns3/cdnsp-ring.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > >> index 1b1438457fb0..e8f5ecbb5c75 100644 > >> --- a/drivers/usb/cdns3/cdnsp-ring.c > >> +++ b/drivers/usb/cdns3/cdnsp-ring.c > >> @@ -1029,6 +1029,8 @@ static void cdnsp_process_ctrl_td(struct cdnsp_device *pdev, > >> return; > >> } > >> > >> + *status = 0; > >> + > >> cdnsp_finish_td(pdev, td, event, pep, status); > >> } > >> > >> -- > >I think you may move *status = 0 at the beginning of > >cdnsp_process_ctrl_td in case you would like to handle some error > >conditions during this function. > > I don't predict any other status code for control request in this place. > I wanted to set this status only once after completion status stage. It was the reason why I put this > statement at the end of function. So, you always consider there is no error for control request handling?
> >On 21-12-09 12:57:31, Pawel Laszczak wrote: >> >> From: Pawel Laszczak <pawell@cadence.com> >> >> >> >> Patch fixes incorrect status for control request. >> >> Without this fix all usb_request objects were returned to upper drivers >> >> with usb_reqest->status field set to -EINPROGRESS. >> >> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") >> >> Reported-by: Ken (Jian) He <jianhe@ambarella.com> >> >> cc: <stable@vger.kernel.org> >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> >> >> --- >> >> drivers/usb/cdns3/cdnsp-ring.c | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c >> >> index 1b1438457fb0..e8f5ecbb5c75 100644 >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c >> >> @@ -1029,6 +1029,8 @@ static void cdnsp_process_ctrl_td(struct cdnsp_device *pdev, >> >> return; >> >> } >> >> >> >> + *status = 0; >> >> + >> >> cdnsp_finish_td(pdev, td, event, pep, status); >> >> } >> >> >> >> -- >> >I think you may move *status = 0 at the beginning of >> >cdnsp_process_ctrl_td in case you would like to handle some error >> >conditions during this function. >> >> I don't predict any other status code for control request in this place. >> I wanted to set this status only once after completion status stage. It was the reason why I put this >> statement at the end of function. > >So, you always consider there is no error for control request handling? Not exactly. In this place driver handles only case when status stage has been completed, so control transfer has been finished successfully. Driver still take into account for control transfer such errors as: STALL, EINVAL and ECONNRESET but in other parts of source code. -- Regards, Pawel Laszczak
On 21-12-13 13:17:53, Pawel Laszczak wrote: > > > >On 21-12-09 12:57:31, Pawel Laszczak wrote: > >> >> From: Pawel Laszczak <pawell@cadence.com> > >> >> > >> >> Patch fixes incorrect status for control request. > >> >> Without this fix all usb_request objects were returned to upper drivers > >> >> with usb_reqest->status field set to -EINPROGRESS. > >> >> > >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver") > >> >> Reported-by: Ken (Jian) He <jianhe@ambarella.com> > >> >> cc: <stable@vger.kernel.org> > >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> > >> >> --- > >> >> drivers/usb/cdns3/cdnsp-ring.c | 2 ++ > >> >> 1 file changed, 2 insertions(+) > >> >> > >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c > >> >> index 1b1438457fb0..e8f5ecbb5c75 100644 > >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c > >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c > >> >> @@ -1029,6 +1029,8 @@ static void cdnsp_process_ctrl_td(struct cdnsp_device *pdev, > >> >> return; > >> >> } > >> >> > >> >> + *status = 0; > >> >> + > >> >> cdnsp_finish_td(pdev, td, event, pep, status); > >> >> } > >> >> > >> >> -- > >> >I think you may move *status = 0 at the beginning of > >> >cdnsp_process_ctrl_td in case you would like to handle some error > >> >conditions during this function. > >> > >> I don't predict any other status code for control request in this place. > >> I wanted to set this status only once after completion status stage. It was the reason why I put this > >> statement at the end of function. > > > >So, you always consider there is no error for control request handling? > > Not exactly. In this place driver handles only case when status stage has been completed, > so control transfer has been finished successfully. > > Driver still take into account for control transfer such errors as: STALL, EINVAL and ECONNRESET but > in other parts of source code. > Reviewed-by: Peter Chen <peter.chen@kernel.org>
diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c index 1b1438457fb0..e8f5ecbb5c75 100644 --- a/drivers/usb/cdns3/cdnsp-ring.c +++ b/drivers/usb/cdns3/cdnsp-ring.c @@ -1029,6 +1029,8 @@ static void cdnsp_process_ctrl_td(struct cdnsp_device *pdev, return; } + *status = 0; + cdnsp_finish_td(pdev, td, event, pep, status); }