diff mbox series

usb: cdnsp: Fix incorrect status for control request

Message ID 20211207091838.39572-1-pawell@gli-login.cadence.com
State New
Headers show
Series usb: cdnsp: Fix incorrect status for control request | expand

Commit Message

Pawel Laszczak Dec. 7, 2021, 9:18 a.m. UTC
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(+)

Comments

Peter Chen Dec. 13, 2021, 1:02 p.m. UTC | #1
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?
Pawel Laszczak Dec. 13, 2021, 1:17 p.m. UTC | #2
>
>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
Peter Chen Dec. 13, 2021, 1:31 p.m. UTC | #3
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 mbox series

Patch

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);
 }