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

[Takashi Iwai]

On Fri, 02 Sep 2016 16:50:39 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sep 2 2016 22:09, Takashi Iwai wrote:
> > On Fri, 02 Sep 2016 13:30:33 +0200,
> > Takashi Sakamoto wrote:
> >> 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].
> > 
> > Well, without the particular purpose explained, it's hard to
> > understand why your patchset is required.  That was the failure.
> > The implementation details come after the design.
> 
> Read my comment in the first patch, again.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112245.html
> 
> I wrote 'This is inconvenient to applications'. It shows that I take
> care of ALSA applications in this patch.
>
> >> 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.
> > 
> > But your patchset changes the call patterns largely.  This annoyed me,
> > and supposedly most people, too.  If the TLV feature isn't needed to
> > be extended, the necessary change shouldn't be too intrusive, either.
> 
> Surprisingly, the annoying planted a bias to you and lost calmness in
> your brain, away from the core concept of this patchset.
> 
> Well, to me, it's really normal for I/O APIs to return processed bytes
> to applications. This is a reason that I explain like that.
> 
> Of course, I don't say I can always write comments to fully explains
> patch features so that everyone can get them. But I believe that my
> comments describes an item:
>  - I/O operation recent supported in TLV feature ignores
>    the protocol of TLV packet.
>  - Then, applications have disadvantages when calling TLV ioctl in a
>    point of operated bytes.
> 
> That's all of my concerns. Most of you interpret the item as what
> convenient to them.

Then raise these things at first before posting a intrusive patchset.
That would have made discussion far easier.


> >>> 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.
> > 
> > It's TLV, so the actual length is always encoded in the block (in
> > tlv[1]).  What's missing...?
> 
> I've already addressed two cases that 'struct snd_ctl_tlv.tlv[1]'
> doesn't have actual length of data:
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html

The usage in ASoC is really an oversight.  It shouldn't have been
accepted in that way; at least, the data passed there should have been
encapsulated in TLV.  But it's too late, so we have to live with it,
and leave it as the only exception.  As you may notice, it's an abuse
of TLV callback, and changing the other TLV callers just because of
this abuse is basically nonsense.  This is the principal idea behind
my objection.

That said, it's a restricted API, and its so per design.  It should be
used only for the case where it fits with the limitation (e.g. just
for passing the large COEF blob), and not for any generic I/O.
If we need more features from the I/O, it shouldn't be done per TLV
call but by another way.

About the user-control TLV: it's the responsibility of reader/write
that gives the right content of TLV.  IOW, the content must be TLV,
and the size is passed there.  If not, it's the fault of the write
size.

Currently kernel has no control over it, since the data passed there
is basically irrelevant with the kernel.  The verification can be done
in alsa-lib instead.


Takashi