Skip to contentSkip to conversation

uas: MS UAS Gadget driver - Infrastructure

PATCH In review Tatyana Brokhman 5 participants 15 messages Apr 14, 2011 – Apr 28, 2011

Subsystems (2)
USB Arm Qualcomm mailing list
lore
Share & tools
lore

Tatyana Brokhman

author
#0 lore raw

PATCH v3 2/5 uas: MS UAS Gadget driver - Infrastructure

This patch implements the infrastructure for the UAS gadget driver. The UAS gadget driver registers as a second configuration of the MS gadet driver.

A new module parameter was added to the mass_storage module:
bool use_uasp. (default = 0)
If this parameter is set to true, the mass_storage module will register with the UAS configuration as the devices first configuration and operate according to the UAS protocol.

The number of buffers used by the mass_storage device was increased according to the number of supported streams.

It defines the API for COMMAND/TASK MANAGEMENT IU implementation.

Signed-off-by Tatyana Brokhman <[email protected]>
drivers/usb/gadget/f_mass_storage.c +15 -11
index a688d04..e79ac84 100644
@@ -2734,7 +2734,8 @@ static inline void fsg_common_put(struct fsg_common *common)
 
 static struct fsg_common *fsg_common_init(struct fsg_common *common,
 					  struct usb_composite_dev *cdev,
-					  struct fsg_config *cfg)
+					  struct fsg_config *cfg,
+					  int start_thread)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 	struct fsg_buffhd *bh;

@@ -2883,12 +2884,14 @@ buffhds_first_it:
 	kref_init(&common->ref);
 
 	/* Tell the thread to start working */
-	common->thread_task =
-		kthread_create(fsg_main_thread, common,
+	if (start_thread) {
+		common->thread_task =
+			kthread_create(fsg_main_thread, common,
 			       cfg->thread_name ?: "file-storage");
-	if (IS_ERR(common->thread_task)) {
-		rc = PTR_ERR(common->thread_task);
-		goto error_release;
+		if (IS_ERR(common->thread_task)) {
+			rc = PTR_ERR(common->thread_task);
+			goto error_release;
+		}
 	}
 	init_completion(&common->thread_notifier);
 	init_waitqueue_head(&common->fsg_wait);

@@ -2919,10 +2922,11 @@ buffhds_first_it:
 	}
 	kfree(pathbuf);
 
-	DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task));
-
-	wake_up_process(common->thread_task);
-
+	if (start_thread) {
+		DBG(common, "I/O thread pid: %d\n",
+		    task_pid_nr(common->thread_task));
+		wake_up_process(common->thread_task);
+	}
 	return common;
 
 error_luns:

@@ -3190,6 +3194,6 @@ fsg_common_from_params(struct fsg_common *common,
 {
 	struct fsg_config cfg;
 	fsg_config_from_params(&cfg, params);
-	return fsg_common_init(common, cdev, &cfg);
+	return fsg_common_init(common, cdev, &cfg, 1);
 }
drivers/usb/gadget/f_uasp.c new file +2399 -0

2,399 lines not on this page · raw message

drivers/usb/gadget/f_uasp.h new file +436 -0

436 lines not on this page · raw message

drivers/usb/gadget/mass_storage.c +61 -6

100 lines not on this page · raw message

drivers/usb/gadget/storage_common.c +15 -4
index b015561..d33d73c 100644
@@ -220,6 +220,7 @@ struct interrupt_data {
 #define SS_UNRECOVERED_READ_ERROR		0x031100
 #define SS_WRITE_ERROR				0x030c02
 #define SS_WRITE_PROTECTED			0x072700
+#define SS_OVERLAPPED_COMMANDS_ATTEMPTED	0x0b4e00
 
 #define SK(x)		((u8) ((x) >> 16))	/* Sense Key byte, etc. */
 #define ASC(x)		((u8) ((x) >> 8))

@@ -262,8 +263,15 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev)
 #define EP0_BUFSIZE	256
 #define DELAYED_STATUS	(EP0_BUFSIZE + 999)	/* An impossibly large value */
 
-/* Number of buffers we will use.  2 is enough for double-buffering */
-#define FSG_NUM_BUFFERS	2
+/*
+ * We limit the number of UASP streams by 16 due to memory requirements.
+ * 4 buffer will be allocated for each supported stream.
[… more lines in the raw message]

Lines of this file are not shown. View the raw message.

drivers/usb/gadget/uasp_cmdiu.c new file +520 -0

520 lines not on this page · raw message

drivers/usb/gadget/uasp_tmiu.c new file +283 -0

283 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 9 lines
1.7.3.3

--
Sent by a Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Part of the diff is not shown here. View the raw message.

#1 lore raw
Quoted 9 lines · Tatyana Brokhman

This patch implements the infrastructure for the UAS gadget driver. The UAS gadget driver registers as a second configuration of the MS gadet driver.

A new module parameter was added to the mass_storage module:
bool use_uasp. (default = 0)
If this parameter is set to true, the mass_storage module will register with the UAS configuration as the devices first configuration and operate according to the UAS protocol.

As I understand it, drives typically have only one configuration and one interface. They implement Bulk-Only Transport on altsetting 0 and UAS on altsetting 1.

On the other hand, I don't think there's anything that says you _have_ to do it that way...

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
In reply to Tatyana Brokhman
Quoted 9 lines · Tatyana Brokhman

This patch implements the infrastructure for the UAS gadget driver. The UAS gadget driver registers as a second configuration of the MS gadet driver.

A new module parameter was added to the mass_storage module:
bool use_uasp. (default = 0)
If this parameter is set to true, the mass_storage module will register with the UAS configuration as the devices first configuration and operate according to the UAS protocol.

Ick, I'd _really_ prefer no new kernel module parameters. It gets messy and a pain to support and document (see you didn't document it...)

Any way to do this "automatically"? Or some other way?

thanks,

greg k-h

author
#3 lore raw
Quoted 8 lines
Quoted 4 lines

A new module parameter was added to the mass_storage module:
bool use_uasp. (default = 0)
If this parameter is set to true, the mass_storage module will

register

Quoted 2 lines

with the UAS configuration as the devices first configuration and operate according to the UAS protocol.

Quoted 7 lines

Ick, I'd _really_ prefer no new kernel module parameters. It gets messy
and a pain to support and document (see you didn't document it...)

Any way to do this "automatically"? Or some other way?

thanks,

We've added this parameter as a first development phase to help us in debugging/testing. The gadget driver is functional and feature complete at the moment but we're still working on some improvements and further testing. The module parameter will be removed in the final version.

Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

#4 lore raw
In reply to Tatyana Brokhman
Quoted 13 lines · Tatyana Brokhman
drivers/usb/gadget/f_uasp.c new file +10 -0
@@ -0,0 +1,2399 @@
+/*
+ * f_uasp.c -- Mass Storage USB UASP Composite Function
+ *
+ * Copyright (C) 2003-2005 Alan Stern
+ * Copyright (C) 2011 Code Aurora Forum.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.

drivers/usb/gadget/f_uasp.c line 20

Quoted 10 lines
drivers/usb/gadget/f_uasp.c new file +10 -0
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.

Minor nit. Never include this last paragraph in your code unless you guarantee you are willing to keep up with the office moves of the FSF for the next 40+ years and promise to update this file as well.

It's pointless and has nothing to do with kernel development stuff, just leave it out please.

thanks,

greg k-h

#5 lore raw
In reply to Tatyana Brokhman
Quoted 9 lines · Tatyana Brokhman

This patch implements the infrastructure for the UAS gadget driver. The UAS gadget driver registers as a second configuration of the MS gadet driver.

A new module parameter was added to the mass_storage module:
bool use_uasp. (default = 0)
If this parameter is set to true, the mass_storage module will register with the UAS configuration as the devices first configuration and operate according to the UAS protocol.

I'm a bit confused by this statement. Do you mean the UAS alternate interface setting, rather than the UAS configuration? I thought that UAS devices would have one configuration with two alternate interface
settings: a bulk-only-transport interface, and a UAS interface.

Wasn't there a requirement in the USB-IF UAS compliance test that UAS devices have the UAS alternate interface setting as the second alternate interface setting, to make sure OSes without UAS support would still be able to operate with the device? Is that what you're doing in these patches?

The other comment I saw was that you chose to run a thread per LUN:

Quoted 7 lines

+ * Several kernel threads are created as part of the init sequence:
+ * - UASP main thread
+ * - A thread for each of the existing LUNs
+ * The UASP main thread handles all of the generic commands/task
+ * management requests and routes LUN specific requests to be handled by
+ * the appropriate LUNs task.
+ * The approach of "task per LUN" was chosen due to the UAS protocol

Quoted 8 lines

+ * enhancement over the BOT protocol. The main retouch of the UAS
+ * protocol of the BOT protocol is the fact that independent commands can
+ * be performed in parallel. For example a READ command for two different
+ * LUNS. Thus in order to implement this concurrency a separate thread is
+ * needed for each of the existing LUNS.
+ * As long as the LUN threads are alive they keep an open reference to the
+ * backing file. This prevents the unmounting of the backing file's
+ * underlying file system and cause problems during system shutdown.

Why not a thread per SCSI command TAG (i.e. per stream ID)? I think that most USB storage devices only have one LUN. You would get better performance if you could, say, kick off several READ commands for the same LUN when you receive two separate SCSI commands on different stream IDs.

I think the SCSI host-side layer will ensure that all outstanding asynchronous commands complete before issuing a command that overlaps with previous commands. For example, if it issues a WRITE for sector 2, a READ of sector 1, and then the host-side SCSI stack receives a WRITE for sectors 1-3, then the SCSI stack will wait for the first two commands to complete. Wouldn't you want those first two commands to be working in parallel on different threads?

Sarah Sharp

author
#6 lore raw

Hi Sarah

We're on holidays here in Israel and will be back at the office on the 26.04.
I went over your comments and addressing them requires some checking and looking into the code, which I'll do as soon as I get back to the office next week.
In the meanwhile - thanks you for your inputs!

Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

author
#7 lore raw
In reply to Sarah Sharp

Hi Sarah

Quoted 17 lines · Tatyana Brokhman
Quoted 7 lines

This patch implements the infrastructure for the UAS gadget driver. The UAS gadget driver registers as a second configuration of the MS gadet driver.

A new module parameter was added to the mass_storage module:
bool use_uasp. (default = 0)
If this parameter is set to true, the mass_storage module will

register

Quoted 2 lines

with the UAS configuration as the devices first configuration and operate according to the UAS protocol.

I'm a bit confused by this statement. Do you mean the UAS alternate interface setting, rather than the UAS configuration? I thought that UAS devices would have one configuration with two alternate interface
settings: a bulk-only-transport interface, and a UAS interface.

Quoted 6 lines

Wasn't there a requirement in the USB-IF UAS compliance test that UAS devices have the UAS alternate interface setting as the second alternate
interface setting, to make sure OSes without UAS support would still be able to operate with the device? Is that what you're doing in these patches?

Our idea was that UAS supporting devices register with 2 configurations (not a second alternate setting to the MS interface). Upon enumeration the host will choose the operational mode/protocol (BOT/UAS) by choosing the configuration it wishes to operate in. As a first development phase it was easier for us to register only 1 configuration and that's why we added the module parameter. We're now working on removing this hack and registering 2 configurations the host can choose from. From what I saw in the host side implementation the host always prefers the first configuration so right now we set the UAS config from user-space via sysfs.

I looked into USB-IF UAS compliance test document and you're right. There is something that mentions "Configuration Descriptor shall report B NUM INTERFACES > 0". Unfortunately in our version of the file this is the only reference I found for this demand so I need to look into that and get more details.
In the meanwhile: will this work with the existing UAS driver implementation? How will I be able to "force" the host to choose the UAS protocol? I can compile just cdc_ncm but I prefer to have my host support both UAS and BOT and be able to choose. As mentioned before, at the moment I achieved that by manually setting the device configuration to UAS from user space.

Quoted 24 lines

The other comment I saw was that you chose to run a thread per LUN:

Quoted 5 lines

+ * Several kernel threads are created as part of the init sequence:
+ * - UASP main thread
+ * - A thread for each of the existing LUNs
+ * The UASP main thread handles all of the generic commands/task
+ * management requests and routes LUN specific requests to be

handled by

Quoted 4 lines

+ * the appropriate LUNs task.
+ * The approach of "task per LUN" was chosen due to the UAS protocol
+ * enhancement over the BOT protocol. The main retouch of the UAS
+ * protocol of the BOT protocol is the fact that independent

commands can

Quoted 1 line

+ * be performed in parallel. For example a READ command for two

different

Quoted 1 line

+ * LUNS. Thus in order to implement this concurrency a separate

thread is

Quoted 2 lines

+ * needed for each of the existing LUNS.
+ * As long as the LUN threads are alive they keep an open reference

to the

Quoted 2 lines

+ * backing file. This prevents the unmounting of the backing file's
+ * underlying file system and cause problems during system shutdown.

Quoted 6 lines

Why not a thread per SCSI command TAG (i.e. per stream ID)? I think that most USB storage devices only have one LUN. You would get better performance if you could, say, kick off several READ commands for the same LUN when you receive two separate SCSI commands on different stream
IDs.

I wasn't aware of the "one LUN" issue. I'm afraid that creating a thread per SCSI command won't be that efficient because we will spend too much CPU on
1. creating/killing the thread
2. context switch between the threads
Perhaps the better solution will be the combination of the two approaches:
create X threads that will handle the received SCSI commands. This way X can be a maximum between number of LUNS and some max_threads_val we decide on.
How does this sound?

Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

#8 lore raw
Quoted 10 lines · Tanya Brokhman

Our idea was that UAS supporting devices register with 2 configurations (not a second alternate setting to the MS interface). Upon enumeration the host will choose the operational mode/protocol (BOT/UAS) by choosing the configuration it wishes to operate in. As a first development phase it was easier for us to register only 1 configuration and that's why we added the module parameter. We're now working on removing this hack and registering 2 configurations the host can choose from. From what I saw in the host side implementation the host always prefers the first configuration so right now we set the UAS config from user-space via sysfs.

Quoted 5 lines

I looked into USB-IF UAS compliance test document and you're right. There is something that mentions "Configuration Descriptor shall report B NUM INTERFACES > 0". Unfortunately in our version of the file this is the only reference I found for this demand so I need to look into that and get more details.

Ok, I'll double check with the people I know from the compliance team. I remember the requirement from the discussion of the UAS compliance test on the USB-IF website. Do you have access to the UAS workgroup?

Quoted 6 lines

In the meanwhile: will this work with the existing UAS driver implementation? How will I be able to "force" the host to choose the UAS protocol? I can compile just cdc_ncm but I prefer to have my host support both UAS and BOT and be able to choose. As mentioned before, at the moment I achieved that by manually setting the device configuration to UAS from user space.

Unfortunately, Linux will choose the first configuration and bind whatever driver that matches that configuration to the device. So if the BOT configuration is configuration 1, then the BOT driver will be loaded. Same with alternate interface settings. Alternate interface 0 will always be chosen first during enumeration.

We need a way to switch the UAS device from the BOT driver to the UAS driver, but I'm not sure how to do that. I think Alan might have had some ideas?

Quoted 7 lines
Quoted 6 lines

Why not a thread per SCSI command TAG (i.e. per stream ID)? I think that most USB storage devices only have one LUN. You would get better performance if you could, say, kick off several READ commands for the same LUN when you receive two separate SCSI commands on different stream
IDs.

Quoted 9 lines

I wasn't aware of the "one LUN" issue. I'm afraid that creating a thread per SCSI command won't be that efficient because we will spend too much CPU on
1. creating/killing the thread
2. context switch between the threads
Perhaps the better solution will be the combination of the two approaches:
create X threads that will handle the received SCSI commands. This way X can be a maximum between number of LUNS and some max_threads_val we decide on.
How does this sound?

Sure, that sounds like the right direction. You can choose how many threads you want to support (X), and then set the bMaxStreams value in the descriptor to that value. That way the host will only queue X number of SCSI commands, and use X number of stream IDs, and each thread can handle one SCSI command. Or if you want each thread to be able to handle C number of commands, you could set bMaxStreams value to C*X.

Right now the UAS driver attempts to allocate 65536 streams (which translates to allowing the SCSI core to submit up to 65536 commands asynchronously), but the xHCI driver will only let it allocate the minimum number of streams that each of the three endpoints advertise. Probably the UAS driver needs to update the .can_queue value for the SCSI host after looking at the device descriptors. I'm not sure if that means the SCSI core can submit up to .can_queue commands for each LUN, or up to .can_queue commands for all LUNs.

Sarah Sahrp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw
Quoted 17 lines · Sarah Sharp
Quoted 10 lines · Tanya Brokhman

Our idea was that UAS supporting devices register with 2 configurations (not a second alternate setting to the MS interface). Upon enumeration the host will choose the operational mode/protocol (BOT/UAS) by choosing the configuration it wishes to operate in. As a first development phase it was easier for us to register only 1 configuration and that's why we added the module parameter. We're now working on removing this hack and registering 2 configurations the host can choose from. From what I saw in the host side implementation the host always prefers the first configuration so right now we set the UAS config from user-space via sysfs.

Quoted 5 lines

I looked into USB-IF UAS compliance test document and you're right. There is something that mentions "Configuration Descriptor shall report B NUM INTERFACES > 0". Unfortunately in our version of the file this is the only reference I found for this demand so I need to look into that and get more details.

Quoted 3 lines

Ok, I'll double check with the people I know from the compliance team. I remember the requirement from the discussion of the UAS compliance test on the USB-IF website. Do you have access to the UAS workgroup?

For what it's worth, my old copy of the UAS spec (working draft Rev. 2) says nothing about this one way or another. Evidently it was thought that vendors would implement dual-protocol support any way they wanted.

Quoted 7 lines
Quoted 6 lines

In the meanwhile: will this work with the existing UAS driver implementation? How will I be able to "force" the host to choose the UAS protocol? I can compile just cdc_ncm but I prefer to have my host support both UAS and BOT and be able to choose. As mentioned before, at the moment I achieved that by manually setting the device configuration to UAS from user space.

Quoted 5 lines

Unfortunately, Linux will choose the first configuration and bind whatever driver that matches that configuration to the device. So if the BOT configuration is configuration 1, then the BOT driver will be loaded. Same with alternate interface settings. Alternate interface 0 will always be chosen first during enumeration.

The part about configurations is right, but the part about altsettings is wrong. The USB core does not choose altsettings; drivers do. The driver that binds to an interface will select whichever altsetting it wants.

Quoted 3 lines

We need a way to switch the UAS device from the BOT driver to the UAS driver, but I'm not sure how to do that. I think Alan might have had some ideas?

One can always unbind usb-storage from an interface and bind uas to that interface by hand, using sysfs. At the moment there doesn't appear to be any mechanism for doing this automatically. For example, usb-storage _could_ choose not to bind to an interface if there's a UAS altsetting -- but currently it doesn't take that into account.

Which reminds me... Fallbacks are always a good idea. If usb-storage did decide not to bind to combined BOT/UAS devices, we should have a mechanism for overriding this choice (i.e., forcing usb-storage to bind regardless).

Likewise, Sarah, you should consider adding a mechanism to xhci-hcd for forcing individual root-hub ports not to run at SuperSpeed (rather like the "companion" attribute file in ehci-hcd, although I'm sure you can come up with a better name).

Quoted 24 lines
Quoted 7 lines
Quoted 6 lines

Why not a thread per SCSI command TAG (i.e. per stream ID)? I think that most USB storage devices only have one LUN. You would get better performance if you could, say, kick off several READ commands for the same LUN when you receive two separate SCSI commands on different stream
IDs.

Quoted 9 lines

I wasn't aware of the "one LUN" issue. I'm afraid that creating a thread per SCSI command won't be that efficient because we will spend too much CPU on
1. creating/killing the thread
2. context switch between the threads
Perhaps the better solution will be the combination of the two approaches:
create X threads that will handle the received SCSI commands. This way X can be a maximum between number of LUNS and some max_threads_val we decide on.
How does this sound?

Sure, that sounds like the right direction. You can choose how many threads you want to support (X), and then set the bMaxStreams value in the descriptor to that value. That way the host will only queue X number of SCSI commands, and use X number of stream IDs, and each thread can handle one SCSI command. Or if you want each thread to be able to handle C number of commands, you could set bMaxStreams value to C*X.

Quoted 8 lines

Right now the UAS driver attempts to allocate 65536 streams (which translates to allowing the SCSI core to submit up to 65536 commands asynchronously), but the xHCI driver will only let it allocate the minimum number of streams that each of the three endpoints advertise. Probably the UAS driver needs to update the .can_queue value for the SCSI host after looking at the device descriptors. I'm not sure if that means the SCSI core can submit up to .can_queue commands for each LUN, or up to .can_queue commands for all LUNs.

For all LUNs.

Since Tanya is writing the gadget driver, she gets to decide how many streams it will support (limited by the device controller hardware). In this setting I would use one thread per stream, since a thread can't have more than one VFS read or write request outstanding at any time.

Except that, as Christoph has mentioned a few times, it would be better to abandon this driver design entirely and use the SCSI Target framework instead. Then presumably threads would not be an issue.

Alan Stern

#10 lore raw
Quoted 19 lines · Alan Stern
Quoted 17 lines · Sarah Sharp
Quoted 15 lines · Tanya Brokhman

Our idea was that UAS supporting devices register with 2 configurations (not a second alternate setting to the MS interface). Upon enumeration the host will choose the operational mode/protocol (BOT/UAS) by choosing the configuration it wishes to operate in. As a first development phase it was easier for us to register only 1 configuration and that's why we added the module parameter. We're now working on removing this hack and registering 2 configurations the host can choose from. From what I saw in the host side implementation the host always prefers the first configuration so right now we set the UAS config from user-space via sysfs.

I looked into USB-IF UAS compliance test document and you're right. There is something that mentions "Configuration Descriptor shall report B NUM INTERFACES > 0". Unfortunately in our version of the file this is the only reference I found for this demand so I need to look into that and get more details.

Quoted 7 lines
Quoted 3 lines

Ok, I'll double check with the people I know from the compliance team. I remember the requirement from the discussion of the UAS compliance test on the USB-IF website. Do you have access to the UAS workgroup?

For what it's worth, my old copy of the UAS spec (working draft Rev. 2) says nothing about this one way or another. Evidently it was thought that vendors would implement dual-protocol support any way they wanted.

Yeah, I'm looking over the UAS compliance spec, and I only see the mention of making sure the bNumInterfaces is greater than 0 (which only means there has to be at least one interface). The specific discussion I remember was about how to make sure a Windows install without a UAS driver would fall back to BOT. They wanted to make it a requirement that BOT be the first alternate interface setting, but it looks like that never made it into the compliance spec. So yes, Tanya can implement it however she wants.

Quoted 13 lines
Quoted 7 lines
Quoted 6 lines

In the meanwhile: will this work with the existing UAS driver implementation? How will I be able to "force" the host to choose the UAS protocol? I can compile just cdc_ncm but I prefer to have my host support both UAS and BOT and be able to choose. As mentioned before, at the moment I achieved that by manually setting the device configuration to UAS from user space.

Quoted 5 lines

Unfortunately, Linux will choose the first configuration and bind whatever driver that matches that configuration to the device. So if the BOT configuration is configuration 1, then the BOT driver will be loaded. Same with alternate interface settings. Alternate interface 0 will always be chosen first during enumeration.

Quoted 4 lines

The part about configurations is right, but the part about altsettings is wrong. The USB core does not choose altsettings; drivers do. The driver that binds to an interface will select whichever altsetting it wants.

I meant that during enumeration, configuration 1 would be installed, and because the USB core doesn't try to install a particular alternate interface setting, alt setting 0 would be active by default.

Quoted 9 lines
Quoted 3 lines

We need a way to switch the UAS device from the BOT driver to the UAS driver, but I'm not sure how to do that. I think Alan might have had some ideas?

One can always unbind usb-storage from an interface and bind uas to that interface by hand, using sysfs. At the moment there doesn't appear to be any mechanism for doing this automatically. For example, usb-storage _could_ choose not to bind to an interface if there's a UAS altsetting -- but currently it doesn't take that into account.

How would the usb-storage driver reject a bind by the USB core? By returning an error from the probe function? Would the USB core go and search for the next driver after the BOT driver rejected the bind? It looks like usb_probe_interface will just return an error if the first driver's probe function fails.

Quoted 4 lines

Which reminds me... Fallbacks are always a good idea. If usb-storage did decide not to bind to combined BOT/UAS devices, we should have a mechanism for overriding this choice (i.e., forcing usb-storage to bind regardless).

Sure, maybe a module parameter like "own_uas"? Or do we want something fancier, like a way to specify a list of VID:PIDs that the usb-storage driver should own? (I think the list parsing might be a bit hard to implement though.)

Quoted 4 lines

Likewise, Sarah, you should consider adding a mechanism to xhci-hcd for forcing individual root-hub ports not to run at SuperSpeed (rather like the "companion" attribute file in ehci-hcd, although I'm sure you can come up with a better name).

I'm not entirely sure I can force a port down to USB 2.0 speeds, because I'm not sure I can disable the port or turn off SuperSpeed terminations from the xHCI driver. I'd have to look into it.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#11 lore raw
Quoted 8 lines · Sarah Sharp
Quoted 5 lines

One can always unbind usb-storage from an interface and bind uas to that interface by hand, using sysfs. At the moment there doesn't appear to be any mechanism for doing this automatically. For example, usb-storage _could_ choose not to bind to an interface if there's a UAS altsetting -- but currently it doesn't take that into account.

How would the usb-storage driver reject a bind by the USB core? By returning an error from the probe function?

Yes.

Quoted 2 lines

Would the USB core go and search for the next driver after the BOT driver rejected the bind?

Yes.

Quoted 2 lines

It looks like usb_probe_interface will just return an error if the first driver's probe function fails.

The driver core continues on and will probe the remaining drivers for that bus. Also, when a new driver shows up, the driver core asks if any of the unbound devices should be bound to this new driver.

So you should be fine.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw
In reply to Sarah Sharp
Quoted 3 lines · Sarah Sharp

I meant that during enumeration, configuration 1 would be installed, and because the USB core doesn't try to install a particular alternate interface setting, alt setting 0 would be active by default.

Unless the driver changed it.

Quoted 5 lines

How would the usb-storage driver reject a bind by the USB core? By returning an error from the probe function? Would the USB core go and search for the next driver after the BOT driver rejected the bind? It looks like usb_probe_interface will just return an error if the first driver's probe function fails.

If a driver's or subsystem's probe routine returns an error then the driver core continues looking for another driver. If the error code is -ENODEV or -ENXIO then it doesn't even put a warning in the kernel log; the driver core takes this to mean that the driver detected it couldn't handle the device. See drivers/base/dd.c:really_probe().

Quoted 9 lines
Quoted 4 lines

Which reminds me... Fallbacks are always a good idea. If usb-storage did decide not to bind to combined BOT/UAS devices, we should have a mechanism for overriding this choice (i.e., forcing usb-storage to bind regardless).

Sure, maybe a module parameter like "own_uas"? Or do we want something fancier, like a way to specify a list of VID:PIDs that the usb-storage driver should own? (I think the list parsing might be a bit hard to implement though.)

The VID:PID thing would work; usb-storage already has a "quirks" parameter that accepts such things. We could add a quirk for binding to a BOT/UAS interface.

Quoted 8 lines
Quoted 4 lines

Likewise, Sarah, you should consider adding a mechanism to xhci-hcd for forcing individual root-hub ports not to run at SuperSpeed (rather like the "companion" attribute file in ehci-hcd, although I'm sure you can come up with a better name).

I'm not entirely sure I can force a port down to USB 2.0 speeds, because I'm not sure I can disable the port or turn off SuperSpeed terminations from the xHCI driver. I'd have to look into it.

This sounds like the problem we encountered while trying to disable a SuperSpeed port. It merely forced the device to switch over to the USB-2 bus. That wasn't what we wanted then, but it is what we're interested in now.

Getting things to switch back might be harder -- re-enabling the SuperSpeed terminations won't force the device to stop using the USB-2 bus. It might be necessary to reset the USB-2 port.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#13 lore raw
In reply to Alan Stern

Hi All

Quoted 11 lines

Since Tanya is writing the gadget driver, she gets to decide how many streams it will support (limited by the device controller hardware). In this setting I would use one thread per stream, since a thread can't have more than one VFS read or write request outstanding at any time.

Except that, as Christoph has mentioned a few times, it would be better to abandon this driver design entirely and use the SCSI Target framework instead. Then presumably threads would not be an issue.

Alan Stern

Thank you very much for you inputs and help! I'm not familiar with the SCSI Target framework Christoph referred me too so I postponed looking into that until the driver is stable. Well, I guess that time has come so I'll switch over to studying that and applying it to the UAS gadget driver.

Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

#14 lore raw
Quoted 4 lines · Tanya Brokhman

Thank you very much for you inputs and help! I'm not familiar with the SCSI Target framework Christoph referred me too so I postponed looking into that until the driver is stable. Well, I guess that time has come so I'll switch over to studying that and applying it to the UAS gadget driver.

I'm not familiar with that framework either, so if you find any good introductory documentation (other than the files in Documentation/target) please post a link to it. Nicholas Bellinger is very willing to answer questions.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html