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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Nov 2022 06:21:49 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Peter Chen <hzpeterchen@...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

>
>> Pawel, with your change, the TD_SIZE is 1 or 0, but not like the
>> kernel doc defined like below:
>>
>
>Please omit my comments, I did not check the code carefully.
>
>> /*
>>  * TD size is the number of max packet sized packets remaining in the
>> TD
>>  * (*not* including this TRB).
>>  *
>
>With your current change, it may work. But your change conflicts with the
>xHCI spec that described above.
>With ZLP, the last useful trb's TD size should be 0, but if it is 0, the controller
>will be confused.
>
>With your change, it makes the code more different with xhci's. Do you
>consider handling ZLP packet at another TD instead of current at the same TD
>？

Yes, I considered it, but to simplify the driver I resigned from handling multiple TD
per usb_request. 
Current solution is simpler and supported by device controller.

Additionally, the next ZLP fix must be added for control endpoint for Data Stage 
and probably it will not work with 2 TDs.

Pawel

>
>>  * Total TD packet count = total_packet_count =
>>  *     DIV_ROUND_UP(TD size in bytes / wMaxPacketSize)
>>  *
>>  * Packets transferred up to and including this TRB = packets_transferred =
>>  *     rounddown(total bytes transferred including this TRB /
>wMaxPacketSize)
>>  *
>>  * TD size = total_packet_count - packets_transferred
>>  *
>>  * It must fit in bits 21:17, so it can't be bigger than 31.
>>  * This is taken care of in the TRB_TD_SIZE() macro
>>  *
>>  * The last TRB in a TD must have the TD size set to zero.
>>  */
>>
>> Peter
>>
>> >         /* 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
>> >