Received-SPF: pass (google.com: domain of linux-kernel+bounces-46986-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240131150332.1326523-1-quic_kriskura@quicinc.com>
 <CANP3RGeHXmEcDN=-h1uGBzu_Ur2UcmiEuFDXAEr0Z2ptXnHq=Q@mail.gmail.com> <47bf719c-a5c1-473b-9fa0-8cad84f0721c@quicinc.com>
In-Reply-To: <47bf719c-a5c1-473b-9fa0-8cad84f0721c@quicinc.com>
From: =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.com>
Date: Wed, 31 Jan 2024 10:54:31 -0800
Message-ID: <CANP3RGf8=3OwCDnE8UT8PQNRwQrtLO97_yR+Vm0h12Q3anbtVA@mail.gmail.com>
Subject: Re: [PATCH v2] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs
To: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Hardik Gajjar <hgajjar@de.adit-jv.com>, 
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 
	quic_ppratap@quicinc.com, quic_wcheng@quicinc.com, quic_jackp@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 10:08=E2=80=AFAM Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
>
>
>
> On 1/31/2024 10:33 PM, Maciej =C5=BBenczykowski wrote:
> >>
> >> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/=
function/f_ncm.c
> >> index ca5d5f564998..8c314dc98952 100644
> >> --- a/drivers/usb/gadget/function/f_ncm.c
> >> +++ b/drivers/usb/gadget/function/f_ncm.c
> >> @@ -1338,11 +1338,17 @@ static int ncm_unwrap_ntb(struct gether *port,
> >>               "Parsed NTB with %d frames\n", dgram_counter);
> >>
> >>          to_process -=3D block_len;
> >> -       if (to_process !=3D 0) {
> >> +
> >> +       if (to_process =3D=3D 1 &&
> >> +           (block_len % 512 =3D=3D 0) &&
> >> +           (*(unsigned char *)(ntb_ptr + block_len) =3D=3D 0x00)) {
> >
>
> Hi Maciej,
>
> > I'm unconvinced this (block_len % 512) works right...
> > imagine you have 2 ntbs back to back (for example 400 + 624) that
> > together add up to 1024,
> > and then a padding null byte.
> > I think block_len will be 624 then and not 1024.
> >
> > perhaps this actually needs to be:
> >    (ntp_ptr + block_len - skb->data) % 512 =3D=3D 0
> >
> > The point is that AFAICT the multiple of 512/1024 that matters is in
> > the size of the USB transfer,
> > not the NTB block size itself.
> >
>
> Ahh !!  So you mean, since this comes because the host doesn't want to
> send packets of size wMaxPacketSize on the wire, we need to consider the

of size 'multiple of 512 or 1024', but yes.

> length of data parsed so far to be checked against 512/1024 and not
> block length.

I believe so, yes.

I believe they are trying to avoid having to send ZLPs.
That's determined *purely* by the size of things as they show up on
the usb cable (ie. the size of the usb xfer).
ie. that's where things that are a multiple of 512 (USB2) or 1024
(USB3) need an extra 0 byte sized packet to prevent ZLP.

The actual size of the NTB doesn't matter.

That said... maybe we're overcomplicating this...
Maybe it's enough to just remove this modulo check entirely (I know I
asked for it before).

Ultimately if we just do:

// Windows NCM driver avoids USB ZLPs by adding a 1-byte zero pad as needed
if (to_process =3D=3D 1 && !*(u8*)(ntb_ptr + block_len)) --to_process;

it'll fix the problem too, and perhaps be easier to understand?

> But either ways, the implementation in the patch also the same thing in
> a different way right ? Process all NTB's present iteratively and while
> doing so, check if there is one byte left. So if there are multiple
> NTB's mixed up to form a packet of size 1024, even then, both the logics
> would converge onto the point where they find that one byte is left.
>
> >> +               goto done;
> >> +       } else if (to_process > 0) {
> >>                  ntb_ptr =3D (unsigned char *)(ntb_ptr + block_len);
> >
> > note that ntb_ptr moves forward by block_len here,
> > so it's *not* always the start of the packet, so block_len is not
> > necessarily the actual on the wire size.
> >
>
> Apologies, I didn't understand this comment. By moving the ntb_ptr by
> block length, we are pointing to the (last byte/ start of the next NTB)
> right as we are fixing in [1] ?

I was just pointing out why the above doesn't (afaict) work.

>
> >>                  goto parse_ntb;
> >>          }
> >>
> >> +done:
> >>          dev_consume_skb_any(skb);
> >>
> >>          return 0;
> >> --
> >> 2.34.1
> >>
> >
> > But it would perhaps be worth confirming in the MS driver source what
> > exactly they're doing...
> > (maybe they never even generate multiple ntbs per usb segment...)
> >
>
> Yes they do and we made a fix for that in [1].
>
>
> > You may also want to mention in the commit message that 512 comes from
> > USB2 block size, and also works for USB3 block size of 1024.
> >
>
> Sure. Will update the commit message accordingly.
>
> [1]:
> https://lore.kernel.org/all/20230927105858.12950-1-quic_kriskura@quicinc.=
com/
>
> Regards,
> Krishna,

--
Maciej =C5=BBenczykowski, Kernel Networking Developer @ Google