> 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`.
