usb/gadget: Add an EP dispose() callback for EP lifetime tracking

Quoted 14 lines · Felipe Balbi

Quoted 6 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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 ?