Re: [PATCH v2] USB: sisusbvga: avoid prolonged initialization on failure

[Ayush Yaduvanshi]

Hi Michal,

Just a quick follow-up on v2.

Syzbot tested the exact v2 patch with the reported reproducer and reported that
the reproducer did not trigger the reported hang.

Please let me know if any further changes or testing are needed.

Thanks,
Ayush Yaduvanshi

On Sun, Sep 20, 2026 at 12:00 AM Ayush Yaduvanshi <ayush37735@xxxxxxxxx> wrote:
>
> Hi Michal,
>
> Just a quick follow-up on v2.
>
> Syzbot tested the exact v2 patch with the reported reproducer and reported that
> the reproducer did not trigger the reported hang.
>
> Please let me know if any further changes or testing are needed.
>
> Thanks,
> Ayush Yaduvanshi
>
> On Mon, Sep 7, 2026 at 1:13 PM Ayush Yaduvanshi <ayush37735@xxxxxxxxx> wrote:
>>
>> The USB core holds minor_rwsem while invoking a USB character
>> device's open callback. Avoid retrying graphics-device initialization
>> from open() and fail probe when early initialization fails.
>>
>> Also stop sisusb_do_init_gfxdevice() after the first failed request.
>> Continuing initialization after a failed USB request causes additional
>> synchronous transfers and their retries against an unresponsive device.
>>
>> v2:
>> Stop sisusb_do_init_gfxdevice() after the first failed USB request and
>> treat early initialization failure in probe() as a probe failure.
>> Remove the initialization retry from sisusb_open().
>>
>> The exact v2 patch was tested by syzbot using the reported reproducer,
>> and the reproducer did not trigger the reported hang.
>>
>> Reported-by: syzbot+3bc656a9271e7c8a5c6b@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Closes: https://syzkaller.appspot.com/bug?extid=3bc656a9271e7c8a5c6b
>> Tested-by: syzbot+3bc656a9271e7c8a5c6b@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: Ayush Yaduvanshi <ayush37735@xxxxxxxxx>
>> ---
>> Thanks for the review.
>>
>> v2:
>> - Stop sisusb_do_init_gfxdevice() after the first failed USB request
>>   instead of continuing with further synchronous requests and their
>>   timeout/retry cost on an unresponsive device.
>> - Treat failure of the early graphics-device initialization in probe()
>>   as a probe failure, so a device which failed initialization is not
>>   left for a later initialization attempt from open().
>> - Remove the graphics-device initialization retry from sisusb_open().
>> - The exact v2 patch was tested by syzbot using the reported reproducer;
>>   syzbot reports that the reproducer did not trigger the reported hang.
>>
>>  drivers/usb/misc/sisusbvga/sisusbvga.c | 133 +++++++++++++++++--------
>>  1 file changed, 89 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
>> index 3e75a7c24828..e0b1111c1f53 100644
>> --- a/drivers/usb/misc/sisusbvga/sisusbvga.c
>> +++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
>> @@ -2075,66 +2075,114 @@ static int sisusb_do_init_gfxdevice(struct sisusb_usb_data *sisusb)
>>         packet.address = 0x00000324;
>>         packet.data    = 0x00000004;
>>         ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       if (ret)
>> +               return ret;
>>
>>         packet.header  = 0x001f;
>>         packet.address = 0x00000364;
>>         packet.data    = 0x00000004;
>> -       ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       if (ret)
>> +               return ret;
>>
>>         packet.header  = 0x001f;
>>         packet.address = 0x00000384;
>>         packet.data    = 0x00000004;
>> -       ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       if (ret)
>> +               return ret;
>>
>>         packet.header  = 0x001f;
>>         packet.address = 0x00000100;
>>         packet.data    = 0x00000700;
>> -       ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       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);
>> +       ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> +       if (ret)
>> +               return ret;
>>
>>         /* 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);
>> +       ret = sisusb_read_pci_config(sisusb, 0x10, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sisusb_write_pci_config(sisusb, 0x10, 0xfffffff0);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sisusb_read_pci_config(sisusb, 0x10, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>>         tmp32 &= 0x0f;
>>         tmp32 |= SISUSB_PCI_MEMBASE;
>> -       ret |= sisusb_write_pci_config(sisusb, 0x10, tmp32);
>> +       ret = sisusb_write_pci_config(sisusb, 0x10, tmp32);
>> +       if (ret)
>> +               return ret;
>>
>>         /* 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);
>> +       ret = sisusb_read_pci_config(sisusb, 0x14, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sisusb_write_pci_config(sisusb, 0x14, 0xfffffff0);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sisusb_read_pci_config(sisusb, 0x14, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>>         tmp32 &= 0x0f;
>>         tmp32 |= SISUSB_PCI_MMIOBASE;
>> -       ret |= sisusb_write_pci_config(sisusb, 0x14, tmp32);
>> +       ret = sisusb_write_pci_config(sisusb, 0x14, tmp32);
>> +       if (ret)
>> +               return ret;
>>
>>         /* 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);
>> +       ret = sisusb_read_pci_config(sisusb, 0x18, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sisusb_write_pci_config(sisusb, 0x18, 0xfffffff0);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sisusb_read_pci_config(sisusb, 0x18, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>>         tmp32 &= 0x0f;
>>         tmp32 |= SISUSB_PCI_IOPORTBASE;
>> -       ret |= sisusb_write_pci_config(sisusb, 0x18, tmp32);
>> +       ret = sisusb_write_pci_config(sisusb, 0x18, tmp32);
>> +       if (ret)
>> +               return ret;
>>
>>         /* Enable memory and i/o access */
>> -       ret |= sisusb_read_pci_config(sisusb, 0x04, &tmp32);
>> +       ret = sisusb_read_pci_config(sisusb, 0x04, &tmp32);
>> +       if (ret)
>> +               return ret;
>> +
>>         tmp32 |= 0x3;
>> -       ret |= sisusb_write_pci_config(sisusb, 0x04, tmp32);
>> -
>> -       if (ret == 0) {
>> -               /* Some further magic */
>> -               packet.header  = 0x001f;
>> -               packet.address = 0x00000050;
>> -               packet.data    = 0x000000ff;
>> -               ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
>> -       }
>> +       ret = sisusb_write_pci_config(sisusb, 0x04, tmp32);
>> +       if (ret)
>> +               return 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)
>> @@ -2223,20 +2271,10 @@ static int sisusb_open(struct inode *inode, struct file *file)
>>         }
>>
>>         if (!sisusb->devinit) {
>> -               if (sisusb->sisusb_dev->speed == USB_SPEED_HIGH ||
>> -                               sisusb->sisusb_dev->speed >= USB_SPEED_SUPER) {
>> -                       if (sisusb_init_gfxdevice(sisusb, 0)) {
>> -                               mutex_unlock(&sisusb->lock);
>> -                               dev_err(&sisusb->sisusb_dev->dev,
>> -                                               "Failed to initialize device\n");
>> -                               return -EIO;
>> -                       }
>> -               } else {
>> -                       mutex_unlock(&sisusb->lock);
>> -                       dev_err(&sisusb->sisusb_dev->dev,
>> -                                       "Device not attached to USB 2.0 hub\n");
>> -                       return -EIO;
>> -               }
>> +               mutex_unlock(&sisusb->lock);
>> +               dev_err(&sisusb->sisusb_dev->dev,
>> +                       "Device not initialized\n");
>> +               return -EIO;
>>         }
>>
>>         /* Increment usage count for our sisusb */
>> @@ -2880,9 +2918,16 @@ static int sisusb_probe(struct usb_interface *intf,
>>
>>         if (dev->speed == USB_SPEED_HIGH || dev->speed >= USB_SPEED_SUPER) {
>>                 int initscreen = 1;
>> -               if (sisusb_init_gfxdevice(sisusb, initscreen))
>> +
>> +               if (sisusb_init_gfxdevice(sisusb, initscreen)) {
>>                         dev_err(&sisusb->sisusb_dev->dev,
>> -                                       "Failed to early initialize device\n");
>> +                               "Failed to early initialize device\n");
>> +                       sisusb->present = 0;
>> +                       usb_set_intfdata(intf, NULL);
>> +                       usb_put_dev(sisusb->sisusb_dev);
>> +                       retval = -EIO;
>> +                       goto error_4;
>> +               }
>>
>>         } else
>>                 dev_info(&sisusb->sisusb_dev->dev,
>> --
>> 2.53.0
>>