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