Re: [PATCH v1] USB: sisusbvga: avoid initializing device in open

[Thomas Winischhofer]

Sorry guys, wrote this more than 20 years ago, no longer have the 
hardware, nor the knowledge about modern kernels to be of any help.

/Thomas

On 06.09.26 08:10, Michal Pecio wrote:
> On Sun,  6 Sep 2026 01:06:34 +0530, Ayush wrote:
> > The USB core holds minor_rwsem while invoking a USB character
> > device's open callback. sisusb_open() can currently initialize the
> > device while holding sisusb->lock, and that initialization performs
> > synchronous USB I/O.
> >
> > High-speed devices are already initialized during probe. Treat a
> > probe-time initialization failure as a probe failure instead of
> > retrying initialization from open, so the open callback no longer
> > performs blocking device initialization.
> >
> > Reported-by: syzbot+3bc656a9271e7c8a5c6b@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=3bc656a9271e7c8a5c6b
>
> Does this even fix the repro? I see no syzbot test requests there.
>
> It seems that even if you delete this code, sisusb_open() will simply
> wait on mutex_lock() before it even reaches this point.
>
> The root cause is isusb_init_gfxdevice(), called by probe() and from
> here, taking 10 minutes to complete on nonresponsive device. This is
> the second time (that I know of) syzbot ran into this recently.
>
>
> Thomas, do you still have this HW and a will to improve the driver,
> or test / comment on patches? I suspect that those timeouts and retries
> in the init routine are vastly excessive.
>
> > Signed-off-by: Ayush <ayush37735@xxxxxxxxx>
> > ---
> >   drivers/usb/misc/sisusbvga/sisusbvga.c | 29 ++++++++++++--------------
> >   1 file changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
> > index 3e75a7c24828..6723a65b12b8 100644
> > --- a/drivers/usb/misc/sisusbvga/sisusbvga.c
> > +++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
> > @@ -2223,20 +2223,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;
>
> Not sure if we really want to log every failure to open some device.
>
> Not sure if EIO is the right code to retun from open(), particularly
> after probe failure.
>
> And really, are we opening the device after probe failed (see below)?
> Seems weird...
>
> >   	}
> >   
> >   	/* Increment usage count for our sisusb */
> > @@ -2880,9 +2870,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