[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Benjamin Herrenschmidt]

Some UDC may want to allocate endpoints dynamically, either because
the HW supports an arbitrary large number or because (like the Aspeed
BMC SoCs), the pool of HW endpoints is shared between multiple gadgets.

The allocation side can be done rather easily using the existing
match_ep() UDC hook.

However we have no good place to "free" them.

This implements a "simple" variant of this, which calls an EP dispose
callback on all EPs associated with a gadget when the composite device
gets unbound.

This is required by my upcoming Aspeed vHub driver.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/usb/gadget/composite.c | 8 ++++++++
 include/linux/usb/gadget.h     | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 77c7ecca816a..ec99c9cfc1a2 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2172,6 +2172,7 @@ int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
 void composite_dev_cleanup(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget_string_container *uc, *tmp;
+	struct usb_ep			   *ep, *tmp_ep;
 
 	list_for_each_entry_safe(uc, tmp, &cdev->gstrings, list) {
 		list_del(&uc->list);
@@ -2193,6 +2194,13 @@ void composite_dev_cleanup(struct usb_composite_dev *cdev)
 	}
 	cdev->next_string_id = 0;
 	device_remove_file(&cdev->gadget->dev, &dev_attr_suspended);
+
+	/* Dispose EPs if the UDC driver tracks lifetime */
+	list_for_each_entry_safe(ep, tmp_ep,
+				 &cdev->gadget->ep_list, ep_list) {
+		if (ep->ops->dispose)
+			ep->ops->dispose(ep);
+	}
 }
 
 static int composite_bind(struct usb_gadget *gadget,
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 66a5cff7ee14..e3424234b23a 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -129,6 +129,7 @@ struct usb_ep_ops {
 	int (*enable) (struct usb_ep *ep,
 		const struct usb_endpoint_descriptor *desc);
 	int (*disable) (struct usb_ep *ep);
+	void (*dispose) (struct usb_ep *ep);
 
 	struct usb_request *(*alloc_request) (struct usb_ep *ep,
 		gfp_t gfp_flags);

[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Felipe Balbi]

Hi,

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> Some UDC may want to allocate endpoints dynamically, either because
> the HW supports an arbitrary large number or because (like the Aspeed
> BMC SoCs), the pool of HW endpoints is shared between multiple gadgets.
>
> The allocation side can be done rather easily using the existing
> match_ep() UDC hook.
>
> However we have no good place to "free" them.
>
> This implements a "simple" variant of this, which calls an EP dispose
> callback on all EPs associated with a gadget when the composite device
> gets unbound.
>
> This is required by my upcoming Aspeed vHub driver.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/usb/gadget/composite.c | 8 ++++++++
>  include/linux/usb/gadget.h     | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 77c7ecca816a..ec99c9cfc1a2 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2172,6 +2172,7 @@ int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
>  void composite_dev_cleanup(struct usb_composite_dev *cdev)
>  {
>  	struct usb_gadget_string_container *uc, *tmp;
> +	struct usb_ep			   *ep, *tmp_ep;
>  
>  	list_for_each_entry_safe(uc, tmp, &cdev->gstrings, list) {
>  		list_del(&uc->list);
> @@ -2193,6 +2194,13 @@ void composite_dev_cleanup(struct usb_composite_dev *cdev)
>  	}
>  	cdev->next_string_id = 0;
>  	device_remove_file(&cdev->gadget->dev, &dev_attr_suspended);
> +
> +	/* Dispose EPs if the UDC driver tracks lifetime */
> +	list_for_each_entry_safe(ep, tmp_ep,
> +				 &cdev->gadget->ep_list, ep_list) {

do you really need this to be safe? You don't seem to be modifying
ep_list here.

[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Benjamin Herrenschmidt]

On Fri, 2018-03-16 at 13:02 +0200, Felipe Balbi wrote:
> Hi,
> 
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> > Some UDC may want to allocate endpoints dynamically, either because
> > the HW supports an arbitrary large number or because (like the Aspeed
> > BMC SoCs), the pool of HW endpoints is shared between multiple gadgets.
> > 
> > The allocation side can be done rather easily using the existing
> > match_ep() UDC hook.
> > 
> > However we have no good place to "free" them.
> > 
> > This implements a "simple" variant of this, which calls an EP dispose
> > callback on all EPs associated with a gadget when the composite device
> > gets unbound.
> > 
> > This is required by my upcoming Aspeed vHub driver.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  drivers/usb/gadget/composite.c | 8 ++++++++
> >  include/linux/usb/gadget.h     | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > index 77c7ecca816a..ec99c9cfc1a2 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -2172,6 +2172,7 @@ int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
> >  void composite_dev_cleanup(struct usb_composite_dev *cdev)
> >  {
> >  	struct usb_gadget_string_container *uc, *tmp;
> > +	struct usb_ep			   *ep, *tmp_ep;
> >  
> >  	list_for_each_entry_safe(uc, tmp, &cdev->gstrings, list) {
> >  		list_del(&uc->list);
> > @@ -2193,6 +2194,13 @@ void composite_dev_cleanup(struct usb_composite_dev *cdev)
> >  	}
> >  	cdev->next_string_id = 0;
> >  	device_remove_file(&cdev->gadget->dev, &dev_attr_suspended);
> > +
> > +	/* Dispose EPs if the UDC driver tracks lifetime */
> > +	list_for_each_entry_safe(ep, tmp_ep,
> > +				 &cdev->gadget->ep_list, ep_list) {
> 
> do you really need this to be safe? You don't seem to be modifying
> ep_list here.

Yes, ep->dispose() may do just that. In my Aspeed implementation in
fact that's pretty much the first thing it does.

IE, I'm returning all the endpoints to the common pool, thus taking
them out of the gadget EP list.

That said, there could be other reasons why a driver might want to know
about disposal without actually taking all the EPs back (for example a
driver could have some dedicated EPs and some in a pool) so I prefer
the list_del remaining in the back-end.

Cheers,
Ben.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Felipe Balbi]

Hi,

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Fri, 2018-03-16 at 13:02 +0200, Felipe Balbi wrote:
>> Hi,
>> 
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> > Some UDC may want to allocate endpoints dynamically, either because
>> > the HW supports an arbitrary large number or because (like the Aspeed
>> > BMC SoCs), the pool of HW endpoints is shared between multiple gadgets.
>> > 
>> > The allocation side can be done rather easily using the existing
>> > match_ep() UDC hook.
>> > 
>> > However we have no good place to "free" them.
>> > 
>> > This implements a "simple" variant of this, which calls an EP dispose
>> > callback on all EPs associated with a gadget when the composite device
>> > gets unbound.
>> > 
>> > This is required by my upcoming Aspeed vHub driver.
>> > 
>> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> > ---
>> >  drivers/usb/gadget/composite.c | 8 ++++++++
>> >  include/linux/usb/gadget.h     | 1 +
>> >  2 files changed, 9 insertions(+)
>> > 
>> > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> > index 77c7ecca816a..ec99c9cfc1a2 100644
>> > --- a/drivers/usb/gadget/composite.c
>> > +++ b/drivers/usb/gadget/composite.c
>> > @@ -2172,6 +2172,7 @@ int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
>> >  void composite_dev_cleanup(struct usb_composite_dev *cdev)
>> >  {
>> >  	struct usb_gadget_string_container *uc, *tmp;
>> > +	struct usb_ep			   *ep, *tmp_ep;
>> >  
>> >  	list_for_each_entry_safe(uc, tmp, &cdev->gstrings, list) {
>> >  		list_del(&uc->list);
>> > @@ -2193,6 +2194,13 @@ void composite_dev_cleanup(struct usb_composite_dev *cdev)
>> >  	}
>> >  	cdev->next_string_id = 0;
>> >  	device_remove_file(&cdev->gadget->dev, &dev_attr_suspended);
>> > +
>> > +	/* Dispose EPs if the UDC driver tracks lifetime */
>> > +	list_for_each_entry_safe(ep, tmp_ep,
>> > +				 &cdev->gadget->ep_list, ep_list) {
>> 
>> do you really need this to be safe? You don't seem to be modifying
>> ep_list here.
>
> Yes, ep->dispose() may do just that. In my Aspeed implementation in
> fact that's pretty much the first thing it does.
>
> IE, I'm returning all the endpoints to the common pool, thus taking
> them out of the gadget EP list.
>
> That said, there could be other reasons why a driver might want to know
> about disposal without actually taking all the EPs back (for example a
> driver could have some dedicated EPs and some in a pool) so I prefer
> the list_del remaining in the back-end.

That seems rather obfuscated. There's no indication that ep_list is
modified from within that iteration block. Seems like it would be best
to make the list_del() explicit and ->dispose() just adds the ep to its
own internal list. No?

[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Benjamin Herrenschmidt]

On Mon, 2018-03-19 at 12:56 +0200, Felipe Balbi wrote:
> >> do you really need this to be safe? You don't seem to be modifying
> >> ep_list here.
> >
> > Yes, ep->dispose() may do just that. In my Aspeed implementation in
> > fact that's pretty much the first thing it does.
> >
> > IE, I'm returning all the endpoints to the common pool, thus taking
> > them out of the gadget EP list.
> >
> > That said, there could be other reasons why a driver might want to know
> > about disposal without actually taking all the EPs back (for example a
> > driver could have some dedicated EPs and some in a pool) so I prefer
> > the list_del remaining in the back-end.
> 
> That seems rather obfuscated. There's no indication that ep_list is
> modified from within that iteration block. Seems like it would be best
> to make the list_del() explicit and ->dispose() just adds the ep to its
> own internal list. No?

The problem with this approach is that other existing UDC drivers who
don't do dynamic EP management might break since they assume the EPs
remain in the EP list for ever.

So we would have to make the list_del conditional to the presence of
the ->dispose callback, or add a flag or something like that, which I
don't find particularily elegant either.

Also it's the backend that adds to the EP list, it should be the
backend that removes from it to. I don't like when you end up in a
situation where a different "layer" does half of the work. It gets more
confusing and bug prone. The ep_list management is under ownership of
the UDC and thus should probably remain that way imho.

I think it makes sense from a high level perspective to say that the
UDC backend can optionally support disposing of EPs. I think all that's
needed here is maybe adding a comment to my patch, something along
those lines:

	/*
	 * Some UDC backends have a dynamic EP allocation scheme.
	 *
	 * In that case, the dispose() callback is used to notify the
	 * backend that the EPs are no longer in use.
	 *
	 * Note: The UDC backend can remove the EP from the ep_list as
	 *       a result, so we need to use the _safe list iterator.
	 */
	list_for_each_entry_safe(ep, tmp_ep,
                           &cdev->gadget->ep_list, ep_list) {
		...

Would that work for you ?

Cheers,
Ben.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[v5,1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Felipe Balbi]

Hi,

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Mon, 2018-03-19 at 12:56 +0200, Felipe Balbi wrote:
>> >> do you really need this to be safe? You don't seem to be modifying
>> >> ep_list here.
>> >
>> > Yes, ep->dispose() may do just that. In my Aspeed implementation in
>> > fact that's pretty much the first thing it does.
>> >
>> > IE, I'm returning all the endpoints to the common pool, thus taking
>> > them out of the gadget EP list.
>> >
>> > That said, there could be other reasons why a driver might want to know
>> > about disposal without actually taking all the EPs back (for example a
>> > driver could have some dedicated EPs and some in a pool) so I prefer
>> > the list_del remaining in the back-end.
>> 
>> That seems rather obfuscated. There's no indication that ep_list is
>> modified from within that iteration block. Seems like it would be best
>> to make the list_del() explicit and ->dispose() just adds the ep to its
>> own internal list. No?
>
> The problem with this approach is that other existing UDC drivers who
> don't do dynamic EP management might break since they assume the EPs
> remain in the EP list for ever.
>
> So we would have to make the list_del conditional to the presence of
> the ->dispose callback, or add a flag or something like that, which I
> don't find particularily elegant either.

I see.

> Also it's the backend that adds to the EP list, it should be the
> backend that removes from it to. I don't like when you end up in a
> situation where a different "layer" does half of the work. It gets more
> confusing and bug prone. The ep_list management is under ownership of
> the UDC and thus should probably remain that way imho.
>
> I think it makes sense from a high level perspective to say that the
> UDC backend can optionally support disposing of EPs. I think all that's
> needed here is maybe adding a comment to my patch, something along
> those lines:
>
> 	/*
> 	 * Some UDC backends have a dynamic EP allocation scheme.
> 	 *
> 	 * In that case, the dispose() callback is used to notify the
> 	 * backend that the EPs are no longer in use.
> 	 *
> 	 * Note: The UDC backend can remove the EP from the ep_list as
> 	 *       a result, so we need to use the _safe list iterator.
> 	 */
> 	list_for_each_entry_safe(ep, tmp_ep,
>                            &cdev->gadget->ep_list, ep_list) {
> 		...
>
> Would that work for you ?

sure