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

[Takashi Iwai]

On Sun, 04 Sep 2016 13:07:41 +0200,
Takashi Sakamoto wrote:
> 
> 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

Yeah, there are obviously some issues in the current implementation of
wm_adsp and ASoC ext ctl.  Although I'll unlikely take Sakamoto-san's
patchset as is from a few reasons, these issues still should be
addressed.

First off, passing the binary blob directly via TLV callback is
incorrect from the ABI perspective.  When Vinod proposed the idea via
TLV access originally, we thought they the data is encoded in TLV
format.  Alas, the resulted code didn't do that and it slipped into
the upstream without consideration. 

Besides that, the second problem is the count value returned via
snd_ctl_elem_info, as mentioned in the above.  It's beyond the
original control API design, and a kind of illegal usage. 
(Well, it's a philosophical argument: what one would expect for an
 element that has neither read nor write access...?)

So, at this point, the main question is whether we keep this access
pattern as is, as a sort of official one, and put some exceptional
rule.  Charles, how is the situation?  Has it been already deployed to
real systems?

If we may still change the wm_adsp behavior, we may "fix" the first
issue by passing the blob properly encoded in TLV format, at least.
OTOH, if we need to keep the current ABI abuse as is, one idea is to
add a special flag in SNDRV_CTL_ELEM_ACCESS_* indicating this kind of
control, and we define more strictly how the code should be
implemented.  Currently we can judge this element as a one that has no
read/write access but with tlv r/w.  But it's way too unclear.


thanks,

Takashi