| Message ID | 20230628092404.13927-1-srinivas.kandagatla@linaro.org |
|---|---|
| State | Accepted |
| Commit | ac192c1a54f9562efe6bac910e6e7aae7b5fbea3 |
| Headers | show |
| Series | ASoC: qdsp6: q6apm: use dai link pcm id as pcm device number | expand |
On Wed, Jun 28, 2023 at 10:24:04AM +0100, Srinivas Kandagatla wrote: > For some reason we ended up with a setup without this flag. > This resulted in inconsistent sound card devices numbers which > are also not starting as expected at dai_link->id. > (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) Why is this a problem? > With this patch patch now the MultiMedia1 PCM ends up with device number 0 > as expected. > > Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") > Cc: <Stable@vger.kernel.org> Won't this be an ABI change? That seems like it'd disrupt things in stable.
On Thu, Jun 29, 2023 at 04:43:57PM +0100, Mark Brown wrote: > On Wed, Jun 28, 2023 at 10:24:04AM +0100, Srinivas Kandagatla wrote: > > For some reason we ended up with a setup without this flag. > > This resulted in inconsistent sound card devices numbers which > > are also not starting as expected at dai_link->id. > > (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) > > Why is this a problem? > > > With this patch patch now the MultiMedia1 PCM ends up with device number 0 > > as expected. > > > > Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") > > Cc: <Stable@vger.kernel.org> > > Won't this be an ABI change? That seems like it'd disrupt things in > stable. ABI changes should disrupt things just the same in Linus's tree, why is stable any different? thanks, greg k-h
On Thu, Jun 29, 2023 at 06:06:05PM +0200, Greg KH wrote: > On Thu, Jun 29, 2023 at 04:43:57PM +0100, Mark Brown wrote: > > Won't this be an ABI change? That seems like it'd disrupt things in > > stable. > ABI changes should disrupt things just the same in Linus's tree, why is > stable any different? This is a numbering resulting from enumeration thing so it gets to be like the issues we've had with the order in which block and ethernet devices appear, it's on the edge the extent to which people might be relying on it. If it's causing some problem as is and there's a reason to do something (see the first half of my reply...) but the case gets even harder to make with stable.
On Thu, Jun 29, 2023 at 05:16:44PM +0100, Mark Brown wrote: > On Thu, Jun 29, 2023 at 06:06:05PM +0200, Greg KH wrote: > > On Thu, Jun 29, 2023 at 04:43:57PM +0100, Mark Brown wrote: > > > > Won't this be an ABI change? That seems like it'd disrupt things in > > > stable. > > > ABI changes should disrupt things just the same in Linus's tree, why is > > stable any different? > > This is a numbering resulting from enumeration thing so it gets to be > like the issues we've had with the order in which block and ethernet > devices appear, it's on the edge the extent to which people might be > relying on it. If it's causing some problem as is and there's a reason > to do something (see the first half of my reply...) but the case gets > even harder to make with stable. It shouldn't matter for stable or not, if the change is acceptable in Linus's tree, with the userspace visable change, then it should be acceptable in any active stable branch as well. There is no difference here for userspace api/abi rules. thanks, greg k-h
On 29/06/2023 16:43, Mark Brown wrote: > On Wed, Jun 28, 2023 at 10:24:04AM +0100, Srinivas Kandagatla wrote: >> For some reason we ended up with a setup without this flag. >> This resulted in inconsistent sound card devices numbers which >> are also not starting as expected at dai_link->id. >> (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) > > Why is this a problem? In existing Qualcomm setup the backend pcm are added first, which results in frontend pcms getting pcm numbers after this. For example: with 3 backend dailinks in DT we have frontend pcm start at 3. Now if we add new backend dai-link in DT we now have frontend pcm start at 4. This is a bug in qualcomm driver. > >> With this patch patch now the MultiMedia1 PCM ends up with device number 0 >> as expected. >> >> Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") >> Cc: <Stable@vger.kernel.org> > > Won't this be an ABI change? That seems like it'd disrupt things in > stable. Yes, but this is a real bug. without fixing this also results in abi(pcm number) change when we add new backend dai-link. I have also sent fix for UCM to handle this. --srini
On Thu, Jun 29, 2023 at 07:22:51PM +0200, Greg KH wrote: > It shouldn't matter for stable or not, if the change is acceptable in > Linus's tree, with the userspace visable change, then it should be > acceptable in any active stable branch as well. There is no difference > here for userspace api/abi rules. As discussed before your tolerance for risk in stable is *far* higher than mine, if there's any value in doing this at all it's probably within what would get taken but that doesn't mean that it's something that it's sensible to highlight as an important fix like tagging for stable does. It's extremely unclear that it fits the severity criteria that are supposed to be being applied to stable, though obviously the documentation doesn't fit the actual practice these days.
On Thu, Jun 29, 2023 at 06:33:09PM +0100, Srinivas Kandagatla wrote: > On 29/06/2023 16:43, Mark Brown wrote: > > On Wed, Jun 28, 2023 at 10:24:04AM +0100, Srinivas Kandagatla wrote: > > > For some reason we ended up with a setup without this flag. > > > This resulted in inconsistent sound card devices numbers which > > > are also not starting as expected at dai_link->id. > > > (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) > > Why is this a problem? > In existing Qualcomm setup the backend pcm are added first, which results in > frontend pcms getting pcm numbers after this. > For example: with 3 backend dailinks in DT we have frontend pcm start at 3. > Now if we add new backend dai-link in DT we now have frontend pcm start at > 4. > This is a bug in qualcomm driver. Why is this an actual problem rather than just being a bit ugly? What is the negative consequence of having a PCM with this number? > > > With this patch patch now the MultiMedia1 PCM ends up with device number 0 > > > as expected. > > > > > > Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") > > > Cc: <Stable@vger.kernel.org> > > > > Won't this be an ABI change? That seems like it'd disrupt things in > > stable. > Yes, but this is a real bug. without fixing this also results in abi(pcm > number) change when we add new backend dai-link. I have also sent fix for > UCM to handle this. I'm still not clear why you believe this to be a bug.
On Thu, Jun 29, 2023 at 06:38:38PM +0100, Mark Brown wrote: > On Thu, Jun 29, 2023 at 07:22:51PM +0200, Greg KH wrote: > > > It shouldn't matter for stable or not, if the change is acceptable in > > Linus's tree, with the userspace visable change, then it should be > > acceptable in any active stable branch as well. There is no difference > > here for userspace api/abi rules. > > As discussed before your tolerance for risk in stable is *far* higher > than mine, if there's any value in doing this at all it's probably > within what would get taken but that doesn't mean that it's something > that it's sensible to highlight as an important fix like tagging for > stable does. It's extremely unclear that it fits the severity criteria > that are supposed to be being applied to stable, though obviously the > documentation doesn't fit the actual practice these days. It's not a matter of "tolerance for risk", it's a "if this change is good enough for future releases, why isn't it good enough for older releases as well?" As you know, we don't break user interfaces, so either this is a break or it isn't, stable trees have nothing to do with it as a normal user would "hit" this when updating to run Linus's tree, just as easily as they would "hit" it updating their stable kernel version. thanks, greg k-h
On Thu, Jun 29, 2023 at 08:48:42PM +0200, Greg KH wrote: > On Thu, Jun 29, 2023 at 06:38:38PM +0100, Mark Brown wrote: > > As discussed before your tolerance for risk in stable is *far* higher > > than mine, if there's any value in doing this at all it's probably > > within what would get taken but that doesn't mean that it's something > > that it's sensible to highlight as an important fix like tagging for > > stable does. It's extremely unclear that it fits the severity criteria > > that are supposed to be being applied to stable, though obviously the > > documentation doesn't fit the actual practice these days. > It's not a matter of "tolerance for risk", it's a "if this change is > good enough for future releases, why isn't it good enough for older > releases as well?" > As you know, we don't break user interfaces, so either this is a break > or it isn't, stable trees have nothing to do with it as a normal user > would "hit" this when updating to run Linus's tree, just as easily as > they would "hit" it updating their stable kernel version. You know as well as I do that we have a bunch of interfaces where things end up getting dynamically numbered as they appear, and provided to userspace together with identifying information that allows userspace to figure out what's what in a stable fashion even though the numbers might change. Like I said earlier in the thread this is one of them, better hardware support also has some risk of disturbing things (and some of the numbering is going to be hotplug dependent, though this patch isn't likely to run into that particular bit of things). ABI stability is a continuum, from for example things relying on race conditions or other timing things that were lucky they ever worked to changes in interfaces that break clear and documented guarantees. Reliance on stability is similar, and how much of an issue it is when something does change and someone notices is going to vary depending on what changed and why. While the risk here seems low if the reasoning is just to make things neater then it's even harder to justify for a stable kernel than it is for mainline. Note also that the patch is still under discussion for mainline...
On 29/06/2023 18:42, Mark Brown wrote: > On Thu, Jun 29, 2023 at 06:33:09PM +0100, Srinivas Kandagatla wrote: >> On 29/06/2023 16:43, Mark Brown wrote: >>> On Wed, Jun 28, 2023 at 10:24:04AM +0100, Srinivas Kandagatla wrote: > >>>> For some reason we ended up with a setup without this flag. >>>> This resulted in inconsistent sound card devices numbers which >>>> are also not starting as expected at dai_link->id. >>>> (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) > >>> Why is this a problem? > >> In existing Qualcomm setup the backend pcm are added first, which results in >> frontend pcms getting pcm numbers after this. > >> For example: with 3 backend dailinks in DT we have frontend pcm start at 3. >> Now if we add new backend dai-link in DT we now have frontend pcm start at >> 4. > >> This is a bug in qualcomm driver. > > Why is this an actual problem rather than just being a bit ugly? What > is the negative consequence of having a PCM with this number? Yes, it is ugly but also breaks the existing UCM as the pcm device numbers keep changing. Which is why I refereed it as bug in the driver. --srini
On Wed, 28 Jun 2023 10:24:04 +0100, Srinivas Kandagatla wrote: > For some reason we ended up with a setup without this flag. > This resulted in inconsistent sound card devices numbers which > are also not starting as expected at dai_link->id. > (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) > > With this patch patch now the MultiMedia1 PCM ends up with device number 0 > as expected. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: qdsp6: q6apm: use dai link pcm id as pcm device number commit: ac192c1a54f9562efe6bac910e6e7aae7b5fbea3 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c index 5eb0b864c740..c90db6daabbd 100644 --- a/sound/soc/qcom/qdsp6/q6apm-dai.c +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c @@ -840,6 +840,7 @@ static const struct snd_soc_component_driver q6apm_fe_dai_component = { .pointer = q6apm_dai_pointer, .trigger = q6apm_dai_trigger, .compress_ops = &q6apm_dai_compress_ops, + .use_dai_pcm_id = true, }; static int q6apm_dai_probe(struct platform_device *pdev)
For some reason we ended up with a setup without this flag. This resulted in inconsistent sound card devices numbers which are also not starting as expected at dai_link->id. (Ex: MultiMedia1 pcm ended up with device number 4 instead of 0) With this patch patch now the MultiMedia1 PCM ends up with device number 0 as expected. Fixes: 9b4fe0f1cd79 ("ASoC: qdsp6: audioreach: add q6apm-dai support") Cc: <Stable@vger.kernel.org> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- sound/soc/qcom/qdsp6/q6apm-dai.c | 1 + 1 file changed, 1 insertion(+)