Hi, Benjamin Herrenschmidt <b...@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
--
balbi
signature.asc
Description: PGP signature
