[PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Takashi Sakamoto]

SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow
to a protocol of ctl and tlv operation. At least, this macro and related
kernel APIs include two misunderstandings:
 - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
   packet paylaod, snd_soc_bytes_info_ext() performs in this way.
 - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
   packet structure, snd_soc_bytes_tlv_callback() performs in this way.

In a policy of kernel land development, it's quite worse to break protocols
for applications. Therefore, developers are discouraged to use these kernel
APIs.

In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
developers who need to add control elements which transfer data larger than
the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
bytes. However, as long as the size is less than the size; e.g. 512 bytes,
SND_SOC_BYTES_EXT is still available. Although there is actually the
limitation of maximum data size, it's better to use this API for stable
application interface till better alternative ways are implemented in
future.

For these reasons, this commit reverts the previous commit which lead
developers to the worse behaviour.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/soc.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6144882..18f7d21 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -311,9 +311,6 @@
 		{.base = xbase, .num_regs = xregs,	      \
 		 .mask = xmask }) }
 
-/*
- * SND_SOC_BYTES_EXT is deprecated, please USE SND_SOC_BYTES_TLV instead
- */
 #define SND_SOC_BYTES_EXT(xname, xcount, xhandler_get, xhandler_put) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = snd_soc_bytes_info_ext, \
-- 
2.7.4

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Charles Keepax]

On Thu, Sep 15, 2016 at 10:45:49PM +0900, Takashi Sakamoto wrote:
> SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow
> to a protocol of ctl and tlv operation. At least, this macro and related
> kernel APIs include two misunderstandings:
>  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
>    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
>  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
>    packet structure, snd_soc_bytes_tlv_callback() performs in this way.
> 
> In a policy of kernel land development, it's quite worse to break protocols
> for applications. Therefore, developers are discouraged to use these kernel
> APIs.
> 
> In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
> developers who need to add control elements which transfer data larger than
> the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
> bytes. However, as long as the size is less than the size; e.g. 512 bytes,
> SND_SOC_BYTES_EXT is still available. Although there is actually the
> limitation of maximum data size, it's better to use this API for stable
> application interface till better alternative ways are implemented in
> future.
> 
> For these reasons, this commit reverts the previous commit which lead
> developers to the worse behaviour.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Yeah would be better to use normal controls for <512 bytes, so
this looks good to me.

Although you might want to resend using Mark's kernel.org address
as I imagine he would be the one to apply the change and that is
the address he normally uses for patches.

Thanks,
Charles

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Takashi Sakamoto]

On Sep 15 2016 23:21, Charles Keepax wrote:
> On Thu, Sep 15, 2016 at 10:45:49PM +0900, Takashi Sakamoto wrote:
>> SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow
>> to a protocol of ctl and tlv operation. At least, this macro and related
>> kernel APIs include two misunderstandings:
>>  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
>>    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
>>  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
>>    packet structure, snd_soc_bytes_tlv_callback() performs in this way.
>>
>> In a policy of kernel land development, it's quite worse to break protocols
>> for applications. Therefore, developers are discouraged to use these kernel
>> APIs.
>>
>> In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
>> developers who need to add control elements which transfer data larger than
>> the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
>> bytes. However, as long as the size is less than the size; e.g. 512 bytes,
>> SND_SOC_BYTES_EXT is still available. Although there is actually the
>> limitation of maximum data size, it's better to use this API for stable
>> application interface till better alternative ways are implemented in
>> future.
>>
>> For these reasons, this commit reverts the previous commit which lead
>> developers to the worse behaviour.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
> 
> Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> 
> Yeah would be better to use normal controls for <512 bytes, so
> this looks good to me.
> 
> Although you might want to resend using Mark's kernel.org address
> as I imagine he would be the one to apply the change and that is
> the address he normally uses for patches.

Oh, he uses the domain instead of linaro.org. I missed it.

Now I add him to CC list.


Thanks

Takashi Sakamoto

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 711 bytes --]

On Thu, Sep 15, 2016 at 11:41:43PM +0900, Takashi Sakamoto wrote:
> On Sep 15 2016 23:21, Charles Keepax wrote:

> > Although you might want to resend using Mark's kernel.org address
> > as I imagine he would be the one to apply the change and that is
> > the address he normally uses for patches.

> Oh, he uses the domain instead of linaro.org. I missed it.

> Now I add him to CC list.

As documented in SubmittingPatches please send patches to the 
maintainers for the code you would like to change.  The normal kernel
workflow is that people apply patches from their inboxes, if they aren't
copied they are likely to not see the patch at all and it is much more
difficult to apply patches.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Takashi Sakamoto]

On Sep 15 2016 23:54, Mark Brown wrote:
> On Thu, Sep 15, 2016 at 11:41:43PM +0900, Takashi Sakamoto wrote:
>> On Sep 15 2016 23:21, Charles Keepax wrote:
> 
>>> Although you might want to resend using Mark's kernel.org address
>>> as I imagine he would be the one to apply the change and that is
>>> the address he normally uses for patches.
> 
>> Oh, he uses the domain instead of linaro.org. I missed it.
> 
>> Now I add him to CC list.
> 
> As documented in SubmittingPatches please send patches to the 
> maintainers for the code you would like to change.  The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.

I see. I'll post this to below addresses.

$ ./scripts/get_maintainer.pl
/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch
fatal:
/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch:
'/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch'
is outside repository
Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER /
DYNAMIC AUDIO POWER MANAGEM...)
Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
Takashi Iwai <tiwai@suse.com> (maintainer:SOUND)
alsa-devel@alsa-project.org (moderated list:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
linux-kernel@vger.kernel.org (open list)


Takashi Sakamoto

[PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Takashi Sakamoto]

SND_SOC_BYTES_EXT was added by commit d98812082c87 ("ASoC: add
SND_SOC_BYTES_EXT") to allow drivers to transfer arbitrary byte-aligned
data to/from devices via ALSA control interface. Later, SND_SOC_BYTES_TLV
was added by commit 7523a271682f ("ASoC: core: add a helper for extended
byte controls using TLV") was added for the same purpose and developers
expected this to obsolete the former.

However, SND_SOC_BYTES_TLV brought confusions to user land because it
doesn't follow to a protocol of ALSA control interface. At least, this
macro and related kernel APIs include two misunderstandings:
 - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
   packet paylaod, snd_soc_bytes_info_ext() performs in this way.
 - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
   packet structure, snd_soc_bytes_tlv_callback() performs in this way.

In a policy of kernel land development, it's quite worse to break protocols
for applications. Therefore, developers are discouraged to use these kernel
APIs.

In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
developers who need to add control elements which transfer data larger than
the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
bytes. As long as the size is less than the size, SND_SOC_BYTES_EXT is
still available. Although there is the limitation of maximum data size,
it's better to use this API for stable application interface till better
alternative ways are implemented in future.

For these reasons, this commit reverts the previous commit which lead
developers to the worse behaviour. Unfortunately, SND_SOC_BYTES_TLV was
already merged in development cycle for kernel 3.17 and published, so
this commit doesn't touch it for any compatibilities.

Reference: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-November/069483.html
Reference: commit d98812082c87 ("ASoC: add SND_SOC_BYTES_EXT")
Reference: commit 7523a271682f ("ASoC: core: add a helper for extended byte controls using TLV")
Cc: Vinod Koul <vinod.koul@intel.com>
Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/soc.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6144882..18f7d21 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -311,9 +311,6 @@
 		{.base = xbase, .num_regs = xregs,	      \
 		 .mask = xmask }) }
 
-/*
- * SND_SOC_BYTES_EXT is deprecated, please USE SND_SOC_BYTES_TLV instead
- */
 #define SND_SOC_BYTES_EXT(xname, xcount, xhandler_get, xhandler_put) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = snd_soc_bytes_info_ext, \
-- 
2.7.4

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Takashi Sakamoto]

On Sep 16 2016 07:29, Takashi Sakamoto wrote:
> On Sep 15 2016 23:54, Mark Brown wrote:
>> On Thu, Sep 15, 2016 at 11:41:43PM +0900, Takashi Sakamoto wrote:
>>> On Sep 15 2016 23:21, Charles Keepax wrote:
>>
>>>> Although you might want to resend using Mark's kernel.org address
>>>> as I imagine he would be the one to apply the change and that is
>>>> the address he normally uses for patches.
>>
>>> Oh, he uses the domain instead of linaro.org. I missed it.
>>
>>> Now I add him to CC list.
>>
>> As documented in SubmittingPatches please send patches to the 
>> maintainers for the code you would like to change.  The normal kernel
>> workflow is that people apply patches from their inboxes, if they aren't
>> copied they are likely to not see the patch at all and it is much more
>> difficult to apply patches.
> 
> I see. I'll post this to below addresses.
> 
> $ ./scripts/get_maintainer.pl
> /tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch
> fatal:
> /tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch:
> '/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch'
> is outside repository
> Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER /
> DYNAMIC AUDIO POWER MANAGEM...)
> Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC
> AUDIO POWER MANAGEM...)
> Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
> Takashi Iwai <tiwai@suse.com> (maintainer:SOUND)
> alsa-devel@alsa-project.org (moderated list:SOUND - SOC LAYER / DYNAMIC
> AUDIO POWER MANAGEM...)
> linux-kernel@vger.kernel.org (open list)

I changed my mind. I just added you to CC list of new patch, then sent
it. Please apply it your tree after reviewing. I don't like to be
involved to the yeast, and dull work.


Regards

Takashi Sakamoto

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2148 bytes --]

On Fri, Sep 16, 2016 at 08:06:02AM +0900, Takashi Sakamoto wrote:

Please submit patches using subject lines reflecting the style for the
subsystem.  This makes it easier for people to identify relevant
patches.  Look at what existing commits in the area you're changing are
doing and make sure your subject lines visually resemble what they're
doing.

> However, SND_SOC_BYTES_TLV brought confusions to user land because it
> doesn't follow to a protocol of ALSA control interface. At least, this
> macro and related kernel APIs include two misunderstandings:
>  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
>    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
>  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
>    packet structure, snd_soc_bytes_tlv_callback() performs in this way.

I can't tell what problem you are trying to describe here, sorry.  What
are you expecting the userspace ABI to do that these controls don't,
you appear to be saying that the code does actually do what's expected
which obviously isn't a problem.  What do you expect to happen, what is
actually happening and why do you beleive this to be a problem?

> In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
> developers who need to add control elements which transfer data larger than
> the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
> bytes. As long as the size is less than the size, SND_SOC_BYTES_EXT is
> still available. Although there is the limitation of maximum data size,
> it's better to use this API for stable application interface till better
> alternative ways are implemented in future.

This doesn't make much sense to me, sorry.  This is an interface for
drivers.  They shouldn't need to know the implementation details of
different controls and we should be striving for consistency in the
interface we present both at the driver level and at the userspace
level.  Having two randomly different interfaces is not going to help
that or stop the larger controls existing, it'll just make the interface
more confusing for driver authors.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Takashi Sakamoto]

Hi Mark,

On Sep 16 2016 19:52, Mark Brown wrote:
> On Fri, Sep 16, 2016 at 08:06:02AM +0900, Takashi Sakamoto wrote:
>
> Please submit patches using subject lines reflecting the style for the
> subsystem.  This makes it easier for people to identify relevant
> patches.  Look at what existing commits in the area you're changing are
> doing and make sure your subject lines visually resemble what they're
> doing.

OK. But the different fashion in ALSA SoC part from ALSA itself often 
puzzles me.

>> However, SND_SOC_BYTES_TLV brought confusions to user land because it
>> doesn't follow to a protocol of ALSA control interface. At least, this
>> macro and related kernel APIs include two misunderstandings:
>>  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
>>    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
>>  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
>>    packet structure, snd_soc_bytes_tlv_callback() performs in this way.
>
> I can't tell what problem you are trying to describe here, sorry.  What
> are you expecting the userspace ABI to do that these controls don't,
> you appear to be saying that the code does actually do what's expected
> which obviously isn't a problem.

These two APIs don't perform what to be expected in a point of 
application interface. These two items are not what to be expected in a 
point of application interface, but what they do wrong.

> What do you expect to happen, what is
> actually happening and why do you beleive this to be a problem?

This patchset comes from a conclusion of this thread:
[alsa-devel] [RFC][PATCH 0/4] ALSA: control: return payload length of 
TLV operation
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112242.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-September/112479.html
(they were cross month end.)

If you need a minute, please read Iwai-san's post:
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-September/112534.html

>> In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
>> developers who need to add control elements which transfer data larger than
>> the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
>> bytes. As long as the size is less than the size, SND_SOC_BYTES_EXT is
>> still available. Although there is the limitation of maximum data size,
>> it's better to use this API for stable application interface till better
>> alternative ways are implemented in future.
>
> This doesn't make much sense to me, sorry.  This is an interface for
> drivers.  They shouldn't need to know the implementation details of
> different controls and we should be striving for consistency in the
> interface we present both at the driver level and at the userspace
> level.

I have no objections to this indication. I agree with it, as a driver 
developer.

> Having two randomly different interfaces is not going to help
> that or stop the larger controls existing, it'll just make the interface
> more confusing for driver authors.

On the other hand, when some driver developers actually follow to the 
message which I'd like to purge, then abuses of the interface are 
increased. Near future, it will bring confusions and disorder to ALSA 
applications. And the cause is hard to be identified from user land, 
because few developers think drivers lose protocol of application 
interface in fact.

Of cource, there's a need to work more to solve this issue. This is 
quite a small part of it.


Regards

Takashi Sakamoto

Re: [PATCH] Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2797 bytes --]

On Fri, Sep 16, 2016 at 10:21:12PM +0900, Takashi Sakamoto wrote:
> On Sep 16 2016 19:52, Mark Brown wrote:

> > Please submit patches using subject lines reflecting the style for the
> > subsystem.  This makes it easier for people to identify relevant
> > patches.  Look at what existing commits in the area you're changing are
> > doing and make sure your subject lines visually resemble what they're
> > doing.

> OK. But the different fashion in ALSA SoC part from ALSA itself often
> puzzles me.

ALSA patches don't usually start "Revert" either...

> > > However, SND_SOC_BYTES_TLV brought confusions to user land because it
> > > doesn't follow to a protocol of ALSA control interface. At least, this
> > > macro and related kernel APIs include two misunderstandings:
> > >  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
> > >    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
> > >  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
> > >    packet structure, snd_soc_bytes_tlv_callback() performs in this way.

> > I can't tell what problem you are trying to describe here, sorry.  What
> > are you expecting the userspace ABI to do that these controls don't,
> > you appear to be saying that the code does actually do what's expected
> > which obviously isn't a problem.

> These two APIs don't perform what to be expected in a point of application
> interface. These two items are not what to be expected in a point of
> application interface, but what they do wrong.

In what concrete way do they not do what is expected?

> > What do you expect to happen, what is
> > actually happening and why do you beleive this to be a problem?

> This patchset comes from a conclusion of this thread:

We need someone reading the changelog to be able to identify what the
change is doing, especially for something like this which is visible
outside the kernel.

> > Having two randomly different interfaces is not going to help
> > that or stop the larger controls existing, it'll just make the interface
> > more confusing for driver authors.

> On the other hand, when some driver developers actually follow to the
> message which I'd like to purge, then abuses of the interface are increased.
> Near future, it will bring confusions and disorder to ALSA applications. And
> the cause is hard to be identified from user land, because few developers
> think drivers lose protocol of application interface in fact.

> Of cource, there's a need to work more to solve this issue. This is quite a
> small part of it.

This isn't really going to help that too much unfortunately given that
one of the users of the new interface is the Intel drivers which are
presumably going to be one of the most common things applications
encounter.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]