[PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"

[Kai Vehmanen]

Hi,

On Mon, 2 Mar 2020, Kuninori Morimoto wrote:

> I guess you are thinking more big scale tracking/solution (?).
> Indeed it is needed, but my indicated one is not for it.
> It is just for "we want to use soc_pcm_close() as soc_pcm_open() error handling".

yes, I'm thinking it would be better to do proper tracking and not do an 
intermediate solution with IDs.

> > > 	int soc_pcm_open(...)
> > > 	{
> > > 		static u8 id;
> > > 
> > > 		/* update ID */
> > > 		id++;
> > > 		if (id == 0)
> > > 			id++;
> 
> Maybe the naming of "ID" makes you confused ?
> It is just "mark" for this "soc_pcm_open()".
> If error happen during open, "error handling soc_pcm_close()" cares only this mark.
> It is just for avoiding mismatch close.

Yes, the main issues I see:
  - the name (maybe "open_tag" would be better than "open_id"),
  - declaration of the id with "static u8 id" -- if multiple unrelated 
    streams are opened concurrently, the id management needs to be handled
    in a thread safe way,
  - the "component->open_id" field only refers to the last substream 
    open -- i.e. field is only relevant in contact of error rollbacks 

The "new id" really just refers to the substream being opened, so 
you could create it from the substream pointer as well. For error 
rollback, you want to close all components of the substream being
opened. But this is still a bit unelegant as you'd end up storing
the last substream opened to component struct (3rd bullet above).

I was thinking more in the lines of:

/* add list of opened substreams to snd_soc_component */
struct snd_soc_component {
...
	struct list_head substream_list;

int snd_soc_component_open(struct snd_soc_component *component,
»       »       »          struct snd_pcm_substream *substream)
{
	int res = 0;
	if (component->driver->open)
		res = component->driver->open(component, substream);

        /* on success, add proxy of substream to component->substream_list  */
...

int snd_soc_component_close(struct snd_soc_component *component,
»       »       »           struct snd_pcm_substream *substream)
{
	/* 
	 * lookup substream from "component->substream_List", 
	 * only call driver->close() if found
	 */
...


... this is arguably more code, but makes the state created in 
snd_soc_component_open() explicit. Upon error in middle of 
components_open(), one can just call soc_pcm_components_close() which will 
close all components for that substream (that had been succesfully 
opened).

Br, Kai