Re: [PATCH v2 09/10] ASoC: topology: Change stream formats to bitwise flag

[Takashi Iwai <tiwai@suse.de>]

On Fri, 14 Aug 2015 22:34:28 +0200,
Mark Brown wrote:
> 
> On Mon, Aug 10, 2015 at 10:48:37PM +0800, mengdong.lin@intel.com wrote:
> 
> >  struct snd_soc_tplg_stream_caps {
> >  	__le32 size;		/* in bytes of this structure */
> >  	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> > -	__le64 formats[SND_SOC_TPLG_MAX_FORMATS];	/* supported formats SNDRV_PCM_FMTBIT_* */
> > +	__le64 formats;	/* supported formats SNDRV_PCM_FORMAT_* */
> 
> Argh, this is *another* ABI change which you've buried in the middle of
> a series you're sending right at the end of the release cycle (this was
> sent after -rc6, we may not even get a -rc7...).  Given how close we are
> to the release and the invasiveness of the changes in this series it's
> very lucky I even saw it before release, I'm reading this on my flight
> to Plumbers which means my bandwidth next week will be limited.

Thanks for checking this.  Mengdong, if there is a change in uapi/*,
this means touching ABI.  So, this change isn't acceptable once when
the kernel is released.  At least, the backward compatibility *must*
be kept no matter which version is.

Also, the patch (and even the current code) looks wrong.  The current
formats[] would receive SNDRV_PCM_FORMAT_* while the comment shows
SNDRV_PCM_FMT_BIT_*.  And your patch changes it again wrongly in the
comment.  Quite confusing.

> This should have at the very least been sent at the start of the series,
> and really should have been sent separately.  It is also worrying that
> there is nothing in the subject or changelog talking about the fact that
> this is an ABI change or explaining why it is required.  The changelog
> was simply a statement of the content of the diff, not a rationale:
> 
> | The toplogy user space tool will generate this bitwise flag by using
> | SNDRV_PCM_FORMAT_* exposed by asound.h, and the topology core will copy
> | this flag when generating DAI streams.
> 
> Once the release goes out this sort of incompatible change will be
> unacceptable.  Given the number of changes that are going into the ABI
> (we also had some others went in last week) I'm now giving some serious
> thought to the idea that we should ask Linus to revert the topology code
> for v4.2 and waiting for v4.3 so we've got more chance to make sure the
> ABI is one we actually want to stick with.  Liam, Takashi I don't know
> what you think?

We can make the topology stuff disabled in Makefile (and the only call
in soc-core.c) instead of the whole revert.  This will be the minimal
impact and safe enough.

BTW, I wonder why there is no Kconfig for topology stuff.  If we had
it, it would have been easy to disable.


thanks,

Takashi