[alsa-devel] [PATCH 0/5] RFC for snd-usb: rework usb endpoint logic

[Takashi Iwai]

At Mon, 31 Oct 2011 13:38:20 +0100,
Daniel Mack wrote:
> 
> On 10/31/2011 01:10 PM, Daniel Mack wrote:
> > I didn't sign-off the patches on purpose, as I would really like to
> > get them reviewed before they go in. Can people have a look and state
> > whether the whole idea is at all sane?
> 
> To briefly state the idea as such: the new implementation defines a 
> model (snd_usb_endpoint) that handles everything that is related to an 
> USB endpoint and its streaming. There are functions to activate and 
> deactivate an endpoint (which call usb_set_interface()), and to start 
> and stop its URBs. It also has function pointers to be called when data 
> was received or is about to be sent, and pointer to a sync slave 
> (another snd_usb_endpoint) that is informed when data has been received. 
> A snd_usb_endpoint knows about its state and implements a refcounting, 
> so only the first user will actually start the URBs and only the last 
> one to stop it will tear them down again.
> 
> With this sort of abstraction, the actual streaming is decoupled from 
> the pcm handling, which makes the "implicit feedback" mechanisms easy to 
> implement. All the code that actually handles the payload of a stream's 
> packets is now implemented in pcm.c, which is were it belongs to.
> 
> But I'm sure there are some unresolved corner cases which need attention.

The overall design looks good to me.  Nice work!
A few nitpicking:

- No need for check of activated flags at starting streams?
- Better to clear subs->data_endpoint & co at closing.
- There might be unbalances when deactivate_endpoints() isn't called
  in prepare callback before activate_endpoints().  The apps may call
  like
	open -> hw_params -> prepare -> hw_params -> prepare -> trigger
  and
	trigger (stop) -> prepare -> trigger (start)
  So, be careful about refcounting and active flags.

And, yes, the protection for snd_usb_add_endpoint() would be really
needed for concurrent access.  The card-global mutex would be enough.


thanks,

Takashi