Hi,
Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
