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

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

[Ayush]

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3bc656a9271e7c8a5c6b
Signed-off-by: Ayush <ayush37735@gmail.com>
---
 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;
 	}
 
 	/* 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

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

[Michal Pecio]

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@syzkaller.appspotmail.com
> 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@gmail.com>
> ---
>  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
> 

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@syzkaller.appspotmail.com
>> 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@gmail.com>
>> ---
>>   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
>>
>