* Michal Nazarewicz | 2011-05-23 18:54:50 [+0200]:
>>>On Fri, 20 May 2011 12:02:59 +0200, Sebastian Andrzej Siewior wrote:
>>>>+struct usb_udc {
>>>>+ struct usb_gadget_driver *driver;
>>>>+ struct usb_gadget *gadget;
>>>>+ struct device dev;
>>>>+ struct list_head list;
>>>>+};
>
>>* Michal Nazarewicz | 2011-05-20 20:28:59 [+0200]:
>>>maybe it's better to just stash those fields inside usb_gadget
>>>structure?
>
>On Mon, 23 May 2011 16:45:51 +0200, Sebastian Andrzej Siewior wrote:
>>I don't think so. This could lead to the access those members directly
>>and bypass the udc_class for what ever reasons.
>
>I don't see it as a problem. UDC drivers have a driver pointer
>anyway, and have no need to touch UDC class device nor the list.
>
>Moreover, if we choose to just add UDC dev and list to usb_gadget,
>some of the functions in the UDC class will be much simpler. Some of
>the list traversals could be easily avoided.
>
>>>Also, isn't it that UDC drivers store pointer to usb_gadget_driver
>>>internally?
>
>>Just checked and I can't see it. The ops pointer is passed/set.
>
>Three more or less random UDC drivers:
>
>http://lxr.linux.no/linux+*/drivers/usb/gadget/s3c-hsotg.c#L147
>http://lxr.linux.no/linux+*/drivers/usb/gadget/at91_udc.h#L130
>http://lxr.linux.no/linux+*/drivers/usb/gadget/mv_udc.h#L165
>
>and all store pointer to struct usb_gadget_driver.
>
>Therefore, stashing fields of the usb_udc structure inside usb_gadget
>structure will save us storing pointer to usb_gadget_driver two times.
Now I got it. Let me try to stash it and see where it goes :)
>>>>+int usb_add_gadget_udc(struct device *parent, struct
>>>>usb_gadget *gadget)
>>>>+{
>>>>+ struct usb_udc *udc;
>>>>+ int ret = -ENOMEM;
>>>>+
>>>>+ udc = kzalloc(sizeof(*udc), GFP_KERNEL);
>>>>+ if (!udc)
>>>>+ goto err1;
>>>>+
>>>>+ device_initialize(&udc->dev);
>>>>+ udc->dev.release = usb_udc_release;
>>>>+ udc->dev.class = udc_class;
>>>>+ udc->dev.parent = parent;
>>>>+ ret = dev_set_name(&udc->dev, "%s", kobject_name(&parent->kobj));
>>>>+ if (ret)
>>>>+ goto err2;
>>>>+
>>>>+ udc->gadget = gadget;
>>>>+
>>>>+ mutex_lock(&udc_lock);
>
>>>mutex_lock_interruptible()? This applies to other places as well.
>
>>Why do we want to abort on signal?
>
>Why don't we want to do that? If something hands, user should be able
>to interrupt
>operation with a signal, I don't see why she shouldn't.
It looks like duct tape. She should not run into dead locks.
So lets see: rmmod on a module, module try to loock udc_lock, we get a
signal, we fail with an error code, the module is removed (the driver
remains registered).
>>>>+void usb_del_gadget_udc(struct usb_gadget *gadget)
>>>>+{
>>>>+ struct usb_udc *udc = NULL;
>>>>+
>>>>+ dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
>>>>+
>>>>+ mutex_lock(&udc_lock);
>>>>+ list_for_each_entry(udc, &udc_list, list) {
>>>>+ if (udc->gadget == gadget)
>>>>+ goto found;
>>>>+ }
>
>>>The braces are not needed.
>
>>yes, but it looks somehow naked since we have another function where we
>>omit the braces.
>
>Coding style requires the braces be dropped though.
>http://lxr.linux.no/linux+*/Documentation/CodingStyle#L166
Since you insist, I'm going to change this.
>>>>+ dev_err(gadget->dev.parent, "gadget not registered.\n");
>>>>+ goto out;
>>>>+found:
>>>>+
>>>>+ list_del(&udc->list);
>>>>+
>>>>+ if (udc->driver)
>>>>+ usb_gadget_remove_driver(udc);
>>>>+
>>>>+ kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
>>>>+ device_unregister(&udc->dev);
>>>>+out:
>>>>+ mutex_unlock(&udc_lock);
>
>>>I think the mutex_unlock() can be put just after list_del().
>
>>It think, it could but then it is no longer symetric with that part
>>where we lock the complete function.
>
>I'd still go with mutex_unlock() just after the list_del(). No need
>to hold mutexes that we don't need.
Okay, will do.
Sebastian
--
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
