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/
