Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi,
On Wed, Mar 11, 2015 at 02:21:38AM +0200, Ruslan Bilovol wrote:
> >> @@ -469,6 +488,16 @@ int usb_gadget_unregister_driver(struct
> >> usb_gadget_driver *driver)
> >> break;
> >> }
> >>
> >> + if (ret) {
> >> + struct usb_gadget_driver *tmp;
> >> +
> >> + list_for_each_entry(tmp, _driver_pending_list,
> >> pending)
> >> + if (tmp == driver) {
> >> + list_del(>pending);
> >> + ret = 0;
> >> + break;
> >> + }
> >> + }
> >
> > If you add the list_init and list_del_init above, this loop won't be
> > needed. You can just call list_del.
>
> I disagree with this. This function is externally visible and we can't
> guarantee that some buggy code will not call it with uninitialized
> 'pending' list_head. For example, if it never called usb_gadget_probe_driver()
> but calls usb_gadget_unregister_driver().
those cases deserve to suffer a really painfull and horrible death by
means of a kernel oops ;-) Sure, defensive programming and all, but at
some point we need to consider certain cases ridiculous and just not do
anything about them, so people know that they need more coffee and
attention while writing code.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Wed, 11 Mar 2015, Ruslan Bilovol wrote: > Hi Alan, Hello. > > If you add the list_init and list_del_init above, this loop won't be > > needed. You can just call list_del. > > I disagree with this. This function is externally visible and we can't > guarantee that some buggy code will not call it with uninitialized > 'pending' list_head. For example, if it never called usb_gadget_probe_driver() > but calls usb_gadget_unregister_driver(). > As per my opinion it's better to check it and return -ENODEV rather than > fail on deleting of uninitialized list_head. In this case adding the list_init > and list_del_init above is not needed. No, that is not the approach used in the rest of the kernel. We _want_ to know about bugs, so we can fix them. If you silently return -ENODEV then nobody will realize anything is wrong, but a big fat WARN or OOPS caused by an uninitialized list_head will draw people's attention very quickly. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi Alan,
On Tue, Feb 17, 2015 at 11:51 PM, Alan Stern wrote:
> On Tue, 17 Feb 2015, Ruslan Bilovol wrote:
>
>> Change behavior during registration of gadgets and
>> gadget drivers in udc-core. Instead of previous
>> approach when for successful probe of usb gadget driver
>> at least one usb gadget should be already registered
>> use another one where gadget drivers and gadgets
>> can be registered in udc-core independently.
>>
>> Independent registration of gadgets and gadget drivers
>> is useful for built-in into kernel gadget and gadget
>> driver case - because it's possible that gadget is
>> really probed only on late_init stage (due to deferred
>> probe) whereas gadget driver's probe is silently failed
>> on module_init stage due to no any UDC added.
>>
>> Also it is useful for modules case - now there is no
>> difference what module to insert first: gadget module
>> or gadget driver one.
>>
>> Signed-off-by: Ruslan Bilovol
>> ---
>> drivers/usb/gadget/udc/udc-core.c | 51
>> ++-
>> include/linux/usb/gadget.h| 2 ++
>> 2 files changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c
>> b/drivers/usb/gadget/udc/udc-core.c
>> index a960f3f..9e82497 100644
>> --- a/drivers/usb/gadget/udc/udc-core.c
>> +++ b/drivers/usb/gadget/udc/udc-core.c
>> @@ -48,8 +48,11 @@ struct usb_udc {
>>
>> static struct class *udc_class;
>> static LIST_HEAD(udc_list);
>> +static LIST_HEAD(gadget_driver_pending_list);
>> static DEFINE_MUTEX(udc_lock);
>>
>> +static int udc_bind_to_driver(struct usb_udc *udc,
>> + struct usb_gadget_driver *driver);
>
> Strange indentation, not at all like the other continuation lines in
> this source file.
>
> Also, there should be a blank line here, as in the original file.
Will fix it
>
>> /*
>> - */
>>
>> #ifdef CONFIG_HAS_DMA
>> @@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent,
>> struct usb_gadget *gadget,
>> void (*release)(struct device *dev))
>> {
>> struct usb_udc *udc;
>> + struct usb_gadget_driver *pgadget;
>
> Don't call it "pgadget". None of the other pointer variables in this
> file have an extra "p" added to the beginnings of their names.
It seems this name is confusing (it is intended to have "pending
gadget" meaning). Will change it in next patch version
>
>> int ret = -ENOMEM;
>>
>> udc = kzalloc(sizeof(*udc), GFP_KERNEL);
>> @@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent,
>> struct usb_gadget *gadget,
>>
>> usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
>>
>> + /* pick up one of pending gadget drivers */
>> + list_for_each_entry(pgadget, _driver_pending_list, pending) {
>> + if (!pgadget->udc_name || strcmp(pgadget->udc_name,
>> + dev_name(>dev)) == 0) {
>> + ret = udc_bind_to_driver(udc, pgadget);
>
> Are you sure it's safe to call this routine while holding the udc_lock
> mutex?
Yes, it's safe here, in the only place where it was used before
this routine is also called while holding the udc_lock mutex
(see usb_gadget_probe_driver)
>
>> + if (ret)
>> + goto err4;
>> + list_del(>pending);
>
> Use list_del_init().
>
>> + break;
>> + }
>> + }
>> +
>> mutex_unlock(_lock);
>>
>> return 0;
>> @@ -364,10 +380,11 @@ found:
>> dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
>>
>> list_del(>list);
>> - mutex_unlock(_lock);
>> -
>> - if (udc->driver)
>> + if (udc->driver) {
>> + list_add(>driver->pending, _driver_pending_list);
>> usb_gadget_remove_driver(udc);
>
> Are you sure it's safe to call this routine while holding the udc_lock
> mutex?
Should not be any issue here but it's possible to prevent this, will do it
in next patch version
>
>> + }
>> + mutex_unlock(_lock);
>>
>> kobject_uevent(>dev.kobj, KOBJ_REMOVE);
>> flush_work(>work);
>> @@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
>> *driver)
>> if (!ret)
>> break;
>> }
>> - if (ret)
>> - ret = -ENODEV;
>> - else if (udc->driver)
>> - ret = -EBUSY;
>> - else
>> + if (!ret && udc->driver) {
>> + mutex_unlock(_lock);
>> + return -EBUSY;
>
> This is a judgement call. It might be better to return 0 and add the
> gadget driver to the pending list.
Yes, that makes sense, will change it
>
>> + } else if (!ret) {
>> goto found;
>> + }
>> } else {
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi,
On Wed, Mar 11, 2015 at 02:21:38AM +0200, Ruslan Bilovol wrote:
@@ -469,6 +488,16 @@ int usb_gadget_unregister_driver(struct
usb_gadget_driver *driver)
break;
}
+ if (ret) {
+ struct usb_gadget_driver *tmp;
+
+ list_for_each_entry(tmp, gadget_driver_pending_list,
pending)
+ if (tmp == driver) {
+ list_del(driver-pending);
+ ret = 0;
+ break;
+ }
+ }
If you add the list_init and list_del_init above, this loop won't be
needed. You can just call list_del.
I disagree with this. This function is externally visible and we can't
guarantee that some buggy code will not call it with uninitialized
'pending' list_head. For example, if it never called usb_gadget_probe_driver()
but calls usb_gadget_unregister_driver().
those cases deserve to suffer a really painfull and horrible death by
means of a kernel oops ;-) Sure, defensive programming and all, but at
some point we need to consider certain cases ridiculous and just not do
anything about them, so people know that they need more coffee and
attention while writing code.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Wed, 11 Mar 2015, Ruslan Bilovol wrote: Hi Alan, Hello. If you add the list_init and list_del_init above, this loop won't be needed. You can just call list_del. I disagree with this. This function is externally visible and we can't guarantee that some buggy code will not call it with uninitialized 'pending' list_head. For example, if it never called usb_gadget_probe_driver() but calls usb_gadget_unregister_driver(). As per my opinion it's better to check it and return -ENODEV rather than fail on deleting of uninitialized list_head. In this case adding the list_init and list_del_init above is not needed. No, that is not the approach used in the rest of the kernel. We _want_ to know about bugs, so we can fix them. If you silently return -ENODEV then nobody will realize anything is wrong, but a big fat WARN or OOPS caused by an uninitialized list_head will draw people's attention very quickly. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi Alan,
On Tue, Feb 17, 2015 at 11:51 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Tue, 17 Feb 2015, Ruslan Bilovol wrote:
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 51
++-
include/linux/usb/gadget.h| 2 ++
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index a960f3f..9e82497 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -48,8 +48,11 @@ struct usb_udc {
static struct class *udc_class;
static LIST_HEAD(udc_list);
+static LIST_HEAD(gadget_driver_pending_list);
static DEFINE_MUTEX(udc_lock);
+static int udc_bind_to_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver);
Strange indentation, not at all like the other continuation lines in
this source file.
Also, there should be a blank line here, as in the original file.
Will fix it
/*
- */
#ifdef CONFIG_HAS_DMA
@@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
void (*release)(struct device *dev))
{
struct usb_udc *udc;
+ struct usb_gadget_driver *pgadget;
Don't call it pgadget. None of the other pointer variables in this
file have an extra p added to the beginnings of their names.
It seems this name is confusing (it is intended to have pending
gadget meaning). Will change it in next patch version
int ret = -ENOMEM;
udc = kzalloc(sizeof(*udc), GFP_KERNEL);
@@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
+ /* pick up one of pending gadget drivers */
+ list_for_each_entry(pgadget, gadget_driver_pending_list, pending) {
+ if (!pgadget-udc_name || strcmp(pgadget-udc_name,
+ dev_name(udc-dev)) == 0) {
+ ret = udc_bind_to_driver(udc, pgadget);
Are you sure it's safe to call this routine while holding the udc_lock
mutex?
Yes, it's safe here, in the only place where it was used before
this routine is also called while holding the udc_lock mutex
(see usb_gadget_probe_driver)
+ if (ret)
+ goto err4;
+ list_del(pgadget-pending);
Use list_del_init().
+ break;
+ }
+ }
+
mutex_unlock(udc_lock);
return 0;
@@ -364,10 +380,11 @@ found:
dev_vdbg(gadget-dev.parent, unregistering gadget\n);
list_del(udc-list);
- mutex_unlock(udc_lock);
-
- if (udc-driver)
+ if (udc-driver) {
+ list_add(udc-driver-pending, gadget_driver_pending_list);
usb_gadget_remove_driver(udc);
Are you sure it's safe to call this routine while holding the udc_lock
mutex?
Should not be any issue here but it's possible to prevent this, will do it
in next patch version
+ }
+ mutex_unlock(udc_lock);
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
flush_work(gadget-work);
@@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
if (!ret)
break;
}
- if (ret)
- ret = -ENODEV;
- else if (udc-driver)
- ret = -EBUSY;
- else
+ if (!ret udc-driver) {
+ mutex_unlock(udc_lock);
+ return -EBUSY;
This is a judgement call. It might be better to return 0 and add the
gadget driver to the pending list.
Yes, that makes sense, will change it
+ } else if (!ret) {
goto found;
+ }
} else {
list_for_each_entry(udc, udc_list, list) {
/* For now we take the first one */
if
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Tue, 17 Feb 2015, Ruslan Bilovol wrote:
> Change behavior during registration of gadgets and
> gadget drivers in udc-core. Instead of previous
> approach when for successful probe of usb gadget driver
> at least one usb gadget should be already registered
> use another one where gadget drivers and gadgets
> can be registered in udc-core independently.
>
> Independent registration of gadgets and gadget drivers
> is useful for built-in into kernel gadget and gadget
> driver case - because it's possible that gadget is
> really probed only on late_init stage (due to deferred
> probe) whereas gadget driver's probe is silently failed
> on module_init stage due to no any UDC added.
>
> Also it is useful for modules case - now there is no
> difference what module to insert first: gadget module
> or gadget driver one.
>
> Signed-off-by: Ruslan Bilovol
> ---
> drivers/usb/gadget/udc/udc-core.c | 51
> ++-
> include/linux/usb/gadget.h| 2 ++
> 2 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/udc-core.c
> b/drivers/usb/gadget/udc/udc-core.c
> index a960f3f..9e82497 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -48,8 +48,11 @@ struct usb_udc {
>
> static struct class *udc_class;
> static LIST_HEAD(udc_list);
> +static LIST_HEAD(gadget_driver_pending_list);
> static DEFINE_MUTEX(udc_lock);
>
> +static int udc_bind_to_driver(struct usb_udc *udc,
> + struct usb_gadget_driver *driver);
Strange indentation, not at all like the other continuation lines in
this source file.
Also, there should be a blank line here, as in the original file.
> /* -
> */
>
> #ifdef CONFIG_HAS_DMA
> @@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent,
> struct usb_gadget *gadget,
> void (*release)(struct device *dev))
> {
> struct usb_udc *udc;
> + struct usb_gadget_driver *pgadget;
Don't call it "pgadget". None of the other pointer variables in this
file have an extra "p" added to the beginnings of their names.
> int ret = -ENOMEM;
>
> udc = kzalloc(sizeof(*udc), GFP_KERNEL);
> @@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent,
> struct usb_gadget *gadget,
>
> usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
>
> + /* pick up one of pending gadget drivers */
> + list_for_each_entry(pgadget, _driver_pending_list, pending) {
> + if (!pgadget->udc_name || strcmp(pgadget->udc_name,
> + dev_name(>dev)) == 0) {
> + ret = udc_bind_to_driver(udc, pgadget);
Are you sure it's safe to call this routine while holding the udc_lock
mutex?
> + if (ret)
> + goto err4;
> + list_del(>pending);
Use list_del_init().
> + break;
> + }
> + }
> +
> mutex_unlock(_lock);
>
> return 0;
> @@ -364,10 +380,11 @@ found:
> dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
>
> list_del(>list);
> - mutex_unlock(_lock);
> -
> - if (udc->driver)
> + if (udc->driver) {
> + list_add(>driver->pending, _driver_pending_list);
> usb_gadget_remove_driver(udc);
Are you sure it's safe to call this routine while holding the udc_lock
mutex?
> + }
> + mutex_unlock(_lock);
>
> kobject_uevent(>dev.kobj, KOBJ_REMOVE);
> flush_work(>work);
> @@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
> *driver)
> if (!ret)
> break;
> }
> - if (ret)
> - ret = -ENODEV;
> - else if (udc->driver)
> - ret = -EBUSY;
> - else
> + if (!ret && udc->driver) {
> + mutex_unlock(_lock);
> + return -EBUSY;
This is a judgement call. It might be better to return 0 and add the
gadget driver to the pending list.
> + } else if (!ret) {
> goto found;
> + }
> } else {
> list_for_each_entry(udc, _list, list) {
> /* For now we take the first one */
> if (!udc->driver)
> goto found;
> }
> - ret = -ENODEV;
> }
>
> - pr_debug("couldn't find an available UDC\n");
> + list_add_tail(>pending, _driver_pending_list);
> + pr_info("udc-core: couldn't find an available UDC "
> + "- added [%s] to list of pending drivers\n",
> + driver->function);
> mutex_unlock(_lock);
> - return ret;
> + return 0;
[PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
Signed-off-by: Ruslan Bilovol
---
drivers/usb/gadget/udc/udc-core.c | 51 ++-
include/linux/usb/gadget.h| 2 ++
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index a960f3f..9e82497 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -48,8 +48,11 @@ struct usb_udc {
static struct class *udc_class;
static LIST_HEAD(udc_list);
+static LIST_HEAD(gadget_driver_pending_list);
static DEFINE_MUTEX(udc_lock);
+static int udc_bind_to_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver);
/* - */
#ifdef CONFIG_HAS_DMA
@@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
void (*release)(struct device *dev))
{
struct usb_udc *udc;
+ struct usb_gadget_driver *pgadget;
int ret = -ENOMEM;
udc = kzalloc(sizeof(*udc), GFP_KERNEL);
@@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
+ /* pick up one of pending gadget drivers */
+ list_for_each_entry(pgadget, _driver_pending_list, pending) {
+ if (!pgadget->udc_name || strcmp(pgadget->udc_name,
+ dev_name(>dev)) == 0) {
+ ret = udc_bind_to_driver(udc, pgadget);
+ if (ret)
+ goto err4;
+ list_del(>pending);
+ break;
+ }
+ }
+
mutex_unlock(_lock);
return 0;
@@ -364,10 +380,11 @@ found:
dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
list_del(>list);
- mutex_unlock(_lock);
-
- if (udc->driver)
+ if (udc->driver) {
+ list_add(>driver->pending, _driver_pending_list);
usb_gadget_remove_driver(udc);
+ }
+ mutex_unlock(_lock);
kobject_uevent(>dev.kobj, KOBJ_REMOVE);
flush_work(>work);
@@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
if (!ret)
break;
}
- if (ret)
- ret = -ENODEV;
- else if (udc->driver)
- ret = -EBUSY;
- else
+ if (!ret && udc->driver) {
+ mutex_unlock(_lock);
+ return -EBUSY;
+ } else if (!ret) {
goto found;
+ }
} else {
list_for_each_entry(udc, _list, list) {
/* For now we take the first one */
if (!udc->driver)
goto found;
}
- ret = -ENODEV;
}
- pr_debug("couldn't find an available UDC\n");
+ list_add_tail(>pending, _driver_pending_list);
+ pr_info("udc-core: couldn't find an available UDC "
+ "- added [%s] to list of pending drivers\n",
+ driver->function);
mutex_unlock(_lock);
- return ret;
+ return 0;
found:
ret = udc_bind_to_driver(udc, driver);
mutex_unlock(_lock);
@@ -469,6 +488,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver
*driver)
break;
}
+ if (ret) {
+ struct usb_gadget_driver *tmp;
+
+ list_for_each_entry(tmp, _driver_pending_list, pending)
+ if (tmp == driver) {
+ list_del(>pending);
+ ret = 0;
+ break;
+ }
+ }
mutex_unlock(_lock);
return ret;
}
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 6070e8d..3878425 100644
---
[PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 51 ++-
include/linux/usb/gadget.h| 2 ++
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index a960f3f..9e82497 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -48,8 +48,11 @@ struct usb_udc {
static struct class *udc_class;
static LIST_HEAD(udc_list);
+static LIST_HEAD(gadget_driver_pending_list);
static DEFINE_MUTEX(udc_lock);
+static int udc_bind_to_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver);
/* - */
#ifdef CONFIG_HAS_DMA
@@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
void (*release)(struct device *dev))
{
struct usb_udc *udc;
+ struct usb_gadget_driver *pgadget;
int ret = -ENOMEM;
udc = kzalloc(sizeof(*udc), GFP_KERNEL);
@@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
+ /* pick up one of pending gadget drivers */
+ list_for_each_entry(pgadget, gadget_driver_pending_list, pending) {
+ if (!pgadget-udc_name || strcmp(pgadget-udc_name,
+ dev_name(udc-dev)) == 0) {
+ ret = udc_bind_to_driver(udc, pgadget);
+ if (ret)
+ goto err4;
+ list_del(pgadget-pending);
+ break;
+ }
+ }
+
mutex_unlock(udc_lock);
return 0;
@@ -364,10 +380,11 @@ found:
dev_vdbg(gadget-dev.parent, unregistering gadget\n);
list_del(udc-list);
- mutex_unlock(udc_lock);
-
- if (udc-driver)
+ if (udc-driver) {
+ list_add(udc-driver-pending, gadget_driver_pending_list);
usb_gadget_remove_driver(udc);
+ }
+ mutex_unlock(udc_lock);
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
flush_work(gadget-work);
@@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
if (!ret)
break;
}
- if (ret)
- ret = -ENODEV;
- else if (udc-driver)
- ret = -EBUSY;
- else
+ if (!ret udc-driver) {
+ mutex_unlock(udc_lock);
+ return -EBUSY;
+ } else if (!ret) {
goto found;
+ }
} else {
list_for_each_entry(udc, udc_list, list) {
/* For now we take the first one */
if (!udc-driver)
goto found;
}
- ret = -ENODEV;
}
- pr_debug(couldn't find an available UDC\n);
+ list_add_tail(driver-pending, gadget_driver_pending_list);
+ pr_info(udc-core: couldn't find an available UDC
+ - added [%s] to list of pending drivers\n,
+ driver-function);
mutex_unlock(udc_lock);
- return ret;
+ return 0;
found:
ret = udc_bind_to_driver(udc, driver);
mutex_unlock(udc_lock);
@@ -469,6 +488,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver
*driver)
break;
}
+ if (ret) {
+ struct usb_gadget_driver *tmp;
+
+ list_for_each_entry(tmp, gadget_driver_pending_list, pending)
+ if (tmp == driver) {
+ list_del(driver-pending);
+ ret = 0;
+ break;
+ }
+ }
mutex_unlock(udc_lock);
return ret;
}
diff --git
Re: [PATCHv3 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Tue, 17 Feb 2015, Ruslan Bilovol wrote:
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 51
++-
include/linux/usb/gadget.h| 2 ++
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index a960f3f..9e82497 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -48,8 +48,11 @@ struct usb_udc {
static struct class *udc_class;
static LIST_HEAD(udc_list);
+static LIST_HEAD(gadget_driver_pending_list);
static DEFINE_MUTEX(udc_lock);
+static int udc_bind_to_driver(struct usb_udc *udc,
+ struct usb_gadget_driver *driver);
Strange indentation, not at all like the other continuation lines in
this source file.
Also, there should be a blank line here, as in the original file.
/* -
*/
#ifdef CONFIG_HAS_DMA
@@ -243,6 +246,7 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
void (*release)(struct device *dev))
{
struct usb_udc *udc;
+ struct usb_gadget_driver *pgadget;
Don't call it pgadget. None of the other pointer variables in this
file have an extra p added to the beginnings of their names.
int ret = -ENOMEM;
udc = kzalloc(sizeof(*udc), GFP_KERNEL);
@@ -288,6 +292,18 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
+ /* pick up one of pending gadget drivers */
+ list_for_each_entry(pgadget, gadget_driver_pending_list, pending) {
+ if (!pgadget-udc_name || strcmp(pgadget-udc_name,
+ dev_name(udc-dev)) == 0) {
+ ret = udc_bind_to_driver(udc, pgadget);
Are you sure it's safe to call this routine while holding the udc_lock
mutex?
+ if (ret)
+ goto err4;
+ list_del(pgadget-pending);
Use list_del_init().
+ break;
+ }
+ }
+
mutex_unlock(udc_lock);
return 0;
@@ -364,10 +380,11 @@ found:
dev_vdbg(gadget-dev.parent, unregistering gadget\n);
list_del(udc-list);
- mutex_unlock(udc_lock);
-
- if (udc-driver)
+ if (udc-driver) {
+ list_add(udc-driver-pending, gadget_driver_pending_list);
usb_gadget_remove_driver(udc);
Are you sure it's safe to call this routine while holding the udc_lock
mutex?
+ }
+ mutex_unlock(udc_lock);
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
flush_work(gadget-work);
@@ -426,24 +443,26 @@ int usb_gadget_probe_driver(struct usb_gadget_driver
*driver)
if (!ret)
break;
}
- if (ret)
- ret = -ENODEV;
- else if (udc-driver)
- ret = -EBUSY;
- else
+ if (!ret udc-driver) {
+ mutex_unlock(udc_lock);
+ return -EBUSY;
This is a judgement call. It might be better to return 0 and add the
gadget driver to the pending list.
+ } else if (!ret) {
goto found;
+ }
} else {
list_for_each_entry(udc, udc_list, list) {
/* For now we take the first one */
if (!udc-driver)
goto found;
}
- ret = -ENODEV;
}
- pr_debug(couldn't find an available UDC\n);
+ list_add_tail(driver-pending, gadget_driver_pending_list);
+ pr_info(udc-core: couldn't find an available UDC
+ - added [%s] to list of pending drivers\n,
+ driver-function);
mutex_unlock(udc_lock);
- return ret;
+ return 0;
found:
Call list_init(driver-pending) here. Or at
