[PATCH] usb: storage: skip only when uas driver is loaded

[PATCH] usb: storage: skip only when uas driver is loaded

[JC Kuo]

When usb-storage driver detects a UAS capable device, it ignores the
device if CONFIG_USB_UAS is enabled. usb-storage driver assumes uas
driver certainly will be loaded. However, it's possible that uas
driver will not be loaded, for example, uas kernel module is not
installed properly or it is in modprobe blacklist.

In case of uas driver not being loaded, the UAS capable device will
not fallback to work at Bulk-only-transfer mode. The device just
disappears without any notification to user/userspace.

This commit changes usb-storage driver to skip UAS capable device
only when uas driver is already loaded to make sure the device will
at least work with Bulk protocol.

Signed-off-by: JC Kuo <jckuo@nvidia.com>
---
 drivers/usb/core/driver.c | 1 +
 drivers/usb/storage/usb.c | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index ebcadaad89d1..265c5dd490d2 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1923,3 +1923,4 @@ struct bus_type usb_bus_type = {
 	.uevent =	usb_uevent,
 	.need_parent_lock =	true,
 };
+EXPORT_SYMBOL_GPL(usb_bus_type);
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 9a79cd9762f3..d8f64e808783 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1097,9 +1097,10 @@ static int storage_probe(struct usb_interface *intf,
 	int result;
 	int size;
 
-	/* If uas is enabled and this device can do uas then ignore it. */
+	/* If uas driver is loaded and this device can do uas then ignore it. */
 #if IS_ENABLED(CONFIG_USB_UAS)
-	if (uas_use_uas_driver(intf, id, NULL))
+	if (driver_find("uas", &usb_bus_type) &&
+		uas_use_uas_driver(intf, id, NULL))
 		return -ENXIO;
 #endif
 
-- 
2.17.1


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Greg KH]

On Mon, Jul 01, 2019 at 04:48:48PM +0800, JC Kuo wrote:
> When usb-storage driver detects a UAS capable device, it ignores the
> device if CONFIG_USB_UAS is enabled. usb-storage driver assumes uas
> driver certainly will be loaded. However, it's possible that uas
> driver will not be loaded, for example, uas kernel module is not
> installed properly or it is in modprobe blacklist.
> 
> In case of uas driver not being loaded, the UAS capable device will
> not fallback to work at Bulk-only-transfer mode. The device just
> disappears without any notification to user/userspace.
> 
> This commit changes usb-storage driver to skip UAS capable device
> only when uas driver is already loaded to make sure the device will
> at least work with Bulk protocol.

But what happens if the driver is loaded afterward, because 'modprobe'
was called by the driver core (or it should have been, because this is a
device that supports that protocol)?

I think you just broke working systems :(

Why wouldn't the UAS driver get loaded automatically if it is configured
in the system as it is today?

thanks,

greg k-h

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[JC Kuo]

On 7/1/19 4:52 PM, Greg KH wrote:
> On Mon, Jul 01, 2019 at 04:48:48PM +0800, JC Kuo wrote:
>> When usb-storage driver detects a UAS capable device, it ignores the
>> device if CONFIG_USB_UAS is enabled. usb-storage driver assumes uas
>> driver certainly will be loaded. However, it's possible that uas
>> driver will not be loaded, for example, uas kernel module is not
>> installed properly or it is in modprobe blacklist.
>>
>> In case of uas driver not being loaded, the UAS capable device will
>> not fallback to work at Bulk-only-transfer mode. The device just
>> disappears without any notification to user/userspace.
>>
>> This commit changes usb-storage driver to skip UAS capable device
>> only when uas driver is already loaded to make sure the device will
>> at least work with Bulk protocol.
> 
> But what happens if the driver is loaded afterward, because 'modprobe'
> was called by the driver core (or it should have been, because this is a
> device that supports that protocol)?
If uas driver is loaded after usb-storage driver probed the device, the device will still work with Bulk-only protocol, though it can't make uses of streams.

> 
> I think you just broke working systems :(
> 
> Why wouldn't the UAS driver get loaded automatically if it is configured
> in the system as it is today?
An user might want to completely disable uas for some reason so he/she adds "blacklist uas" to modprobe conf file. I think in case of this, usb-storage driver has to enable this device with the legacy Bulk-only protocol instead of ignoring the device.

As an alternative to this patch, I thought I could get uas driver loaded before usb-storage driver so I tried moving the functions in drivers/usb/storage/uas-detect.h into uas.c and letting usb-storage links uas_use_uas_driver() of uas.ko. However, that didn't work because uas driver actually depends on usb-storage driver for usb_stor_adjust_quirks(). There will be a recursive dependency.

Please let me know if there is better approach to avoid the issue.

Thanks,
JC
> 
> thanks,
> 
> greg k-h
> 

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Greg KH]

On Tue, Jul 02, 2019 at 10:36:59AM +0800, JC Kuo wrote:
> On 7/1/19 4:52 PM, Greg KH wrote:
> > On Mon, Jul 01, 2019 at 04:48:48PM +0800, JC Kuo wrote:
> >> When usb-storage driver detects a UAS capable device, it ignores the
> >> device if CONFIG_USB_UAS is enabled. usb-storage driver assumes uas
> >> driver certainly will be loaded. However, it's possible that uas
> >> driver will not be loaded, for example, uas kernel module is not
> >> installed properly or it is in modprobe blacklist.
> >>
> >> In case of uas driver not being loaded, the UAS capable device will
> >> not fallback to work at Bulk-only-transfer mode. The device just
> >> disappears without any notification to user/userspace.
> >>
> >> This commit changes usb-storage driver to skip UAS capable device
> >> only when uas driver is already loaded to make sure the device will
> >> at least work with Bulk protocol.
> > 
> > But what happens if the driver is loaded afterward, because 'modprobe'
> > was called by the driver core (or it should have been, because this is a
> > device that supports that protocol)?
> If uas driver is loaded after usb-storage driver probed the device,
> the device will still work with Bulk-only protocol, though it can't
> make uses of streams.

Which is not a good thing, and is what the original code was there to
prevent happening.

> > I think you just broke working systems :(
> > 
> > Why wouldn't the UAS driver get loaded automatically if it is configured
> > in the system as it is today?
> An user might want to completely disable uas for some reason so he/she
> adds "blacklist uas" to modprobe conf file. I think in case of this,
> usb-storage driver has to enable this device with the legacy Bulk-only
> protocol instead of ignoring the device.

Why would they want to do that?  Where are people doing this in ways
that breaks their systems?

> As an alternative to this patch, I thought I could get uas driver
> loaded before usb-storage driver so I tried moving the functions in
> drivers/usb/storage/uas-detect.h into uas.c and letting usb-storage
> links uas_use_uas_driver() of uas.ko. However, that didn't work
> because uas driver actually depends on usb-storage driver for
> usb_stor_adjust_quirks(). There will be a recursive dependency.
> 
> Please let me know if there is better approach to avoid the issue.

If users blacklist the uas driver, that's their choice and they should
rebuild their kernel :)

Or better yet, talk to us to get the issue fixed for why they would want
to blacklist such a driver.

As it is, this patch is not acceptable.

thanks,

greg k-h

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[JC Kuo]

On 7/2/19 12:42 PM, Greg KH wrote:
> On Tue, Jul 02, 2019 at 10:36:59AM +0800, JC Kuo wrote:
>> On 7/1/19 4:52 PM, Greg KH wrote:
>>> On Mon, Jul 01, 2019 at 04:48:48PM +0800, JC Kuo wrote:
>>>> When usb-storage driver detects a UAS capable device, it ignores the
>>>> device if CONFIG_USB_UAS is enabled. usb-storage driver assumes uas
>>>> driver certainly will be loaded. However, it's possible that uas
>>>> driver will not be loaded, for example, uas kernel module is not
>>>> installed properly or it is in modprobe blacklist.
>>>>
>>>> In case of uas driver not being loaded, the UAS capable device will
>>>> not fallback to work at Bulk-only-transfer mode. The device just
>>>> disappears without any notification to user/userspace.
>>>>
>>>> This commit changes usb-storage driver to skip UAS capable device
>>>> only when uas driver is already loaded to make sure the device will
>>>> at least work with Bulk protocol.
>>>
>>> But what happens if the driver is loaded afterward, because 'modprobe'
>>> was called by the driver core (or it should have been, because this is a
>>> device that supports that protocol)?
>> If uas driver is loaded after usb-storage driver probed the device,
>> the device will still work with Bulk-only protocol, though it can't
>> make uses of streams.
> 
> Which is not a good thing, and is what the original code was there to
> prevent happening.
> 
>>> I think you just broke working systems :(
>>>
>>> Why wouldn't the UAS driver get loaded automatically if it is configured
>>> in the system as it is today?
>> An user might want to completely disable uas for some reason so he/she
>> adds "blacklist uas" to modprobe conf file. I think in case of this,
>> usb-storage driver has to enable this device with the legacy Bulk-only
>> protocol instead of ignoring the device.
> 
> Why would they want to do that?  Where are people doing this in ways
> that breaks their systems?

I think that could be because user sees issues with UAS but he/she is not
aware of the quirks parameter that usb-storage module offers to disable UAS
for any particular device.

IMHO, blacklisting uas driver should not break the system because the UAS
devices are supposed to be working fine with legacy Bulk-only protocol if
system software doesn't have UAS support.

> 
>> As an alternative to this patch, I thought I could get uas driver
>> loaded before usb-storage driver so I tried moving the functions in
>> drivers/usb/storage/uas-detect.h into uas.c and letting usb-storage
>> links uas_use_uas_driver() of uas.ko. However, that didn't work
>> because uas driver actually depends on usb-storage driver for
>> usb_stor_adjust_quirks(). There will be a recursive dependency.
>>
>> Please let me know if there is better approach to avoid the issue.
> 
> If users blacklist the uas driver, that's their choice and they should
> rebuild their kernel :)
> 
> Or better yet, talk to us to get the issue fixed for why they would want
> to blacklist such a driver.

Agree. :)

> 
> As it is, this patch is not acceptable.

Understood. Thanks for the comments. I will drop this patch.

Thanks,
JC

> 
> thanks,
> 
> greg k-h
> 

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[JC Kuo]

On 7/2/19 1:29 PM, JC Kuo wrote:
> On 7/2/19 12:42 PM, Greg KH wrote:
>> On Tue, Jul 02, 2019 at 10:36:59AM +0800, JC Kuo wrote:
>>> On 7/1/19 4:52 PM, Greg KH wrote:
>>>> On Mon, Jul 01, 2019 at 04:48:48PM +0800, JC Kuo wrote:
>>>>> When usb-storage driver detects a UAS capable device, it ignores the
>>>>> device if CONFIG_USB_UAS is enabled. usb-storage driver assumes uas
>>>>> driver certainly will be loaded. However, it's possible that uas
>>>>> driver will not be loaded, for example, uas kernel module is not
>>>>> installed properly or it is in modprobe blacklist.
>>>>>
>>>>> In case of uas driver not being loaded, the UAS capable device will
>>>>> not fallback to work at Bulk-only-transfer mode. The device just
>>>>> disappears without any notification to user/userspace.
>>>>>
>>>>> This commit changes usb-storage driver to skip UAS capable device
>>>>> only when uas driver is already loaded to make sure the device will
>>>>> at least work with Bulk protocol.
>>>>
>>>> But what happens if the driver is loaded afterward, because 'modprobe'
>>>> was called by the driver core (or it should have been, because this is a
>>>> device that supports that protocol)?
>>> If uas driver is loaded after usb-storage driver probed the device,
>>> the device will still work with Bulk-only protocol, though it can't
>>> make uses of streams.
>>
>> Which is not a good thing, and is what the original code was there to
>> prevent happening.
>>
>>>> I think you just broke working systems :(
>>>>
>>>> Why wouldn't the UAS driver get loaded automatically if it is configured
>>>> in the system as it is today?
>>> An user might want to completely disable uas for some reason so he/she
>>> adds "blacklist uas" to modprobe conf file. I think in case of this,
>>> usb-storage driver has to enable this device with the legacy Bulk-only
>>> protocol instead of ignoring the device.
>>
>> Why would they want to do that?  Where are people doing this in ways
>> that breaks their systems?
> 
> I think that could be because user sees issues with UAS but he/she is not
> aware of the quirks parameter that usb-storage module offers to disable UAS
> for any particular device.
> 
> IMHO, blacklisting uas driver should not break the system because the UAS
> devices are supposed to be working fine with legacy Bulk-only protocol if
> system software doesn't have UAS support.
> 
>>
>>> As an alternative to this patch, I thought I could get uas driver
>>> loaded before usb-storage driver so I tried moving the functions in
>>> drivers/usb/storage/uas-detect.h into uas.c and letting usb-storage
>>> links uas_use_uas_driver() of uas.ko. However, that didn't work
>>> because uas driver actually depends on usb-storage driver for
>>> usb_stor_adjust_quirks(). There will be a recursive dependency.
>>>
>>> Please let me know if there is better approach to avoid the issue.
>>
>> If users blacklist the uas driver, that's their choice and they should
>> rebuild their kernel :)
>>
>> Or better yet, talk to us to get the issue fixed for why they would want
>> to blacklist such a driver.
> 
> Agree. :)
> 
>>
>> As it is, this patch is not acceptable.
> 
> Understood. Thanks for the comments. I will drop this patch.
> 
> Thanks,
> JC
> 
>>
>> thanks,
>>
>> greg k-h
>>

Hi Greg,
Since blacklisting uas kernel module is not a good idea and could break UAS 
capable storage functionality, do we consider forbidding making uas driver
as module? That means to make CONFIG_USB_UAS a bool option.

Thanks,
JC

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Greg KH]

On Tue, Jul 02, 2019 at 02:56:25PM +0800, JC Kuo wrote:
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information.  Any unauthorized review, use, disclosure or distribution
> is prohibited.  If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------

Footers like this are not allowed on public mailing lists, and forbid me
to respond to...

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[JC Kuo]

On 7/2/19 3:34 PM, Greg KH wrote:
> 
> Footers like this are not allowed on public mailing lists, and forbid me
> to respond to...
> 

Hi Greg,
I am truly sorry for that. I have just figured out how to tell mail server
not to add the footer. Please allow me to query again.

Since blacklisting uas kernel module is not a good idea and could break UAS 
capable storage functionality, although user-space should be blamed for the
improper configuration, do we consider forbidding making uas driver as
module? That means to make CONFIG_USB_UAS a bool option.

Thanks,
JC

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Greg KH]

On Tue, Jul 02, 2019 at 03:57:04PM +0800, JC Kuo wrote:
> On 7/2/19 3:34 PM, Greg KH wrote:
> > 
> > Footers like this are not allowed on public mailing lists, and forbid me
> > to respond to...
> > 
> 
> Hi Greg,
> I am truly sorry for that. I have just figured out how to tell mail server
> not to add the footer. Please allow me to query again.
> 
> Since blacklisting uas kernel module is not a good idea and could break UAS 
> capable storage functionality, although user-space should be blamed for the
> improper configuration, do we consider forbidding making uas driver as
> module? That means to make CONFIG_USB_UAS a bool option.

Step back and try to describe the real problem that you are having here.

Why is the kernel responsible for fixing a broken userspace
configuration?  What UAS devices are not working with Linux that could
cause someone to want to blacklist the uas driver and who is telling
them to do that?

Also see Oliver's response.

thanks,

greg k-h

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Oliver Neukum]

Am Dienstag, den 02.07.2019, 14:56 +0800 schrieb JC Kuo:
> 
> Since blacklisting uas kernel module is not a good idea and could break UAS

Then don't do it. If you don't want a driver loaded for a device
blacklisting the driver must not magically assign another driver.

> capable storage functionality, do we consider forbidding making uas driver
> as module? That means to make CONFIG_USB_UAS a bool option.

No. Absolutely not. We cannot force people to build UAS into their
kernel or not use it. Building either driver not at all, modular
or statically are all valid use cases. Just not building UAS must
trigger a fallback. And we must have a flag to override the kernel's
decision

Making driver assignments depend on module loading order is a very bad
idea. We also have the necessary quirk in one way. I would accept a
patch adding a flag to force usage of UAS, but other than that, the
existing code is as it must be.

	Regards
		Oliver

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[JC Kuo]

On 7/2/19 5:11 PM, Oliver Neukum wrote:
> Am Dienstag, den 02.07.2019, 14:56 +0800 schrieb JC Kuo:
>>
>> Since blacklisting uas kernel module is not a good idea and could break UAS
> 
> Then don't do it. If you don't want a driver loaded for a device
> blacklisting the driver must not magically assign another driver.
> 
>> capable storage functionality, do we consider forbidding making uas driver
>> as module? That means to make CONFIG_USB_UAS a bool option.
> 
> No. Absolutely not. We cannot force people to build UAS into their
> kernel or not use it. Building either driver not at all, modular
> or statically are all valid use cases. Just not building UAS must
> trigger a fallback. And we must have a flag to override the kernel's
> decision
> 
> Making driver assignments depend on module loading order is a very bad
> idea. We also have the necessary quirk in one way. I would accept a
> patch adding a flag to force usage of UAS, but other than that, the
> existing code is as it must be.
> 
> 	Regards
> 		Oliver
> 
Hi Greg and Oliver,
Thank you so much for your time and guidance. I understood that we can make
use of usb-storage quirks parameter to blacklist any UAS capable device.

I don't see the uas issue myself. I was trying to describe a situation that
user having issue with UAS storage and would like to blacklist uas module when
the user is not aware of the usb-storage quirks parameter.

https://marc.info/?l=linux-usb&m=143385909823645&w=2

UAS capable devices are backward-compatible with legacy Bulk-only protocol.
Therefore, IMHO, ideally if system software doesn't have UAS support, system
software should enable the UAS device with Bulk-only protocol, unless
usb-storage driver is not there as well.

Now the only valid way to disable UAS support for all UAS devices is to disable
CONFIG_USB_UAS option and rebuild kernel. Blacklisting uas driver is intuitive,
however it doesn't really disable UAS support but actually prevents UAS devices
to be functional even though usb-storage driver is there and the devices indeed
support Bulk-only protocol.



Thanks,
JC

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Oliver Neukum]

Am Dienstag, den 02.07.2019, 22:05 +0800 schrieb JC Kuo:

Hi,

> I don't see the uas issue myself. I was trying to describe a situation that
> user having issue with UAS storage and would like to blacklist uas module when
> the user is not aware of the usb-storage quirks parameter.
> 
> https://marc.info/?l=linux-usb&m=143385909823645&w=2

So we are not meeting the expectations of some users. Ideally we would.
Yet there are things we cannot do.

> UAS capable devices are backward-compatible with legacy Bulk-only protocol.
> Therefore, IMHO, ideally if system software doesn't have UAS support, system

Exact. At compilation time this is a valid consideration.

> software should enable the UAS device with Bulk-only protocol, unless
> usb-storage driver is not there as well.

What you could do, if we cannot change what the kernel does, is
improving documentation. We can at least tell users how it is done
correctly.

	Regards
		Oliver

Re: [PATCH] usb: storage: skip only when uas driver is loaded

[Oliver Neukum]

Am Dienstag, den 02.07.2019, 10:36 +0800 schrieb JC Kuo:

> An user might want to completely disable uas for some reason so he/she adds "blacklist uas" to modprobe conf file. I think in case of this, usb-storage driver has to enable this device with the legacy Bulk-only protocol instead of ignoring the device.
> 
> As an alternative to this patch, I thought I could get uas driver loaded before usb-storage driver so I tried moving the functions in drivers/usb/storage/uas-detect.h into uas.c and letting usb-storage links uas_use_uas_driver() of uas.ko. However, that didn't work because uas driver actually depends on usb-storage driver for usb_stor_adjust_quirks(). There will be a recursive dependency.
> 
> Please let me know if there is better approach to avoid the issue.

Use US_FL_IGNORE_UAS. You can trigger this with a module parameter.
We cannot just use another driver because the user block a module
load.

	Regards
		Oliver