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

[Takashi Iwai]

On Wed, 31 Aug 2016 11:05:52 +0200,
Charles Keepax wrote:
> 
> On Wed, Aug 31, 2016 at 01:30:02PM +0900, Takashi Sakamoto wrote:
> > On Aug 31 2016 13:20, Vinod Koul wrote:
> > >> The layout of TLV packet is:
> > >> struct snd_ctl_tlv {
> > >>     unsigned int numid;   # numerical ID of a control element
> > >>     unsigned int length;  # length of payload
> > >>     unsigned int tlv[0];  # payload
> > >> };
> > >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.8-rc4#n945
> > >>
> > >> In our implementaion, TLV packet payload (struct snd_ctl_tlv.tlv) is
> > >> used to transfer data. For pure threshold level information, we expects
> > >> applications and drivers to fill the payload with this protocol:
> > >> struct snd_ctl_tlv.tlv[0]:  one of SNDRV_CTL_TLVT_XXX
> > >> struct snd_ctl_tlv.tlv[1]:  length of data
> > >> struct snd_ctl_tlv.tlv[2..]: data
> > >>
> > >> (You can see SNDRV_CTL_TLVT_XXX in this header.
> > >> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/tlv.h?h=sound-4.8-rc4
> > >> )
> > >>
> > >> On the other hand, ALSA SoC part performs:
> > >> struct snd_ctl_tlv.tlv[0..]: arbitrary data
> > >>
> > >> If your 'tlv[1]' means the 'struct snd_ctl_tlv.tlv[1]', no sense.
> > >>
> > >> The issue I address is current implementation cannot correctly handle
> > >> this case:
> > >>  - applications request a buffer with a certain size
> > >>  - drivers processes the request with smaller size
> > >>  - application cannot get the size
> 
> Is this an expected use-case? The TLV controls were implemented
> to allow ALSA controls of greater than 512 bytes, I am not sure
> the intention was to provide completely generalised binary pipe. In
> general the expection for reading a control is that you can
> always read the whole control (AFAIK), so it feels like something
> returning less than the requested amount of data is buggy.

Right.  TLV has never been and (will never be) an API to handle a
generic binary stream.  Its usage in control read/write was accepted
because it still fitted with the current TLV design as is.

I'm not against to move some TLV verification or hardening code into
the common place, as long as it improves the code.  But your current
patchset smells fishy, as if it requires surgery over all bodies.
That would require more justification.


thanks,

Takashi