[alsa-devel] [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Takashi Sakamoto]

On Sep 3 2016 20:38, Charles Keepax wrote:
> On Fri, Sep 02, 2016 at 08:30:33PM +0900, Takashi Sakamoto wrote:
>> Sorry to be late. I'm a bit busy for my daily work and things related
>> to my poor life.
>>
>> On Sep 1 2016 00:40, Takashi Iwai wrote:
>>> Which application do you have in mind?
>>
>> At first, I did never suggest that 'TLV feature should handle byte
>> stream' or 'TLV feature should be extended to somewhere'. I just
>> addressed that 'Now, TLV feature is not used only for transmission of
>> pure threshold information between applications/drivers, but it's also
>> used for I/O operation. Unfortunately, protocol of TLV packet payload is
>> not kept anymore.'
>>
>> It's not my intension to discuss about this patchset in a point of 'byte
>> stream'. It comes from your bias, and the other developers are led to
>> the wrong direction, sigh. I really hope them to read my comment in this
>> patchset carefully and compare them to actual code implementations
>> in driver/core/library and applications[1][2][3].
>>
>> Again, I have few interests about actual implementation of TLV feature
>> in driver side and for what purposes applications are developed to
>> utilize TLV feature. They're free for developers in each area now, and
>> for future. What we should do is how to assist their activity via the
>> design of APIs. This is my place in this patchset. It's not my intension
>> to request extensions of TLV feature.
>>
>>
>>> Applications would access via either alsa-lib or tinyalsa.  And these
>> libraries do already care about how to access via TLV.
>>
>> Without enough care due to implementation in kernel land.
>>
>> As I already cleared, current TLV feature has a difficulty not to return
>> the length of actually handled bytes[4]. Correspondingly, APIs in these
>> libraries have defections, as APIs for I/O operation or transmission of
>> arbitrary data, because applications cannot get to know the actual
>> length of handled data.
>>
> 
>     /* This simulates the other processes to read it. */
>     packet->length = 100 - sizeof(struct snd_ctl_tlv);
>     if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_READ, packet) < 0) {
>         printf("ioctl(TLV_READ): %s\n", strerror(errno));
>         goto end_addition;
>     }
> 
>     printf("struct snd_ctl_tlv.length: %d\n", packet->length);
>     printf("But I store %ld bytes.\n", 6 * sizeof(unsigned int));
> 
> Here you wrote 6 items into the control but the read said it
> gave you 100 items and you would have preferred it to have said
> 6 items? But you have created a 128 item long ALSA control if we
> forget about the TLV aspect (which in my view is just a means to
> access the control) and assume this was a regular ALSA control
> then you would never expect that to give you less data than the
> size of the control, so this behaviour would be expected.

You misunderstand the design of ALSA control core. I wrote a part of it,
later.

> As this was originally intended (at least AFAIK) as a mechanism
> to provide byte controls >512 bytes, my point is really just that
> I would expect this to work like a normal control. There is no
> need to pass the length actually processed because this isn't
> for arbitrary length data its just a way to read/write a control
> which is something that already has a defined size.

Just for current implementation, it's appropriate.

But for future, it may be certainly inappropriate.

>> There's no way for user space to get appropriate length in advance, this
>> brings contradiction for content of given buffer. For example, in ALSA
>> SoC part, the length is trimmed according to a parameter of driver
>> instance, implicitly[5]. As a result, users are beguiled. They requests
>> a certain length of buffer to be handled via TLV feature. Even if they
>> receive success, a _part_ of buffer is actually handled. This is not
>> good for software stacks in a point of abstraction layer of hardware.
>> There's no transparency independent from hardwares. Application
>> developers is forced to consider about device driver's codes which are
>> not open via APIs.
>>
> 
> That length you refer to is not some secret internal length it
> is the length of the control as returned by the info ioctl. I
> guess we could make the read/write fail if it was larger than the
> control, that might be better behaviour than just silently not
> handling all the data.

It's an abuse in a point of application interfaces.

'struct snd_ctl_elem_info.count' represents the number of members which
can hold a value, in a element of a element set. It's not for something
about TLV information.

And some core codes and user lands are written according to the design.
Please investigate 'sound/core/control.c' in Linux kernel and
'src/control/*' in alsa-lib and 'alsactl/alsaloop/amixer' in alsa-utils.
Then, we may find to lost something important in kernel land development.

The latest alsa-lib includes a document of the design of ALSA control
core. It may help your understanding.
http://www.alsa-project.org/alsa-doc/alsa-lib/control.html


Regards

Takashi Sakamoto