| Message ID | 20201229224443.31623-1-jackp@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6cd0fe91387917be48e91385a572a69dfac2f3f7 |
| Headers | show |
| Series | [v2] usb: gadget: configfs: Preserve function ordering after bind failure | expand |
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?
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
> > > 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
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
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",