Hello Michal,
Thanks for the review, I generally agree, but have some comments below.
On Tuesday, March 27, 2012 3:42 PM Michał Nazarewicz wrote:
> On Tue, 27 Mar 2012 12:48:53 +0200, Andrzej Pietrasiewicz
> <andrzej.p@xxxxxxxxxxx> wrote:
> > From: Mike Lockwood <lockwood@xxxxxxxxxxx>
>
> > diff --git a/drivers/usb/gadget/android.c b/drivers/usb/gadget/android.c
> > @@ -0,0 +1,1065 @@
> > +/*
> > + * Gadget Driver for Android
> > + *
> > + * Copyright (C) 2008 Google, Inc.
> > + * Author: Mike Lockwood <lockwood@...>
> > + * Benoit Goby <benoit@...>
<snip>
> > +static int android_init_functions(struct android_usb_function **functions,
> > + struct usb_composite_dev *cdev)
> > +{
> > + struct android_dev *dev = _android_dev;
> > + struct android_usb_function *f;
> > + struct device_attribute **attrs;
> > + struct device_attribute *attr;
> > + int err;
> > + int index = 0;
> > +
> > + for (; (f = *functions++); index++) {
> > + f->dev_name = kasprintf(GFP_KERNEL, "f_%s", f->name);
>
> This may fail, no?
>
> > + f->dev = device_create(android_class, dev->dev,
> > + MKDEV(0, index), f, f->dev_name);
> > + if (IS_ERR(f->dev)) {
> > + pr_err("%s: Failed to create dev %s", __func__,
> > + f->dev_name);
> > + err = PTR_ERR(f->dev);
> > + goto err_create;
>
> Wouldn't that leak devices? I don't see where the devices are destroyed
> in error recovery path.
They are. If android_init_functions fails, then android_bind fails, then
composite_bind from composite.c fails and calls composite->unbind which
happens to be android_unbind, which calls android_cleanup_functions,
which does device_destroy.
<snip>
>
> > +static int android_bind(struct usb_composite_dev *cdev)
> > +{
> > + struct android_dev *dev = _android_dev;
> > + struct usb_gadget *gadget = cdev->gadget;
> > + int gcnum, id, ret;
> > +
> > + /*
> > + * Start disconnected. Userspace will connect the gadget once
> > + * it is done configuring the functions.
> > + */
> > + usb_gadget_disconnect(gadget);
> > +
> > + ret = android_init_functions(dev->functions, cdev);
> > + if (ret)
> > + return ret;
> > +
> > + /* Allocate string descriptor numbers ... note that string
> > + * contents can be overridden by the composite_dev glue.
> > + */
> > + id = usb_string_id(cdev);
> > + if (id < 0)
> > + return id;
> > + strings_dev[STRING_MANUFACTURER_IDX].id = id;
> > + device_desc.iManufacturer = id;
> > +
> > + id = usb_string_id(cdev);
> > + if (id < 0)
> > + return id;
> > + strings_dev[STRING_PRODUCT_IDX].id = id;
> > + device_desc.iProduct = id;
> > +
> > + /* Default strings - should be updated by userspace */
> > + strncpy(manufacturer_string, "Android", sizeof(manufacturer_string)-1);
> > + strncpy(product_string, "Android", sizeof(product_string) - 1);
>
> usb_composite_dev::iProduct and usb_composite_dev::iManufacturer are there so
> that
> gadget drivers don't have to deal with those strings directly. The above
> should
> be removed from android composite gadget as it can be handled by regular
> composite
> gadget.
>
The purpose of a number of sysfs attributes found in this driver is to enable
configuration at runtime while the driver is compiled-in. Module parameters
are no good in such a situation provided we don't want to/can't pass kernel
command line parameters. Please note that future works will probably be aimed
at switching to configfs, as Felipe suggests.
Andrzej
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
