[PATCH] usb: sisusbvga: stop early init on the first failed transfer

Nguyen Ngoc Thang posted 1 patch 21 hours ago
drivers/usb/misc/sisusbvga/sisusbvga.c | 120 +++++++++++++------------
1 file changed, 62 insertions(+), 58 deletions(-)
[PATCH] usb: sisusbvga: stop early init on the first failed transfer
Posted by Nguyen Ngoc Thang 21 hours ago
sisusb_probe() initializes the graphics device from within the hub's
enumeration work, i.e. with the parent hub's device lock held.
sisusb_do_init_gfxdevice() accumulates errors with "ret |=" and keeps
going after a failed transfer. If the device does not answer, every
remaining transfer sits out its full timeout (5s, retried 6 times), so
probe holds the hub lock for ~30s per access and about 10 minutes in
total. Anything that needs that lock, such as usbdev_open() and
usbdev_ioctl() on the root hub, is blocked for the duration and trips the
hung task detector.

Return on the first error instead. Also fold the three identical BAR
setup sequences into a helper and the leading magic writes into a table.
The sequence of accesses is unchanged when the device responds.

Reproduced with a raw-gadget device (0711:0550, six bulk endpoints that
never complete I/O) on dummy_hcd: opening the root hub blocked for 603s
before, 29s after.

Reported-by: syzbot+109061940215dd4b011e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=109061940215dd4b011e
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
Notes (not for the commit log):
I have no SiS USB2VGA hardware, so the reordering is verified by inspection
only. A single silent device can still hold the hub lock for ~30s (one failed
transfer); removing that entirely would mean moving init out of probe. Happy
to do that if you prefer.
 drivers/usb/misc/sisusbvga/sisusbvga.c | 120 +++++++++++++------------
 1 file changed, 62 insertions(+), 58 deletions(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
index 3e75a7c24828..544284111191 100644
--- a/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -2064,77 +2064,81 @@ static void sisusb_get_ramconfig(struct sisusb_usb_data *sisusb)
 			ram_datarate[ramtype], ram_dynamictype[ramtype], bw);
 }
 
+/* Probe BAR size, then map it at @base */
+static int sisusb_init_bar(struct sisusb_usb_data *sisusb, int regnum, u32 base)
+{
+	u32 tmp32;
+	int ret;
+
+	ret = sisusb_read_pci_config(sisusb, regnum, &tmp32);
+	if (ret)
+		return ret;
+	ret = sisusb_write_pci_config(sisusb, regnum, 0xfffffff0);
+	if (ret)
+		return ret;
+	ret = sisusb_read_pci_config(sisusb, regnum, &tmp32);
+	if (ret)
+		return ret;
+	return sisusb_write_pci_config(sisusb, regnum, (tmp32 & 0x0f) | base);
+}
+
+/* Bail out on the first failure: each access to a dead device takes 5s */
 static int sisusb_do_init_gfxdevice(struct sisusb_usb_data *sisusb)
 {
+	static const struct { u32 address, data; } magic[] = {
+		{ 0x00000324, 0x00000004 },
+		{ 0x00000364, 0x00000004 },
+		{ 0x00000384, 0x00000004 },
+		{ 0x00000100, 0x00000700 },
+	};
+	static const struct { int regnum; u32 base; } bars[] = {
+		{ 0x10, SISUSB_PCI_MEMBASE },
+		{ 0x14, SISUSB_PCI_MMIOBASE },
+		{ 0x18, SISUSB_PCI_IOPORTBASE },
+	};
 	struct sisusb_packet packet;
-	int ret;
+	int ret, i;
 	u32 tmp32;
 
 	/* Do some magic */
-	packet.header  = 0x001f;
-	packet.address = 0x00000324;
-	packet.data    = 0x00000004;
-	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
-
-	packet.header  = 0x001f;
-	packet.address = 0x00000364;
-	packet.data    = 0x00000004;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
-
-	packet.header  = 0x001f;
-	packet.address = 0x00000384;
-	packet.data    = 0x00000004;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
-
-	packet.header  = 0x001f;
-	packet.address = 0x00000100;
-	packet.data    = 0x00000700;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	for (i = 0; i < ARRAY_SIZE(magic); i++) {
+		packet.header  = 0x001f;
+		packet.address = magic[i].address;
+		packet.data    = magic[i].data;
+		ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+		if (ret)
+			return ret;
+	}
 
 	packet.header  = 0x000f;
 	packet.address = 0x00000004;
-	ret |= sisusb_send_bridge_packet(sisusb, 6, &packet, 0);
+	ret = sisusb_send_bridge_packet(sisusb, 6, &packet, 0);
+	if (ret)
+		return ret;
 	packet.data |= 0x17;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
-
-	/* Init BAR 0 (VRAM) */
-	ret |= sisusb_read_pci_config(sisusb, 0x10, &tmp32);
-	ret |= sisusb_write_pci_config(sisusb, 0x10, 0xfffffff0);
-	ret |= sisusb_read_pci_config(sisusb, 0x10, &tmp32);
-	tmp32 &= 0x0f;
-	tmp32 |= SISUSB_PCI_MEMBASE;
-	ret |= sisusb_write_pci_config(sisusb, 0x10, tmp32);
-
-	/* Init BAR 1 (MMIO) */
-	ret |= sisusb_read_pci_config(sisusb, 0x14, &tmp32);
-	ret |= sisusb_write_pci_config(sisusb, 0x14, 0xfffffff0);
-	ret |= sisusb_read_pci_config(sisusb, 0x14, &tmp32);
-	tmp32 &= 0x0f;
-	tmp32 |= SISUSB_PCI_MMIOBASE;
-	ret |= sisusb_write_pci_config(sisusb, 0x14, tmp32);
-
-	/* Init BAR 2 (i/o ports) */
-	ret |= sisusb_read_pci_config(sisusb, 0x18, &tmp32);
-	ret |= sisusb_write_pci_config(sisusb, 0x18, 0xfffffff0);
-	ret |= sisusb_read_pci_config(sisusb, 0x18, &tmp32);
-	tmp32 &= 0x0f;
-	tmp32 |= SISUSB_PCI_IOPORTBASE;
-	ret |= sisusb_write_pci_config(sisusb, 0x18, tmp32);
-
-	/* Enable memory and i/o access */
-	ret |= sisusb_read_pci_config(sisusb, 0x04, &tmp32);
-	tmp32 |= 0x3;
-	ret |= sisusb_write_pci_config(sisusb, 0x04, tmp32);
+	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	if (ret)
+		return ret;
 
-	if (ret == 0) {
-		/* Some further magic */
-		packet.header  = 0x001f;
-		packet.address = 0x00000050;
-		packet.data    = 0x000000ff;
-		ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	for (i = 0; i < ARRAY_SIZE(bars); i++) {
+		ret = sisusb_init_bar(sisusb, bars[i].regnum, bars[i].base);
+		if (ret)
+			return ret;
 	}
 
-	return ret;
+	/* Enable memory and i/o access */
+	ret = sisusb_read_pci_config(sisusb, 0x04, &tmp32);
+	if (ret)
+		return ret;
+	ret = sisusb_write_pci_config(sisusb, 0x04, tmp32 | 0x3);
+	if (ret)
+		return ret;
+
+	/* Some further magic */
+	packet.header  = 0x001f;
+	packet.address = 0x00000050;
+	packet.data    = 0x000000ff;
+	return sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
 }
 
 /* Initialize the graphics device (return 0 on success)
-- 
2.43.0