| Message ID | b9a7ab97e4b4f96cced9faa2e2bce2b533d25a10.1439217448.git.mengdong.lin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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?
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
> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Saturday, August 15, 2015 3:38 PM > 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. Thanks for your review, Mark and Takashi. This series including the ABI change, is not for v4.2, but is an RFC to adding DAI/DAI links support in topology. I'm sorry that I should have explicitly said this in my comments. Is there some topic branch which can accept topology patches for next kernel release? Can we change topology ABI in v4.3? Now only Intel SKL driver is using topology code, so the affect should be limited. And I feel using bitwise flag is simpler in both user space and in kernel coding. We can increase the topology ABI number when there is ABI change, and the topology driver always checks this version. > 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. Sorry. I will change the comments. I hope to change "formats" to a bitwise flag of _SNDRV_PCM_FMTBIT_*. Current upstreamed topology driver does not really handle DAIs (streams caps) so the error is not discovered. > > 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. We'll add Kconfig for topology. Thanks Mengdong
On Sat, 15 Aug 2015 15:49:42 +0200, Lin, Mengdong wrote: > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Saturday, August 15, 2015 3:38 PM > > > 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. > > Thanks for your review, Mark and Takashi. > This series including the ABI change, is not for v4.2, but is an RFC to adding DAI/DAI links support in topology. > I'm sorry that I should have explicitly said this in my comments. > > Is there some topic branch which can accept topology patches for next kernel release? > Can we change topology ABI in v4.3? In general "yes, but only if it's backward-compatible". > Now only Intel SKL driver is using topology code, so the affect should be limited. > And I feel using bitwise flag is simpler in both user space and in kernel coding. Yes, using FMTBIT_* is better, but this must not justify to break ABI. Either we disable this API/ABI for 4.2, or make the change backward compatible. > We can increase the topology ABI number when there is ABI change, and the topology driver always checks this version. It's a big NO, sorry, this isn't acceptable. Again, the kernel must give the backward compatibility. That is, the kernel must accept the old dialect the user space talks if user space wants. You cannot force user space to communicate in a new ABI out of sudden. So, yes, ABI version bump is needed if you change ABI. But it means that the kernel accepts all ABIs up to this version, not meaning *only* this ABI version. Hopefully this clarifies the situation. thanks, Takashi
On Sat, Aug 15, 2015 at 09:37:34AM +0200, Takashi Iwai wrote: > 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. Yes, though it does leave the UAPI files there for people to look at which is a big part of the risk since they end up getting shipped separately to the kernel binary. Might be sufficient, though - we could just put some #errors in the headers to catch potential users. Or perhaps we're OK with the ABI as it stands. > BTW, I wonder why there is no Kconfig for topology stuff. If we had > it, it would have been easy to disable. It's probably worth adding that anyway - I know Intel at least are keen on having the capability to minimise memory usage!
On Sat, Aug 15, 2015 at 01:49:42PM +0000, Lin, Mengdong wrote: > Is there some topic branch which can accept topology patches for next > kernel release? Can we change topology ABI in v4.3? You're not quite getting the issue here - the problem is that once we have released the ABI we need to keep supporting applications using it so we can't just change the ABI, we also need to keep compatibility code for the original ABI. This makes it a lot more effort to change things. > Now only Intel SKL driver is using topology code, so the affect should > be limited. And I feel using bitwise flag is simpler in both user > space and in kernel coding. We can increase the topology ABI number > when there is ABI change, and the topology driver always checks this > version. Someone else may take this kernel and use it in their product, or some distro may decide to ship v4.2 as their kernel. Once it's in a release we have to assume someone is going to start relying on the ABI.
> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Saturday, August 15, 2015 10:46 PM > On Sat, 15 Aug 2015 15:49:42 +0200, > Lin, Mengdong wrote: > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: Saturday, August 15, 2015 3:38 PM > > > > > 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. > > > > Thanks for your review, Mark and Takashi. > > This series including the ABI change, is not for v4.2, but is an RFC to adding > DAI/DAI links support in topology. > > I'm sorry that I should have explicitly said this in my comments. > > > > Is there some topic branch which can accept topology patches for next > kernel release? > > Can we change topology ABI in v4.3? > > In general "yes, but only if it's backward-compatible". > > > Now only Intel SKL driver is using topology code, so the affect should be > limited. > > And I feel using bitwise flag is simpler in both user space and in kernel > coding. > > Yes, using FMTBIT_* is better, but this must not justify to break ABI. Either > we disable this API/ABI for 4.2, or make the change backward compatible. Many thanks for your clarification! We'd better keep current ABI unchanged to keep backward compatibility. In addition, does "disable this API/ABI for 4.2" mean disabling all topology features, or only some structures for v4.2? But alsa-lib already integrated this ABI version. I'm worried that If we have to add new fields to ABI structures, how to keep the backward compatibility. Thanks Mengdong > > > We can increase the topology ABI number when there is ABI change, and > the topology driver always checks this version. > > It's a big NO, sorry, this isn't acceptable. Again, the kernel must give the > backward compatibility. That is, the kernel must accept the old dialect the > user space talks if user space wants. You cannot force user space to > communicate in a new ABI out of sudden. > > So, yes, ABI version bump is needed if you change ABI. But it means that the > kernel accepts all ABIs up to this version, not meaning > *only* this ABI version. > > Hopefully this clarifies the situation. > > > thanks, > > Takashi
On Sat, 15 Aug 2015 17:25:18 +0200, Lin, Mengdong wrote: > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Saturday, August 15, 2015 10:46 PM > > > On Sat, 15 Aug 2015 15:49:42 +0200, > > Lin, Mengdong wrote: > > > > > > > -----Original Message----- > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > Sent: Saturday, August 15, 2015 3:38 PM > > > > > > > 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. > > > > > > Thanks for your review, Mark and Takashi. > > > This series including the ABI change, is not for v4.2, but is an RFC to adding > > DAI/DAI links support in topology. > > > I'm sorry that I should have explicitly said this in my comments. > > > > > > Is there some topic branch which can accept topology patches for next > > kernel release? > > > Can we change topology ABI in v4.3? > > > > In general "yes, but only if it's backward-compatible". > > > > > Now only Intel SKL driver is using topology code, so the affect should be > > limited. > > > And I feel using bitwise flag is simpler in both user space and in kernel > > coding. > > > > Yes, using FMTBIT_* is better, but this must not justify to break ABI. Either > > we disable this API/ABI for 4.2, or make the change backward compatible. > > Many thanks for your clarification! > We'd better keep current ABI unchanged to keep backward compatibility. > > In addition, does "disable this API/ABI for 4.2" mean disabling all topology features, or only some structures for v4.2? I suppose the whole topology feature. > But alsa-lib already integrated this ABI version. Yes, but it's no big problem, as we haven't released alsa-lib changes yet. > I'm worried that If we have to add new fields to ABI structures, how to keep the backward compatibility. There are reserved areas for the future extension, and usually take a part of them for the additional fields. Even if doing that, it's still very sensitive to change the structure in general, though. Takashi
On Sat, Aug 15, 2015 at 03:25:18PM +0000, Lin, Mengdong wrote: > In addition, does "disable this API/ABI for 4.2" mean disabling all > topology features, or only some structures for v4.2? But alsa-lib > already integrated this ABI version. I'm suggesting not including this at all in the v4.2 release so we can continue to make changes, or perhaps just leaving the code there but making sure it can't be used as Takashi suggested. alsa-lib hasn't released yet as far as I'm aware so we can continue to make changes there. > I'm worried that If we have to add new fields to ABI structures, how > to keep the backward compatibility. Right, it's tricky - there's a bunch of techniques like the versioning that's in the current structures but they all take work and effort.
On Sat, 15 Aug 2015 16:59:04 +0200, Mark Brown wrote: > > On Sat, Aug 15, 2015 at 09:37:34AM +0200, Takashi Iwai wrote: > > > 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. > > Yes, though it does leave the UAPI files there for people to look at > which is a big part of the risk since they end up getting shipped > separately to the kernel binary. True, if we want really 120% safety... > Might be sufficient, though - we could > just put some #errors in the headers to catch potential users. Or > perhaps we're OK with the ABI as it stands. Yeah, #error or a big fat comment should suffice, I suppose. > > BTW, I wonder why there is no Kconfig for topology stuff. If we had > > it, it would have been easy to disable. > > It's probably worth adding that anyway - I know Intel at least are keen > on having the capability to minimise memory usage! Right, I had it in my mind, too. Takashi
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Sunday, August 16, 2015 12:52 AM > On Sat, Aug 15, 2015 at 03:25:18PM +0000, Lin, Mengdong wrote: > > > In addition, does "disable this API/ABI for 4.2" mean disabling all > > topology features, or only some structures for v4.2? But alsa-lib > > already integrated this ABI version. > > I'm suggesting not including this at all in the v4.2 release so we can continue > to make changes, or perhaps just leaving the code there but making sure it > can't be used as Takashi suggested. alsa-lib hasn't released yet as far as I'm > aware so we can continue to make changes there. Hi Takashi/Mark, After syncing with Liam, we hope to disable topology in the v4.2 release atm but leave the code there. There may be topology ABI changes later and keeping backward compatibility would be difficult. Sorry for the inconvenience. Thanks Mengdong > > > I'm worried that If we have to add new fields to ABI structures, how > > to keep the backward compatibility. > > Right, it's tricky - there's a bunch of techniques like the versioning that's in the > current structures but they all take work and effort.
diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h index 69383b0..eb2ad59 100644 --- a/include/uapi/sound/asoc.h +++ b/include/uapi/sound/asoc.h @@ -191,7 +191,7 @@ struct snd_soc_tplg_ctl_hdr { 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_* */ __le32 rates; /* supported rates SNDRV_PCM_RATE_* */ __le32 rate_min; /* min rate */ __le32 rate_max; /* max rate */