Hi,
Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> writes:
> On Fri, 2018-03-16 at 13:02 +0200, Felipe Balbi wrote:
>> Hi,
>>
>> Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>> > ---
>> > 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?
--
balbi
Attachment:
signature.asc
Description: PGP signature
