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

[Felipe Balbi <balbi@kernel.org>]

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?