[PATCH v2] usb: gadget: configfs: Preserve function ordering after bind failure

[PATCH v2] usb: gadget: configfs: Preserve function ordering after bind failure

[Jack Pham]

From: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>

When binding the ConfigFS gadget to a UDC, the functions in each
configuration are added in list order. However, if usb_add_function()
fails, the failed function is put back on its configuration's
func_list and purge_configs_funcs() is called to further clean up.

purge_configs_funcs() iterates over the configurations and functions
in forward order, calling unbind() on each of the previously added
functions. But after doing so, each function gets moved to the
tail of the configuration's func_list. This results in reshuffling
the original order of the functions within a configuration such
that the failed function now appears first even though it may have
originally appeared in the middle or even end of the list. At this
point if the ConfigFS gadget is attempted to re-bind to the UDC,
the functions will be added in a different order than intended,
with the only recourse being to remove and relink the functions all
over again.

An example of this as follows:

ln -s functions/mass_storage.0 configs/c.1
ln -s functions/ncm.0 configs/c.1
ln -s functions/ffs.adb configs/c.1	# oops, forgot to start adbd
echo "<udc device>" > UDC		# fails
start adbd
echo "<udc device>" > UDC		# now succeeds, but...
					# bind order is
					# "ADB", mass_storage, ncm

[30133.118289] configfs-gadget gadget: adding 'Mass Storage Function'/ffffff810af87200 to config 'c'/ffffff817d6a2520
[30133.119875] configfs-gadget gadget: adding 'cdc_network'/ffffff80f48d1a00 to config 'c'/ffffff817d6a2520
[30133.119974] using random self ethernet address
[30133.120002] using random host ethernet address
[30133.139604] usb0: HOST MAC 3e:27:46:ba:3e:26
[30133.140015] usb0: MAC 6e:28:7e:42:66:6a
[30133.140062] configfs-gadget gadget: adding 'Function FS Gadget'/ffffff80f3868438 to config 'c'/ffffff817d6a2520
[30133.140081] configfs-gadget gadget: adding 'Function FS Gadget'/ffffff80f3868438 --> -19
[30133.140098] configfs-gadget gadget: unbind function 'Mass Storage Function'/ffffff810af87200
[30133.140119] configfs-gadget gadget: unbind function 'cdc_network'/ffffff80f48d1a00
[30133.173201] configfs-gadget a600000.dwc3: failed to start g1: -19
[30136.661933] init: starting service 'adbd'...
[30136.700126] read descriptors
[30136.700413] read strings
[30138.574484] configfs-gadget gadget: adding 'Function FS Gadget'/ffffff80f3868438 to config 'c'/ffffff817d6a2520
[30138.575497] configfs-gadget gadget: adding 'Mass Storage Function'/ffffff810af87200 to config 'c'/ffffff817d6a2520
[30138.575554] configfs-gadget gadget: adding 'cdc_network'/ffffff80f48d1a00 to config 'c'/ffffff817d6a2520
[30138.575631] using random self ethernet address
[30138.575660] using random host ethernet address
[30138.595338] usb0: HOST MAC 2e:cf:43:cd:ca:c8
[30138.597160] usb0: MAC 6a:f0:9f:ee:82:a0
[30138.791490] configfs-gadget gadget: super-speed config #1: c

Fix this by reversing the iteration order of the functions in
purge_config_funcs() when unbinding them, and adding them back to
the config's func_list at the head instead of the tail. This
ensures that we unbind and unwind back to the original list order.

Fixes: 88af8bbe4ef7 ("usb: gadget: the start of the configfs interface")
Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
v2: rebase on gregkh's usb-testing

 drivers/usb/gadget/configfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 56051bb97349..3dda1d63f231 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1248,9 +1248,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
 
 		cfg = container_of(c, struct config_usb_cfg, c);
 
-		list_for_each_entry_safe(f, tmp, &c->functions, list) {
+		list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
 
-			list_move_tail(&f->list, &cfg->func_list);
+			list_move(&f->list, &cfg->func_list);
 			if (f->unbind) {
 				dev_dbg(&gi->cdev.gadget->dev,
 					"unbind function '%s'/%p\n",
-- 
2.24.0

Re: [PATCH v2] usb: gadget: configfs: Preserve function ordering after bind failure

[Peter Chen]

On 20-12-29 14:44:43, Jack Pham wrote:
> From: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> 
> When binding the ConfigFS gadget to a UDC, the functions in each
> configuration are added in list order. However, if usb_add_function()
> fails, the failed function is put back on its configuration's
> func_list and purge_configs_funcs() is called to further clean up.
> 
> purge_configs_funcs() iterates over the configurations and functions
> in forward order, calling unbind() on each of the previously added
> functions. But after doing so, each function gets moved to the
> tail of the configuration's func_list. This results in reshuffling
> the original order of the functions within a configuration such
> that the failed function now appears first even though it may have
> originally appeared in the middle or even end of the list. At this
> point if the ConfigFS gadget is attempted to re-bind to the UDC,
> the functions will be added in a different order than intended,
> with the only recourse being to remove and relink the functions all
> over again.
> 
> An example of this as follows:
> 
> ln -s functions/mass_storage.0 configs/c.1
> ln -s functions/ncm.0 configs/c.1
> ln -s functions/ffs.adb configs/c.1	# oops, forgot to start adbd
> echo "<udc device>" > UDC		# fails
> start adbd
> echo "<udc device>" > UDC		# now succeeds, but...
> 					# bind order is
> 					# "ADB", mass_storage, ncm
> 
> [30133.118289] configfs-gadget gadget: adding 'Mass Storage Function'/ffffff810af87200 to config 'c'/ffffff817d6a2520
> [30133.119875] configfs-gadget gadget: adding 'cdc_network'/ffffff80f48d1a00 to config 'c'/ffffff817d6a2520
> [30133.119974] using random self ethernet address
> [30133.120002] using random host ethernet address
> [30133.139604] usb0: HOST MAC 3e:27:46:ba:3e:26
> [30133.140015] usb0: MAC 6e:28:7e:42:66:6a
> [30133.140062] configfs-gadget gadget: adding 'Function FS Gadget'/ffffff80f3868438 to config 'c'/ffffff817d6a2520
> [30133.140081] configfs-gadget gadget: adding 'Function FS Gadget'/ffffff80f3868438 --> -19
> [30133.140098] configfs-gadget gadget: unbind function 'Mass Storage Function'/ffffff810af87200
> [30133.140119] configfs-gadget gadget: unbind function 'cdc_network'/ffffff80f48d1a00
> [30133.173201] configfs-gadget a600000.dwc3: failed to start g1: -19
> [30136.661933] init: starting service 'adbd'...
> [30136.700126] read descriptors
> [30136.700413] read strings
> [30138.574484] configfs-gadget gadget: adding 'Function FS Gadget'/ffffff80f3868438 to config 'c'/ffffff817d6a2520
> [30138.575497] configfs-gadget gadget: adding 'Mass Storage Function'/ffffff810af87200 to config 'c'/ffffff817d6a2520
> [30138.575554] configfs-gadget gadget: adding 'cdc_network'/ffffff80f48d1a00 to config 'c'/ffffff817d6a2520
> [30138.575631] using random self ethernet address
> [30138.575660] using random host ethernet address
> [30138.595338] usb0: HOST MAC 2e:cf:43:cd:ca:c8
> [30138.597160] usb0: MAC 6a:f0:9f:ee:82:a0
> [30138.791490] configfs-gadget gadget: super-speed config #1: c
> 
> Fix this by reversing the iteration order of the functions in
> purge_config_funcs() when unbinding them, and adding them back to
> the config's func_list at the head instead of the tail. This
> ensures that we unbind and unwind back to the original list order.
> 
> Fixes: 88af8bbe4ef7 ("usb: gadget: the start of the configfs interface")
> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
> v2: rebase on gregkh's usb-testing
> 
>  drivers/usb/gadget/configfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 56051bb97349..3dda1d63f231 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1248,9 +1248,9 @@ static void purge_configs_funcs(struct gadget_info *gi)
>  
>  		cfg = container_of(c, struct config_usb_cfg, c);
>  
> -		list_for_each_entry_safe(f, tmp, &c->functions, list) {
> +		list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
>  
> -			list_move_tail(&f->list, &cfg->func_list);
> +			list_move(&f->list, &cfg->func_list);
>  			if (f->unbind) {
>  				dev_dbg(&gi->cdev.gadget->dev,
>  					"unbind function '%s'/%p\n",
> -- 

Hi Jack,

I am curious what features are broken if the functions are added with
not planned order?

-- 

Thanks,
Peter Chen

Re: [PATCH v2] usb: gadget: configfs: Preserve function ordering after bind failure

[Jack Pham]

On Thu, Dec 31, 2020 at 06:04:00PM +0800, Peter Chen wrote:
> On 20-12-29 14:44:43, Jack Pham wrote:
> > From: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> > 
> > When binding the ConfigFS gadget to a UDC, the functions in each
> > configuration are added in list order. However, if usb_add_function()
> > fails, the failed function is put back on its configuration's
> > func_list and purge_configs_funcs() is called to further clean up.
> > 
> > purge_configs_funcs() iterates over the configurations and functions
> > in forward order, calling unbind() on each of the previously added
> > functions. But after doing so, each function gets moved to the
> > tail of the configuration's func_list. This results in reshuffling
> > the original order of the functions within a configuration such
> > that the failed function now appears first even though it may have
> > originally appeared in the middle or even end of the list. At this
> > point if the ConfigFS gadget is attempted to re-bind to the UDC,
> > the functions will be added in a different order than intended,
> > with the only recourse being to remove and relink the functions all
> > over again.

<snip>

> Hi Jack,
> 
> I am curious what features are broken if the functions are added with
> not planned order?

Hi Peter,

This is mainly an issue for devices with functions that use vendor-
specific instead of standard class/subclass IDs for their interface
descriptors. Android ADB and Qualcomm QMI are a couple examples. So
host interface drivers would only be able to install or bind based on
matching VID/PID and interface position only. This is true for both
Windows as well as Linux (see USB_DEVICE_INTERFACE_NUMBER).

So if the gadget's function bind order gets jumbled up from the intended
order, the resulting assigned interface numbers would be different and
the host would not match its drivers to the correct interface. Instead
we see the host driver gets bound but the endpoints it communicates with
are wrong as they are for a completely different interface.

Thanks,
Jack

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v2] usb: gadget: configfs: Preserve function ordering after bind failure

[Peter Chen]

 
> 
> > Hi Jack,
> >
> > I am curious what features are broken if the functions are added with
> > not planned order?
> 
> Hi Peter,
> 
> This is mainly an issue for devices with functions that use vendor- specific
> instead of standard class/subclass IDs for their interface descriptors. Android
> ADB and Qualcomm QMI are a couple examples. So host interface drivers
> would only be able to install or bind based on matching VID/PID and interface
> position only. This is true for both Windows as well as Linux (see
> USB_DEVICE_INTERFACE_NUMBER).
> 
> So if the gadget's function bind order gets jumbled up from the intended order,
> the resulting assigned interface numbers would be different and the host would
> not match its drivers to the correct interface. Instead we see the host driver
> gets bound but the endpoints it communicates with are wrong as they are for a
> completely different interface.
> 
> Thanks,
> Jack
 
Thanks for your information, jack. But does Android ADB has specific host driver, I am
assumed it uses drivers/usb/core/devio.c, am I wrong?

Peter

Re: [PATCH v2] usb: gadget: configfs: Preserve function ordering after bind failure

[Jack Pham]

Hi Peter,

On Mon, Jan 04, 2021 at 12:58:39AM +0000, Peter Chen wrote:
> > > Hi Jack,
> > >
> > > I am curious what features are broken if the functions are added with
> > > not planned order?
> > 
> > Hi Peter,
> > 
> > This is mainly an issue for devices with functions that use vendor- specific
> > instead of standard class/subclass IDs for their interface descriptors. Android
> > ADB and Qualcomm QMI are a couple examples. So host interface drivers
> > would only be able to install or bind based on matching VID/PID and interface
> > position only. This is true for both Windows as well as Linux (see
> > USB_DEVICE_INTERFACE_NUMBER).
> > 
> > So if the gadget's function bind order gets jumbled up from the intended order,
> > the resulting assigned interface numbers would be different and the host would
> > not match its drivers to the correct interface. Instead we see the host driver
> > gets bound but the endpoints it communicates with are wrong as they are for a
> > completely different interface.
>  
> Thanks for your information, jack. But does Android ADB has specific host driver, I am
> assumed it uses drivers/usb/core/devio.c, am I wrong?

That is true for the Linux ADB client; it does use usbdevfs directly,
and it does match based on the vendor Class/Subclass/Protocol so it's
generally not a problem for ADB connected to Linux hosts.

However for the Windows adb.exe client, it uses the native WinUSB
driver which typically needs an associated INF file to declare what
VID/PID/Interface is to be used for the ADB interface. (Alternatively,
the WinUSB driver can be automatically installed if the device provides
MSFT OS Descriptors during enumeration to indicate if an interface uses
WinUSB, so that might avoid requiring an INF file.) But regardless, in
either case the installed driver information gets stored in the Windows
registry and is always tied to VID/PID/Interface, so subsequent
connections to the same device must always enumerate in the same
function order otherwise the problem I stated above will happen.

Further, if a device has a QMI interface (we have downstream QMI
function drivers that many commercial Android devices use) that
drivers/net/usb/qmi_wwan.c is expecting to bind to, because the majority
of the usb_device_id entries in this driver uses QMI_FIXED_INTF /
USB_DEVICE_INTERFACE_NUMBER, these devices could unfortunately suffer
this problem if the function bind order is not guaranteed and the
interface appears in a different position than expected.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project