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

[Peter Chen <hzpeterchen@gmail.com>]

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