RE: [PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
> > > > > > > An idea just struck me... Instead of looping through all the > > > > > udc's to find the right one, why not simply store a pointer to > > > > > the udc in struct usb_gadget? > > > > > > > > > > > We still have code to find usb_gadget_driver to iterate udc_list, it > > > is better to change all, we need to have solution to consolidate > > > struct usb_udc, struct usb_gadget, and struct usb_gadget_drver. > > > I have no good solution now. > > > > I think it's okay to iterate through the list to find a > > usb_gadget_driver. But it's silly to iterate to find the usb_udc > > corresponding to a usb_gadget. > > Felipe, what's your opinion? > Hi Felipe, would you have comments for it? - Follow Alan's suggestion - Adding a new API like this patch - Drop this patch, and still iterate udc_list to get usb_gadget Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
On Mon, Jan 19, 2015 at 11:29:22AM -0500, Alan Stern wrote: > On Mon, 19 Jan 2015, Peter Chen wrote: > > > > > An idea just struck me... Instead of looping through all the udc's to > > > > find the right one, why not simply store a pointer to the udc in struct > > > > usb_gadget? > > > > > > > > We still have code to find usb_gadget_driver to iterate udc_list, it > > is better to change all, we need to have solution to consolidate > > struct usb_udc, struct usb_gadget, and struct usb_gadget_drver. > > I have no good solution now. > > I think it's okay to iterate through the list to find a > usb_gadget_driver. But it's silly to iterate to find the usb_udc > corresponding to a usb_gadget. Felipe, what's your opinion? > > > > > Also, it looks like there's a bug in usb_add_gadget_udc_release() in > > > > udc-core.c. The error pathway (between err3 and err2) does not undo > > > > the > > > > > > > > ret = device_register(&gadget->dev); > > > > > > > > call. There's a put_device() call but no device_del(). > > > > > > good point :-) Do you want to send a patch for that ? > > > > > > > It looks like there is another issue in error pathway, the > > put_device(&udc->dev) at err3 should be moved to err4 since > > the get_device is called at device_add. > > No, the get_device is done by device_initialize, so the code is correct > as it stands. See the kerneldoc for device_initialize() in > drivers/base/core.c. > > Alan Stern > Yes, you are right, just device_initialize calls kobject_init to set refcount as 1, not call get_device directly. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
On Mon, 19 Jan 2015, Peter Chen wrote: > > > An idea just struck me... Instead of looping through all the udc's to > > > find the right one, why not simply store a pointer to the udc in struct > > > usb_gadget? > > > > > We still have code to find usb_gadget_driver to iterate udc_list, it > is better to change all, we need to have solution to consolidate > struct usb_udc, struct usb_gadget, and struct usb_gadget_drver. > I have no good solution now. I think it's okay to iterate through the list to find a usb_gadget_driver. But it's silly to iterate to find the usb_udc corresponding to a usb_gadget. > > > Also, it looks like there's a bug in usb_add_gadget_udc_release() in > > > udc-core.c. The error pathway (between err3 and err2) does not undo > > > the > > > > > > ret = device_register(&gadget->dev); > > > > > > call. There's a put_device() call but no device_del(). > > > > good point :-) Do you want to send a patch for that ? > > > > It looks like there is another issue in error pathway, the > put_device(&udc->dev) at err3 should be moved to err4 since > the get_device is called at device_add. No, the get_device is done by device_initialize, so the code is correct as it stands. See the kerneldoc for device_initialize() in drivers/base/core.c. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
On Fri, Jan 16, 2015 at 10:15:45AM -0600, Felipe Balbi wrote:
> On Fri, Jan 16, 2015 at 11:05:07AM -0500, Alan Stern wrote:
> > On Fri, 16 Jan 2015, Peter Chen wrote:
> >
> > > This is an internal API, and is used to find corresponding udc according
> > > to gadget.
> > >
> > > Signed-off-by: Peter Chen
> > > ---
> > > drivers/usb/gadget/udc/udc-core.c | 51
> > > ---
> > > 1 file changed, 26 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/udc/udc-core.c
> > > b/drivers/usb/gadget/udc/udc-core.c
> > > index e31d574..36c58c9 100644
> > > --- a/drivers/usb/gadget/udc/udc-core.c
> > > +++ b/drivers/usb/gadget/udc/udc-core.c
> > > @@ -52,6 +52,25 @@ static DEFINE_MUTEX(udc_lock);
> > >
> > > /*
> > > -
> > > */
> > >
> > > +static struct usb_udc *usb_gadget_find_udc(struct usb_gadget *gadget)
> > > +{
> > > + struct usb_udc *udc = NULL;
> > > +
> > > + mutex_lock(&udc_lock);
> > > + list_for_each_entry(udc, &udc_list, list)
> > > + if (udc->gadget == gadget)
> > > + goto found;
> > > + mutex_unlock(&udc_lock);
> > > + dev_err(gadget->dev.parent, "gadget not registered.\n");
> > > +
> > > + return NULL;
> > > +
> > > +found:
> > > + mutex_unlock(&udc_lock);
> > > + return udc;
> > > +}
> >
> > An idea just struck me... Instead of looping through all the udc's to
> > find the right one, why not simply store a pointer to the udc in struct
> > usb_gadget?
> >
We still have code to find usb_gadget_driver to iterate udc_list, it
is better to change all, we need to have solution to consolidate
struct usb_udc, struct usb_gadget, and struct usb_gadget_drver.
I have no good solution now.
> > Also, it looks like there's a bug in usb_add_gadget_udc_release() in
> > udc-core.c. The error pathway (between err3 and err2) does not undo
> > the
> >
> > ret = device_register(&gadget->dev);
> >
> > call. There's a put_device() call but no device_del().
>
> good point :-) Do you want to send a patch for that ?
>
It looks like there is another issue in error pathway, the
put_device(&udc->dev) at err3 should be moved to err4 since
the get_device is called at device_add.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
On Fri, Jan 16, 2015 at 11:05:07AM -0500, Alan Stern wrote:
> On Fri, 16 Jan 2015, Peter Chen wrote:
>
> > This is an internal API, and is used to find corresponding udc according
> > to gadget.
> >
> > Signed-off-by: Peter Chen
> > ---
> > drivers/usb/gadget/udc/udc-core.c | 51
> > ---
> > 1 file changed, 26 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/udc-core.c
> > b/drivers/usb/gadget/udc/udc-core.c
> > index e31d574..36c58c9 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -52,6 +52,25 @@ static DEFINE_MUTEX(udc_lock);
> >
> > /*
> > - */
> >
> > +static struct usb_udc *usb_gadget_find_udc(struct usb_gadget *gadget)
> > +{
> > + struct usb_udc *udc = NULL;
> > +
> > + mutex_lock(&udc_lock);
> > + list_for_each_entry(udc, &udc_list, list)
> > + if (udc->gadget == gadget)
> > + goto found;
> > + mutex_unlock(&udc_lock);
> > + dev_err(gadget->dev.parent, "gadget not registered.\n");
> > +
> > + return NULL;
> > +
> > +found:
> > + mutex_unlock(&udc_lock);
> > + return udc;
> > +}
>
> An idea just struck me... Instead of looping through all the udc's to
> find the right one, why not simply store a pointer to the udc in struct
> usb_gadget?
>
> Also, it looks like there's a bug in usb_add_gadget_udc_release() in
> udc-core.c. The error pathway (between err3 and err2) does not undo
> the
>
> ret = device_register(&gadget->dev);
>
> call. There's a put_device() call but no device_del().
good point :-) Do you want to send a patch for that ?
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
On Fri, 16 Jan 2015, Peter Chen wrote:
> This is an internal API, and is used to find corresponding udc according
> to gadget.
>
> Signed-off-by: Peter Chen
> ---
> drivers/usb/gadget/udc/udc-core.c | 51
> ---
> 1 file changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/udc-core.c
> b/drivers/usb/gadget/udc/udc-core.c
> index e31d574..36c58c9 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -52,6 +52,25 @@ static DEFINE_MUTEX(udc_lock);
>
> /* -
> */
>
> +static struct usb_udc *usb_gadget_find_udc(struct usb_gadget *gadget)
> +{
> + struct usb_udc *udc = NULL;
> +
> + mutex_lock(&udc_lock);
> + list_for_each_entry(udc, &udc_list, list)
> + if (udc->gadget == gadget)
> + goto found;
> + mutex_unlock(&udc_lock);
> + dev_err(gadget->dev.parent, "gadget not registered.\n");
> +
> + return NULL;
> +
> +found:
> + mutex_unlock(&udc_lock);
> + return udc;
> +}
An idea just struck me... Instead of looping through all the udc's to
find the right one, why not simply store a pointer to the udc in struct
usb_gadget?
Also, it looks like there's a bug in usb_add_gadget_udc_release() in
udc-core.c. The error pathway (between err3 and err2) does not undo
the
ret = device_register(&gadget->dev);
call. There's a put_device() call but no device_del().
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] usb: udc: add usb_gadget_find_udc
This is an internal API, and is used to find corresponding udc according
to gadget.
Signed-off-by: Peter Chen
---
drivers/usb/gadget/udc/udc-core.c | 51 ---
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index e31d574..36c58c9 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -52,6 +52,25 @@ static DEFINE_MUTEX(udc_lock);
/* - */
+static struct usb_udc *usb_gadget_find_udc(struct usb_gadget *gadget)
+{
+ struct usb_udc *udc = NULL;
+
+ mutex_lock(&udc_lock);
+ list_for_each_entry(udc, &udc_list, list)
+ if (udc->gadget == gadget)
+ goto found;
+ mutex_unlock(&udc_lock);
+ dev_err(gadget->dev.parent, "gadget not registered.\n");
+
+ return NULL;
+
+found:
+ mutex_unlock(&udc_lock);
+ return udc;
+}
+
+/* - */
#ifdef CONFIG_HAS_DMA
int usb_gadget_map_request(struct usb_gadget *gadget,
@@ -128,21 +147,10 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
static void usb_gadget_state_work(struct work_struct *work)
{
- struct usb_gadget *gadget = work_to_gadget(work);
- struct usb_udc *udc = NULL;
-
- mutex_lock(&udc_lock);
- list_for_each_entry(udc, &udc_list, list)
- if (udc->gadget == gadget)
- goto found;
- mutex_unlock(&udc_lock);
-
- return;
-
-found:
- mutex_unlock(&udc_lock);
+ struct usb_udc *udc = usb_gadget_find_udc(work_to_gadget(work));
- sysfs_notify(&udc->dev.kobj, NULL, "state");
+ if (udc)
+ sysfs_notify(&udc->dev.kobj, NULL, "state");
}
void usb_gadget_set_state(struct usb_gadget *gadget,
@@ -348,21 +356,14 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
*/
void usb_del_gadget_udc(struct usb_gadget *gadget)
{
- struct usb_udc *udc = NULL;
-
- mutex_lock(&udc_lock);
- list_for_each_entry(udc, &udc_list, list)
- if (udc->gadget == gadget)
- goto found;
-
- dev_err(gadget->dev.parent, "gadget not registered.\n");
- mutex_unlock(&udc_lock);
+ struct usb_udc *udc = usb_gadget_find_udc(gadget);
- return;
+ if (!udc)
+ return;
-found:
dev_vdbg(gadget->dev.parent, "unregistering gadget\n");
+ mutex_lock(&udc_lock);
list_del(&udc->list);
mutex_unlock(&udc_lock);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
