diff mbox series

[v2] usb: cdnsp: fix issue with ZLP - added TD_SIZE = 1

Message ID 1666620275-139704-1-git-send-email-pawell@cadence.com
State New
Headers
Series [v2] usb: cdnsp: fix issue with ZLP - added TD_SIZE = 1 |

Commit Message

Pawel Laszczak Oct. 24, 2022, 2:04 p.m. UTC 
  Patch modifies the TD_SIZE in TRB before ZLP TRB.
The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
processing ZLP TRB by controller.

cc: <stable@vger.kernel.org>
Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
Signed-off-by: Pawel Laszczak <pawell@cadence.com>

---
Changelog:
v2:
- returned value for last TRB must be 0

 drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Peter Chen Oct. 27, 2022, 7:24 a.m. UTC | #1 
On 22-10-24 10:04:35, Pawel Laszczak wrote:
> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
> processing ZLP TRB by controller.
> 
> cc: <stable@vger.kernel.org>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> 
> ---
> Changelog:
> v2:
> - returned value for last TRB must be 0
> 
>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index 04dfcaa08dc4..aa79bce89d8a 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct cdnsp_device *pdev,
>  
>  	/* One TRB with a zero-length data packet. */
>  	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> -	    trb_buff_len == td_total_len)
> +	    trb_buff_len == td_total_len) {
> +		/* Before ZLP driver needs set TD_SIZE=1. */
> +		if (more_trbs_coming)
> +			return 1;
> +
>  		return 0;
> +	}

Does that fix the issue you want at bulk transfer, which has zero-length
packet at the last packet? It seems not align with your previous fix.
Would you mind explaining more?

>  
>  	maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>  	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> -- 
> 2.25.1
>
Pawel Laszczak Oct. 27, 2022, 8:46 a.m. UTC | #2 
>
>On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force processing
>> ZLP TRB by controller.
>>
>> cc: <stable@vger.kernel.org>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>>
>> ---
>> Changelog:
>> v2:
>> - returned value for last TRB must be 0
>>
>>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
>> 100644
>> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> cdnsp_device *pdev,
>>
>>  	/* One TRB with a zero-length data packet. */
>>  	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> -	    trb_buff_len == td_total_len)
>> +	    trb_buff_len == td_total_len) {
>> +		/* Before ZLP driver needs set TD_SIZE=1. */
>> +		if (more_trbs_coming)
>> +			return 1;
>> +
>>  		return 0;
>> +	}
>
>Does that fix the issue you want at bulk transfer, which has zero-length packet
>at the last packet? It seems not align with your previous fix.
>Would you mind explaining more?

Value returned by function cdnsp_td_remainder is used 
as TD_SIZE in TRB.

The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one
TRB then the controller stops the transfer and ignore trb for ZLP packet.

To fix this, the driver in such case must set TD_SIZE = 1
before the last TRB. 

e.g.

TD -> TRB1  transfer_length = 64KB, TD_SIZE =0
          TRB2 transfer_length =0, TD_SIZE = 0  - controller will
		    ignore this transfer and stop transfer on previous one

TD -> TRB1  transfer_length = 64KB, TD_SIZE =1
          TRB2 transfer_length =0, TD_SIZE = 0  - controller will
		    execute this trb and send ZLP

As you noticed previously, previous fix for last TRB returned
TD_SIZE = 1 in some cases.
Previous fix was working correct but was not compliance with
controller specification.

>
>>
>>  	maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>>  	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
>> --
>> 2.25.1
>>
>
>--
>

Thanks,
Pawel Laszczak
Peter Chen Nov. 6, 2022, 9:02 a.m. UTC | #3 
On 22-10-27 08:46:17, Pawel Laszczak wrote:
> 
> >
> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force processing
> >> ZLP TRB by controller.
> >>
> >> cc: <stable@vger.kernel.org>
> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> USBSSP DRD Driver")
> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> >>
> >> ---
> >> Changelog:
> >> v2:
> >> - returned value for last TRB must be 0
> >>
> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
> >> 100644
> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
> >> cdnsp_device *pdev,
> >>
> >>  	/* One TRB with a zero-length data packet. */
> >>  	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> -	    trb_buff_len == td_total_len)
> >> +	    trb_buff_len == td_total_len) {
> >> +		/* Before ZLP driver needs set TD_SIZE=1. */
> >> +		if (more_trbs_coming)
> >> +			return 1;
> >> +
> >>  		return 0;
> >> +	}
> >
> >Does that fix the issue you want at bulk transfer, which has zero-length packet
> >at the last packet? It seems not align with your previous fix.
> >Would you mind explaining more?
> 
> Value returned by function cdnsp_td_remainder is used 
> as TD_SIZE in TRB.
> 
> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
> set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one
> TRB then the controller stops the transfer and ignore trb for ZLP packet.
> 
> To fix this, the driver in such case must set TD_SIZE = 1
> before the last TRB. 

  	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
 -	    trb_buff_len == td_total_len)
 +	    trb_buff_len == td_total_len) {
 +		/* Before ZLP driver needs set TD_SIZE=1. */
 +		if (more_trbs_coming)
 +			return 1;
 +
  		return 0;
 +	}

How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
Which conditions are satisfied?

Peter

> e.g.
> 
> TD -> TRB1  transfer_length = 64KB, TD_SIZE =0
>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
> 		    ignore this transfer and stop transfer on previous one
> 
> TD -> TRB1  transfer_length = 64KB, TD_SIZE =1
>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
> 		    execute this trb and send ZLP
> 
> As you noticed previously, previous fix for last TRB returned
> TD_SIZE = 1 in some cases.
> Previous fix was working correct but was not compliance with
> controller specification.
> 
> >
> >>
> >>  	maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
> >>  	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> >> --
> >> 2.25.1
> >>
> >
> >--
> >
> 
> Thanks,
> Pawel Laszczak
Pawel Laszczak Nov. 7, 2022, 5:39 a.m. UTC | #4 
>
>
>On 22-10-27 08:46:17, Pawel Laszczak wrote:
>>
>> >
>> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> processing ZLP TRB by controller.
>> >>
>> >> cc: <stable@vger.kernel.org>
>> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> >> USBSSP DRD Driver")
>> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> >>
>> >> ---
>> >> Changelog:
>> >> v2:
>> >> - returned value for last TRB must be 0
>> >>
>> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
>> >> 100644
>> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> cdnsp_device *pdev,
>> >>
>> >>  	/* One TRB with a zero-length data packet. */
>> >>  	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> >> -	    trb_buff_len == td_total_len)
>> >> +	    trb_buff_len == td_total_len) {
>> >> +		/* Before ZLP driver needs set TD_SIZE=1. */
>> >> +		if (more_trbs_coming)
>> >> +			return 1;
>> >> +
>> >>  		return 0;
>> >> +	}
>> >
>> >Does that fix the issue you want at bulk transfer, which has
>> >zero-length packet at the last packet? It seems not align with your previous
>fix.
>> >Would you mind explaining more?
>>
>> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
>> TRB.
>>
>> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
>> set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one TRB
>> then the controller stops the transfer and ignore trb for ZLP packet.
>>
>> To fix this, the driver in such case must set TD_SIZE = 1 before the
>> last TRB.
>
>  	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> -	    trb_buff_len == td_total_len)
> +	    trb_buff_len == td_total_len) {
> +		/* Before ZLP driver needs set TD_SIZE=1. */
> +		if (more_trbs_coming)
> +			return 1;
> +
>  		return 0;
> +	}
>
>How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>Which conditions are satisfied?

For last non-ZLP TRB TD_SIZE should be 0 or 1.

We have three casess: 
1.  
	TRB1 - length > 1
	TRb2 - ZLP

In this case TRB1 should have set TD_SIZE = 1. In this case the condition
	if (more_trbs_coming)
		return 1;

returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for TRB2 is 0


2. 
	TRB1 - length >1 and we don't except ZLP

In this case TD_SIZE should be set to 0 for TRB1 and function returns 0, more_trbs_comming for TRB1 will be set to 0.

3 More TRBs without ZLP:
	e.g.
	TRB1  -  length > 0,  more_trbs_comming = 1 - TD_SIZE  > 0
	TRB2 -  length > 0, more_trbs_comming = 1  - TD_SIZE > 0
	TRB3 - length >= 0, more_trbs_comming = 0 -  TD_SIZE  = 0

Pawel

>
>Peter
>
>> e.g.
>>
>> TD -> TRB1  transfer_length = 64KB, TD_SIZE =0
>>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
>> 		    ignore this transfer and stop transfer on previous one
>>
>> TD -> TRB1  transfer_length = 64KB, TD_SIZE =1
>>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
>> 		    execute this trb and send ZLP
>>
>> As you noticed previously, previous fix for last TRB returned TD_SIZE
>> = 1 in some cases.
>> Previous fix was working correct but was not compliance with
>> controller specification.
>>
>> >
>> >>
>> >>  	maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>> >>  	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
>> >> --
>> >> 2.25.1
>> >>
>> >
>> >--
>> >
>>
>> Thanks,
>> Pawel Laszczak
>
>--
>
>Thanks,
>Peter Chen

Regards,
Pawel Laszczak
Peter Chen Nov. 10, 2022, 1:02 a.m. UTC | #5 
On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <pawell@cadence.com> wrote:
>
> >
> >
> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
> >>
> >> >
> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
> >> >> processing ZLP TRB by controller.
> >> >>
> >> >> cc: <stable@vger.kernel.org>
> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> >> USBSSP DRD Driver")
> >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> >> >>
> >> >> ---
> >> >> Changelog:
> >> >> v2:
> >> >> - returned value for last TRB must be 0
> >> >>
> >> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
> >> >> 100644
> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
> >> >> cdnsp_device *pdev,
> >> >>
> >> >>   /* One TRB with a zero-length data packet. */
> >> >>   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> >> -     trb_buff_len == td_total_len)
> >> >> +     trb_buff_len == td_total_len) {
> >> >> +         /* Before ZLP driver needs set TD_SIZE=1. */
> >> >> +         if (more_trbs_coming)
> >> >> +                 return 1;
> >> >> +
> >> >>           return 0;
> >> >> + }
> >> >
> >> >Does that fix the issue you want at bulk transfer, which has
> >> >zero-length packet at the last packet? It seems not align with your previous
> >fix.
> >> >Would you mind explaining more?
> >>
> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
> >> TRB.
> >>
> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
> >> set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one TRB
> >> then the controller stops the transfer and ignore trb for ZLP packet.
> >>
> >> To fix this, the driver in such case must set TD_SIZE = 1 before the
> >> last TRB.
> >
> >       if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> > -         trb_buff_len == td_total_len)
> > +         trb_buff_len == td_total_len) {
> > +             /* Before ZLP driver needs set TD_SIZE=1. */
> > +             if (more_trbs_coming)
> > +                     return 1;
> > +
> >               return 0;
> > +     }
> >
> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
> >Which conditions are satisfied?
>
> For last non-ZLP TRB TD_SIZE should be 0 or 1.
>
> We have three casess:
> 1.
>         TRB1 - length > 1
>         TRb2 - ZLP
>
> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
>         if (more_trbs_coming)
>                 return 1;
>
> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for TRB2 is 0
>

This one is my question. How below condition is true for your case 1:

 if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
          trb_buff_len == td_total_len)


Peter



>
> 2.
>         TRB1 - length >1 and we don't except ZLP
>
> In this case TD_SIZE should be set to 0 for TRB1 and function returns 0, more_trbs_comming for TRB1 will be set to 0.
>
> 3 More TRBs without ZLP:
>         e.g.
>         TRB1  -  length > 0,  more_trbs_comming = 1 - TD_SIZE  > 0
>         TRB2 -  length > 0, more_trbs_comming = 1  - TD_SIZE > 0
>         TRB3 - length >= 0, more_trbs_comming = 0 -  TD_SIZE  = 0
>
> Pawel
>
> >
> >Peter
> >
> >> e.g.
> >>
> >> TD -> TRB1  transfer_length = 64KB, TD_SIZE =0
> >>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
> >>                  ignore this transfer and stop transfer on previous one
> >>
> >> TD -> TRB1  transfer_length = 64KB, TD_SIZE =1
> >>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
> >>                  execute this trb and send ZLP
> >>
> >> As you noticed previously, previous fix for last TRB returned TD_SIZE
> >> = 1 in some cases.
> >> Previous fix was working correct but was not compliance with
> >> controller specification.
> >>
> >> >
> >> >>
> >> >>   maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
> >> >>   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> >> >> --
> >> >> 2.25.1
> >> >>
> >> >
> >> >--
> >> >
> >>
> >> Thanks,
> >> Pawel Laszczak
> >
> >--
> >
> >Thanks,
> >Peter Chen
>
> Regards,
> Pawel Laszczak
Pawel Laszczak Nov. 10, 2022, 5:38 a.m. UTC | #6 
>On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <pawell@cadence.com>
>wrote:
>>
>> >
>> >
>> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
>> >>
>> >> >
>> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> >> processing ZLP TRB by controller.
>> >> >>
>> >> >> cc: <stable@vger.kernel.org>
>> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> >> >> USBSSP DRD Driver")
>> >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> >> >>
>> >> >> ---
>> >> >> Changelog:
>> >> >> v2:
>> >> >> - returned value for last TRB must be 0
>> >> >>
>> >> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index
>> >> >> 04dfcaa08dc4..aa79bce89d8a
>> >> >> 100644
>> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> >> cdnsp_device *pdev,
>> >> >>
>> >> >>   /* One TRB with a zero-length data packet. */
>> >> >>   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> >> >> -     trb_buff_len == td_total_len)
>> >> >> +     trb_buff_len == td_total_len) {
>> >> >> +         /* Before ZLP driver needs set TD_SIZE=1. */
>> >> >> +         if (more_trbs_coming)
>> >> >> +                 return 1;
>> >> >> +
>> >> >>           return 0;
>> >> >> + }
>> >> >
>> >> >Does that fix the issue you want at bulk transfer, which has
>> >> >zero-length packet at the last packet? It seems not align with
>> >> >your previous
>> >fix.
>> >> >Would you mind explaining more?
>> >>
>> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
>> >> TRB.
>> >>
>> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should
>> >> have set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last
>> >> one TRB then the controller stops the transfer and ignore trb for ZLP
>packet.
>> >>
>> >> To fix this, the driver in such case must set TD_SIZE = 1 before
>> >> the last TRB.
>> >
>> >       if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> > -         trb_buff_len == td_total_len)
>> > +         trb_buff_len == td_total_len) {
>> > +             /* Before ZLP driver needs set TD_SIZE=1. */
>> > +             if (more_trbs_coming)
>> > +                     return 1;
>> > +
>> >               return 0;
>> > +     }
>> >
>> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>> >Which conditions are satisfied?
>>
>> For last non-ZLP TRB TD_SIZE should be 0 or 1.
>>
>> We have three casess:
>> 1.
>>         TRB1 - length > 1
>>         TRb2 - ZLP
>>
>> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
>>         if (more_trbs_coming)
>>                 return 1;
>>
>> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for
>> TRB2 is 0
>>
>
>This one is my question. How below condition is true for your case 1:
>
> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>          trb_buff_len == td_total_len)

For TRB1:
   more_trbs_coming = true
   transferred == 0 && trb_buff_len == 0  - false - it does not matter in this case
   trb_buff_len == td_total_len - true 
  so whole condition is true.
 
  Because more_trb_comming = true so:
             if (more_trbs_coming)
                        return 1;
returns TD_SIZE = 1

For TRB2 - ZLP:
   more_trbs_coming = false
   transferred == 0 && trb_buff_len == 0  - false - it does not matter in this case
   trb_buff_len == td_total_len - true

  Because more_trb_comming = false so function returns TD_SIZE = 0  for last ZLP trb.

Pawel

>
>Peter
>
>
>
>>
>> 2.
>>         TRB1 - length >1 and we don't except ZLP
>>
>> In this case TD_SIZE should be set to 0 for TRB1 and function returns 0,
>more_trbs_comming for TRB1 will be set to 0.
>>
>> 3 More TRBs without ZLP:
>>         e.g.
>>         TRB1  -  length > 0,  more_trbs_comming = 1 - TD_SIZE  > 0
>>         TRB2 -  length > 0, more_trbs_comming = 1  - TD_SIZE > 0
>>         TRB3 - length >= 0, more_trbs_comming = 0 -  TD_SIZE  = 0
>>
>> Pawel
>>
>> >
>> >Peter
>> >
>> >> e.g.
>> >>
>> >> TD -> TRB1  transfer_length = 64KB, TD_SIZE =0
>> >>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
>> >>                  ignore this transfer and stop transfer on previous
>> >> one
>> >>
>> >> TD -> TRB1  transfer_length = 64KB, TD_SIZE =1
>> >>           TRB2 transfer_length =0, TD_SIZE = 0  - controller will
>> >>                  execute this trb and send ZLP
>> >>
>> >> As you noticed previously, previous fix for last TRB returned
>> >> TD_SIZE = 1 in some cases.
>> >> Previous fix was working correct but was not compliance with
>> >> controller specification.
>> >>
>> >> >
>> >> >>
>> >> >>   maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>> >> >>   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
>> >> >> --
>> >> >> 2.25.1
>> >> >>
>> >> >
>> >> >--
>> >> >
>> >>
>> >> Thanks,
>> >> Pawel Laszczak
>> >
>> >--
>> >
>> >Thanks,
>> >Peter Chen
>>
>> Regards,
>> Pawel Laszczak
Peter Chen Nov. 11, 2022, 1:28 a.m. UTC | #7 
On Thu, Nov 10, 2022 at 1:38 PM Pawel Laszczak <pawell@cadence.com> wrote:
>
> >On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <pawell@cadence.com>
> >wrote:
> >>
> >> >
> >> >
> >> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
> >> >>
> >> >> >
> >> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
> >> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> >> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
> >> >> >> processing ZLP TRB by controller.
> >> >> >>
> >> >> >> cc: <stable@vger.kernel.org>
> >> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> >> >> USBSSP DRD Driver")
> >> >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> >> >> >>
> >> >> >> ---
> >> >> >> Changelog:
> >> >> >> v2:
> >> >> >> - returned value for last TRB must be 0
> >> >> >>
> >> >> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> >> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index
> >> >> >> 04dfcaa08dc4..aa79bce89d8a
> >> >> >> 100644
> >> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
> >> >> >> cdnsp_device *pdev,
> >> >> >>
> >> >> >>   /* One TRB with a zero-length data packet. */
> >> >> >>   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> >> >> -     trb_buff_len == td_total_len)
> >> >> >> +     trb_buff_len == td_total_len) {
> >> >> >> +         /* Before ZLP driver needs set TD_SIZE=1. */
> >> >> >> +         if (more_trbs_coming)
> >> >> >> +                 return 1;
> >> >> >> +
> >> >> >>           return 0;
> >> >> >> + }
> >> >> >
> >> >> >Does that fix the issue you want at bulk transfer, which has
> >> >> >zero-length packet at the last packet? It seems not align with
> >> >> >your previous
> >> >fix.
> >> >> >Would you mind explaining more?
> >> >>
> >> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
> >> >> TRB.
> >> >>
> >> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should
> >> >> have set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last
> >> >> one TRB then the controller stops the transfer and ignore trb for ZLP
> >packet.
> >> >>
> >> >> To fix this, the driver in such case must set TD_SIZE = 1 before
> >> >> the last TRB.
> >> >
> >> >       if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> > -         trb_buff_len == td_total_len)
> >> > +         trb_buff_len == td_total_len) {
> >> > +             /* Before ZLP driver needs set TD_SIZE=1. */
> >> > +             if (more_trbs_coming)
> >> > +                     return 1;
> >> > +
> >> >               return 0;
> >> > +     }
> >> >
> >> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
> >> >Which conditions are satisfied?
> >>
> >> For last non-ZLP TRB TD_SIZE should be 0 or 1.
> >>
> >> We have three casess:
> >> 1.
> >>         TRB1 - length > 1
> >>         TRb2 - ZLP
> >>
> >> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
> >>         if (more_trbs_coming)
> >>                 return 1;
> >>
> >> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for
> >> TRB2 is 0
> >>
> >
> >This one is my question. How below condition is true for your case 1:
> >
> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >          trb_buff_len == td_total_len)
>
> For TRB1:
>    more_trbs_coming = true
>    transferred == 0 && trb_buff_len == 0  - false - it does not matter in this case
>    trb_buff_len == td_total_len - true

Why trb_buff_len equals to td_total_length? Considering the bulk
transfer with request length
larger than 64KB.

Peter

>   so whole condition is true.
>
>   Because more_trb_comming = true so:
>              if (more_trbs_coming)
>                         return 1;
> returns TD_SIZE = 1
>
> For TRB2 - ZLP:
>    more_trbs_coming = false
>    transferred == 0 && trb_buff_len == 0  - false - it does not matter in this case
>    trb_buff_len == td_total_len - true
>
>   Because more_trb_comming = false so function returns TD_SIZE = 0  for last ZLP trb.
>
> Pawel
Pawel Laszczak Nov. 15, 2022, 9:31 a.m. UTC | #8 
>On Thu, Nov 10, 2022 at 1:38 PM Pawel Laszczak <pawell@cadence.com>
>wrote:
>>
>> >On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <pawell@cadence.com>
>> >wrote:
>> >>
>> >> >
>> >> >
>> >> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
>> >> >>
>> >> >> >
>> >> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> >> >> processing ZLP TRB by controller.
>> >> >> >>
>> >> >> >> cc: <stable@vger.kernel.org>
>> >> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of
>> >> >> >> Cadence USBSSP DRD Driver")
>> >> >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> >> >> >>
>> >> >> >> ---
>> >> >> >> Changelog:
>> >> >> >> v2:
>> >> >> >> - returned value for last TRB must be 0
>> >> >> >>
>> >> >> >>  drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >> >> >>
>> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index
>> >> >> >> 04dfcaa08dc4..aa79bce89d8a
>> >> >> >> 100644
>> >> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> >> >> cdnsp_device *pdev,
>> >> >> >>
>> >> >> >>   /* One TRB with a zero-length data packet. */
>> >> >> >>   if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0)
>||
>> >> >> >> -     trb_buff_len == td_total_len)
>> >> >> >> +     trb_buff_len == td_total_len) {
>> >> >> >> +         /* Before ZLP driver needs set TD_SIZE=1. */
>> >> >> >> +         if (more_trbs_coming)
>> >> >> >> +                 return 1;
>> >> >> >> +
>> >> >> >>           return 0;
>> >> >> >> + }
>> >> >> >
>> >> >> >Does that fix the issue you want at bulk transfer, which has
>> >> >> >zero-length packet at the last packet? It seems not align with
>> >> >> >your previous
>> >> >fix.
>> >> >> >Would you mind explaining more?
>> >> >>
>> >> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE
>> >> >> in TRB.
>> >> >>
>> >> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should
>> >> >> have set also TD_SIZE=0. If driver set TD_SIZE=0 on before the
>> >> >> last one TRB then the controller stops the transfer and ignore
>> >> >> trb for ZLP
>> >packet.
>> >> >>
>> >> >> To fix this, the driver in such case must set TD_SIZE = 1 before
>> >> >> the last TRB.
>> >> >
>> >> >       if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0)
>||
>> >> > -         trb_buff_len == td_total_len)
>> >> > +         trb_buff_len == td_total_len) {
>> >> > +             /* Before ZLP driver needs set TD_SIZE=1. */
>> >> > +             if (more_trbs_coming)
>> >> > +                     return 1;
>> >> > +
>> >> >               return 0;
>> >> > +     }
>> >> >
>> >> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>> >> >Which conditions are satisfied?
>> >>
>> >> For last non-ZLP TRB TD_SIZE should be 0 or 1.
>> >>
>> >> We have three casess:
>> >> 1.
>> >>         TRB1 - length > 1
>> >>         TRb2 - ZLP
>> >>
>> >> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
>> >>         if (more_trbs_coming)
>> >>                 return 1;
>> >>
>> >> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and
>> >> for
>> >> TRB2 is 0
>> >>
>> >
>> >This one is my question. How below condition is true for your case 1:
>> >
>> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> >          trb_buff_len == td_total_len)
>>
>> For TRB1:
>>    more_trbs_coming = true
>>    transferred == 0 && trb_buff_len == 0  - false - it does not matter in this
>case
>>    trb_buff_len == td_total_len - true
>
>Why trb_buff_len equals to td_total_length? Considering the bulk transfer
>with request length larger than 64KB.
>

You have right, there might still be a problem with ZLP. 
I've posted the v3 implemented a little differently.

Thanks
Pawel

>
>>   so whole condition is true.
>>
>>   Because more_trb_comming = true so:
>>              if (more_trbs_coming)
>>                         return 1;
>> returns TD_SIZE = 1
>>
>> For TRB2 - ZLP:
>>    more_trbs_coming = false
>>    transferred == 0 && trb_buff_len == 0  - false - it does not matter in this
>case
>>    trb_buff_len == td_total_len - true
>>
>>   Because more_trb_comming = false so function returns TD_SIZE = 0  for
>last ZLP trb.
>>
>> Pawel
diff mbox series

Patch

diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
index 04dfcaa08dc4..aa79bce89d8a 100644
--- a/drivers/usb/cdns3/cdnsp-ring.c
+++ b/drivers/usb/cdns3/cdnsp-ring.c
@@ -1769,8 +1769,13 @@  static u32 cdnsp_td_remainder(struct cdnsp_device *pdev,
 
 	/* One TRB with a zero-length data packet. */
 	if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
-	    trb_buff_len == td_total_len)
+	    trb_buff_len == td_total_len) {
+		/* Before ZLP driver needs set TD_SIZE=1. */
+		if (more_trbs_coming)
+			return 1;
+
 		return 0;
+	}
 
 	maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
 	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);