Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221115092218.421267-1-pawell@cadence.com> <CAL411-qwcboX-vTn+3oOPna+tixFNEajYi_E_rgweqrC3CcXCA@mail.gmail.com>
In-Reply-To: <CAL411-qwcboX-vTn+3oOPna+tixFNEajYi_E_rgweqrC3CcXCA@mail.gmail.com>
From:   Peter Chen <hzpeterchen@gmail.com>
Date:   Thu, 17 Nov 2022 14:02:48 +0800
Message-ID: <CAL411-pZm_8M4gSXJQ4zC1F-1w4zfTS2JRzxRM2e-ZQFjsx_Uw@mail.gmail.com>
Subject: Re: [PATCH v3] usb: cdnsp: fix issue with ZLP - added TD_SIZE = 1
To:     Pawel Laszczak <pawell@cadence.com>
Cc:     peter.chen@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

> 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=EF=BC=9F

Peter

>  * Total TD packet count =3D total_packet_count =3D
>  *     DIV_ROUND_UP(TD size in bytes / wMaxPacketSize)
>  *
>  * Packets transferred up to and including this TRB =3D packets_transferr=
ed =3D
>  *     rounddown(total bytes transferred including this TRB / wMaxPacketS=
ize)
>  *
>  * TD size =3D 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 =3D=3D 0 && trb_buff_len =
=3D=3D 0) ||
> >             trb_buff_len =3D=3D 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 =3D cdnsp_td_remainder(pdev, enqd_len, trb_bu=
ff_len,
> >                                                full_len, preq,
> > -                                              more_trbs_coming);
> > +                                              more_trbs_coming,
> > +                                              zero_len_trb);
> >
> >                 length_field =3D TRB_LEN(trb_buff_len) | TRB_TD_SIZE(re=
mainder) |
> >                         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 =3D cdnsp_td_remainder(pdev, 0, preq->request=
.length,
> > -                                              preq->request.length, pr=
eq, 1);
> > +                                              preq->request.length, pr=
eq, 1, 0);
> >
> >                 length_field =3D TRB_LEN(preq->request.length) |
> >                                 TRB_TD_SIZE(remainder) | TRB_INTR_TARGE=
T(0);
> > @@ -2225,7 +2231,7 @@ static int cdnsp_queue_isoc_tx(struct cdnsp_devic=
e *pdev,
> >                 /* Set the TRB length, TD size, & interrupter fields. *=
/
> >                 remainder =3D cdnsp_td_remainder(pdev, running_total,
> >                                                trb_buff_len, td_len, pr=
eq,
> > -                                              more_trbs_coming);
> > +                                              more_trbs_coming, 0);
> >
> >                 length_field =3D TRB_LEN(trb_buff_len) | TRB_INTR_TARGE=
T(0);
> >
> > --
> > 2.25.1
> >