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. Are you interested in working up a proper patch, as described in Documentation/process/submitting-patches.rst ? > Patch: > > --- f_printer.c.orig 2021-11-26 19:12:21.632221126 +0200 > +++ f_printer.c 2021-11-26 19:09:19.454991670 +0200 > @@ -1003,11 +1003,11 @@ > value = 0; > break; > } > - value = strlen(dev->pnp_string) ; > + value = strlen(dev->pnp_string) + 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, value - 2); > + DBG(dev, "1284 PNP String: %x %s\n", value - 2, > dev->pnp_string); > /* Length of packet is length of length field and length of iPNPstring. */ > break;
