Re: BUG: iPNPstring in f_printer USB gadget is reduced by two bytes

[John Keeping <john@metanate.com>]

On Fri, Dec 03, 2021 at 04:46:17PM +0200, Volodymyr Lisivka wrote:
> > On Tue, Nov 30, 2021 at 08:01:46PM +0200, Volodymyr Lisivka wrote:
> > > Description:
> > >
> > > Printer USB gadget uses iPNPstring to communicate device name and
> > > command language with host. Linux driver for printer gadget sends
> > > GET_DEVICE_ID response packet without 2 last bytes, which may cause
> > > trouble for the host driver.
> > >
> > > Steps to reproduce:
> > >
> > > Use Raspberry Pi, or an other device with USB OTG plug. Raspberry Pi 4
> > > was used by issue author.
> > > Configure plug to be in peripheral mode, e.g. by adding
> > > dtoverlay=dwc2,dr_mode=peripheral to /boot/config.txt.
> > > Connect OTG port to host and reboot Raspberry Pi.
> > > Load g_printer module using command sudo modprobe g_printer.
> > > Use command ./get-iPNPstring.pl /dev/usb/lp1 to get iPNPstring from
> > > the device. (See get-iPNPstring.pl.gz ). As alternative, kernel usbmon
> > > or WireShark can be used to capture raw USB packet for GET_DEVICE_ID
> > > response.
> > >
> > > Expected result:
> > >
> > > It's expected to receive same string as defined in module:
> > > iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:1;'
> > >
> > > Actual result:
> > >
> > > iPNPstring='MFG:linux;MDL:g_printer;CLS:PRINTER;SN:'
> > >
> > > (Notice that last 2 chars are missing).
> > >
> > > Workarround:
> > >
> > > Just add two space to the end of printer gadget iPNPstring.
> > >
> > > Root cause:
> > >
> > > In drivers/usb/gadget/function/f_printer.c, length of iPNPstring is
> > > used as length of the whole packet, without length of 2 byte size
> > > field.
> >
> > If I understand correctly, the length should be inclusive of the two
> > length bytes, but currently this driver encodes it exclusive.
> >
> > The USB printer class specification says:
> >
> > ... a device ID string that is compatible with IEEE 1284.  See
> > IEEE 1284 for syntax and formatting information
> >
> > and goes on to specify that this includes the length in the first two
> > bytes as big endian.
> >
> > I don't have access to IEEE 1284, but looking in drivers/parport/probe.c
> > which implements the host side of IEEE 1284, we find
> > parport_read_device_id() with the comment:
> >
> > /* Some devices wrongly send LE length, and some send it two
> > * bytes short. Construct a sorted array of lengths to try. */
> >
> > and code that assumes the values are inclusive of the length bytes.
> >
> > So the patch below looks like it does the right thing to me, although it
> > appears whitespace damaged and it may be clearer to introduce a separate
> > variable for the string length compared to the value.
> 
> diff --git a/drivers/usb/gadget/function/f_printer.c
> b/drivers/usb/gadget/function/f_printer.c
> index 236ecc968..403faa05b 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -987,6 +987,7 @@ static int printer_func_setup(struct usb_function *f,
>         u16                     wIndex = le16_to_cpu(ctrl->wIndex);
>         u16                     wValue = le16_to_cpu(ctrl->wValue);
>         u16                     wLength = le16_to_cpu(ctrl->wLength);
> +       u16                     pnp_length;
> 
>         DBG(dev, "ctrl req%02x.%02x v%04x i%04x l%d\n",
>                 ctrl->bRequestType, ctrl->bRequest, wValue, wIndex, wLength);
> @@ -1003,11 +1004,12 @@ static int printer_func_setup(struct usb_function *f,
>                                 value = 0;
>                                 break;
>                         }
> -                       value = strlen(dev->pnp_string);
> +                       pnp_length = strlen(dev->pnp_string);
> +                       value = pnp_length + 2;
>                         buf[0] = (value >> 8) & 0xFF;
>                         buf[1] = value & 0xFF;
> -                       memcpy(buf + 2, dev->pnp_string, value);
> -                       DBG(dev, "1284 PNP String: %x %s\n", value,
> +                       memcpy(buf + 2, dev->pnp_string, pnp_length);
> +                       DBG(dev, "1284 PNP String: %x %s\n", pnp_length,
>                             dev->pnp_string);
>                         break;
> 
> 
> 
> >
> > Are you interested in working up a proper patch, as described in
> > Documentation/process/submitting-patches.rst ?
> >
> 
> No. It's my second patch in 15 years. If you have a proper procedure
> for diffing/patching, then make corresponding targets in the top
> Makefile, e.g. `make diff` or `make patch`.

Do you mean `git format-patch`? ;-)

Note that the diff is only part of a patch, the commit message and
Signed-off-by line ceritifying the developer certificate of origin [1]
are also important.

[1] https://developercertificate.org/