On Wednesday, June 12, 2013 5:17 PM Felipe Balbi wrote: > Hi, > > This is a note to let you know that the patch: > > usb: gadget: cdc2: convert to new interface of f_ecm > > has been applied to my tree and can be found at: > > http://bit.ly/172DRHC > > a snapshot of the tree can be downloaded from: > > http://bit.ly/11zcltc > > Note that it might take up to 20 minutes for kernel.org to synchronize the > changes to its frontend servers. > > If you have any concerns, please let me know by replying to this mail. >
Hi Felipe, During testing the gadgets I discovered a strange behavior of the cdc.ko gadget. It turns out that what has been applied to your tree is different from what I have submitted. The main difference (which I suppose causes trouble) is that usb_get_function_instance() is called from cdc_bind() (which is good and intended) and then again its invocation is repeated in cdc_do_config (which is bad). Originally Sebastian's conversion of acm to new function interface did usb_get_function_instance() only in one place, cdc_do_config(). Later on I discovered that the code is much cleaner if the following convention is obeyed: usb_get_function_instance() is called only from xxx_bind(), while usb_get_function() is called only from xxx_do_config(). The patch I submitted: http://www.spinics.net/lists/linux-usb/msg86326.html indeed follows the said convention. However, what has been applied does not. I reverted the patch in question and applied it again after making necessary corrections so that it is consistent with what I have submitted. After squashing the two a simple patch results and I'm sending it - in a separate mail. Please see below for what looks bad to me: > From a38a275030086d95306555e544fc7c0e65ccd00e Mon Sep 17 00:00:00 2001 > From: Andrzej Pietrasiewicz <[email protected]> > Date: Thu, 23 May 2013 10:32:04 +0200 > Subject: [PATCH] usb: gadget: cdc2: convert to new interface of f_ecm > > f_ecm has been converted to new configfs infrastructure, fixing cdc2 > gadget driver. > > [ [email protected] : fixed a bunch of errors when adding ECM function ] > > Signed-off-by: Andrzej Pietrasiewicz <[email protected]> > Signed-off-by: Kyungmin Park <[email protected]> > Signed-off-by: Felipe Balbi <[email protected]> > --- > drivers/usb/gadget/Kconfig | 1 + > drivers/usb/gadget/cdc2.c | 86 +++++++++++++++++++++++++++++++---------- > ----- > 2 files changed, 60 insertions(+), 27 deletions(-) > <snip> > diff --git a/drivers/usb/gadget/cdc2.c b/drivers/usb/gadget/cdc2.c index > ceedaf7..5a5acf2 100644 > --- a/drivers/usb/gadget/cdc2.c > +++ b/drivers/usb/gadget/cdc2.c > @@ -15,6 +15,7 @@ > <snip> > /* > * We _always_ have both CDC ECM and CDC ACM functions. > */ > @@ -122,13 +113,27 @@ static int __init cdc_do_config(struct > usb_configuration *c) > c->bmAttributes |= USB_CONFIG_ATT_WAKEUP; > } > > - status = ecm_bind_config(c, host_mac, the_dev); > - if (status < 0) > - return status; > + fi_ecm = usb_get_function_instance("ecm"); When control reaches this point the usb_get_function_instance("ecm") has already been called in cdc_bind > + if (IS_ERR(fi_ecm)) { > + status = PTR_ERR(fi_ecm); > + goto err_func_ecm; > + } > + > + f_ecm = usb_get_function(fi_ecm); > + if (IS_ERR(f_ecm)) { > + status = PTR_ERR(f_ecm); > + goto err_get_ecm; > + } > + > + status = usb_add_function(c, f_ecm); > + if (status) > + goto err_add_ecm; > > fi_serial = usb_get_function_instance("acm"); When control reaches this point the usb_get_function_instance("acm") has already been called in cdc_bind > - if (IS_ERR(fi_serial)) > - return PTR_ERR(fi_serial); > + if (IS_ERR(fi_serial)) { > + status = PTR_ERR(fi_serial); > + goto err_get_acm; > + } > > f_acm = usb_get_function(fi_serial); > if (IS_ERR(f_acm)) { > @@ -138,12 +143,21 @@ static int __init cdc_do_config(struct > usb_configuration *c) > > status = usb_add_function(c, f_acm); > if (status) > - goto err_conf; > + goto err_add_acm; > + > return 0; > -err_conf: > + > +err_add_acm: > usb_put_function(f_acm); > err_func_acm: > usb_put_function_instance(fi_serial); > +err_get_acm: > + usb_remove_function(c, f_ecm); > +err_add_ecm: > + usb_put_function(f_ecm); > +err_get_ecm: > + usb_put_function_instance(fi_ecm); > +err_func_ecm: > return status; > } > > @@ -159,6 +173,7 @@ static struct usb_configuration cdc_config_driver = > { static int __init cdc_bind(struct usb_composite_dev *cdev) { > struct usb_gadget *gadget = cdev->gadget; > + struct f_ecm_opts *ecm_opts; > int status; > > if (!can_support_ecm(cdev->gadget)) { > @@ -167,11 +182,23 @@ static int __init cdc_bind(struct usb_composite_dev > *cdev) > return -EINVAL; > } > > - /* set up network link layer */ > - the_dev = gether_setup(cdev->gadget, dev_addr, host_addr, host_mac, > - qmult); > - if (IS_ERR(the_dev)) > - return PTR_ERR(the_dev); > + fi_ecm = usb_get_function_instance("ecm"); This is ok, we get the instance in cdc_bind and get the function in cdc_do_config > + if (IS_ERR(fi_ecm)) > + return PTR_ERR(fi_ecm); > + > + ecm_opts = container_of(fi_ecm, struct f_ecm_opts, func_inst); > + > + gether_set_qmult(ecm_opts->net, qmult); > + if (!gether_set_host_addr(ecm_opts->net, host_addr)) > + pr_info("using host ethernet address: %s", host_addr); > + if (!gether_set_dev_addr(ecm_opts->net, dev_addr)) > + pr_info("using self ethernet address: %s", dev_addr); > + > + fi_serial = usb_get_function_instance("acm"); This is ok, we get the instance in cdc_bind and get the function in cdc_do_config <snip> Thanks, Andrzej -- 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
