linux-kernel - RE: [PATCH v3] usb: cdnsp: fix issue with ZLP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <BYAPR07MB5381EC4893D325AB9B464C09DD079@BYAPR07MB5381.namprd07.prod.outlook.com>
Date:   Wed, 16 Nov 2022 05:35:36 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     chao zeng <chao.zengup@...il.com>
CC:     "peter.chen@...nel.org" <peter.chen@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH v3] usb: cdnsp: fix issue with ZLP - added TD_SIZE = 1

>
>Hello
>
>   I think the problem is that you want to handle the zero-length trb and the
>other trbs in a transaction.  but when we calculate the td size，we do not
>count the zero-length trb. so maybe we can solve the problem  as below:
>
>
>
>--- a/drivers/usb/cdns3/cdnsp-ring.c
>
>+++ b/drivers/usb/cdns3/cdnsp-ring.c
>
>@@ -1960,7 +1960,7 @@ int cdnsp_queue_bulk_tx(struct cdnsp_device
>*pdev, struct cdnsp_request *preq)
>
>                /* Set the TRB length, TD size, and interrupter fields. */
>
>                remainder = cdnsp_td_remainder(pdev, enqd_len, trb_buff_len,
>
>                                               full_len, preq,
>
>-                                              more_trbs_coming);
>
>+                                              more_trbs_coming) + (need_zero_pkt ? 1 : 0);
>
>
>
>                length_field = TRB_LEN(trb_buff_len) | TRB_TD_SIZE(remainder) |
>
>                        TRB_INTR_TARGET(0);
>

Your proposition makes the same thing as the v3 patch but v3 contains the extra parameter. 
In my opinion the patch with extra parameter is more readably.

Did you find something wrong in v3. I have tested it with ECM class which make possible to
force ZLP.

Thanks
Pawel

>
>On Tue, Nov 15, 2022 at 5:31 PM Pawel Laszczak <pawell@...ence.com
><mailto:pawell@...ence.com> > 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@...r.kernel.org <mailto:stable@...r.kernel.org> >
>	Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>USBSSP DRD Driver")
>	Signed-off-by: Pawel Laszczak <pawell@...ence.com
><mailto:pawell@...ence.com> >
>	---
>	v2:
>	- returned value for last TRB must be 0
>	v3:
>	- fix issue for request->length > 64KB
>
>	 drivers/usb/cdns3/cdnsp-ring.c | 14 ++++++++++----
>	 1 file changed, 10 insertions(+), 4 deletions(-)
>
>	diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-
>ring.c
>	index 794e413800ae..86e1141e150f 100644
>	--- a/drivers/usb/cdns3/cdnsp-ring.c
>	+++ b/drivers/usb/cdns3/cdnsp-ring.c
>	@@ -1763,10 +1763,15 @@ static u32 cdnsp_td_remainder(struct
>cdnsp_device *pdev,
>	                              int trb_buff_len,
>	                              unsigned int td_total_len,
>	                              struct cdnsp_request *preq,
>	-                             bool more_trbs_coming)
>	+                             bool more_trbs_coming,
>	+                             bool zlp)
>	 {
>	        u32 maxp, total_packet_count;
>
>	+       /* Before ZLP driver needs set TD_SIZE = 1. */
>	+       if (zlp)
>	+               return 1;
>	+
>	        /* One TRB with a zero-length data packet. */
>	        if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0)
>||
>	            trb_buff_len == td_total_len)
>	@@ -1960,7 +1965,8 @@ int cdnsp_queue_bulk_tx(struct
>cdnsp_device *pdev, struct cdnsp_request *preq)
>	                /* Set the TRB length, TD size, and interrupter fields. */
>	                remainder = cdnsp_td_remainder(pdev, enqd_len,
>trb_buff_len,
>	                                               full_len, preq,
>	-                                              more_trbs_coming);
>	+                                              more_trbs_coming,
>	+                                              zero_len_trb);
>
>	                length_field = TRB_LEN(trb_buff_len) |
>TRB_TD_SIZE(remainder) |
>	                        TRB_INTR_TARGET(0);
>	@@ -2025,7 +2031,7 @@ int cdnsp_queue_ctrl_tx(struct
>cdnsp_device *pdev, struct cdnsp_request *preq)
>
>	        if (preq->request.length > 0) {
>	                remainder = cdnsp_td_remainder(pdev, 0, preq-
>>request.length,
>	-                                              preq->request.length, preq, 1);
>	+                                              preq->request.length, preq, 1, 0);
>
>	                length_field = TRB_LEN(preq->request.length) |
>	                                TRB_TD_SIZE(remainder) | TRB_INTR_TARGET(0);
>	@@ -2225,7 +2231,7 @@ static int cdnsp_queue_isoc_tx(struct
>cdnsp_device *pdev,
>	                /* Set the TRB length, TD size, & interrupter fields. */
>	                remainder = cdnsp_td_remainder(pdev, running_total,
>	                                               trb_buff_len, td_len, preq,
>	-                                              more_trbs_coming);
>	+                                              more_trbs_coming, 0);
>
>	                length_field = TRB_LEN(trb_buff_len) |
>TRB_INTR_TARGET(0);
>
>	--
>	2.25.1
>
>