Re: [PATCH 1/4] ALSA: control: return payload length for TLV operation

[Vinod Koul <vinod.koul@intel.com>]

On Wed, Aug 31, 2016 at 07:04:12AM +0900, Takashi Sakamoto wrote:
> >>> The size is already returned in scale[1] (it's initialized by
> >>> DECLARE_TLV_DB_MINMAX()).  That's exactly what the "L" in "TLV" means.
> >>>
> >>> All other TLV callbacks also take care to set this field correctly.
> >>>
> >>> If there were any TLV callback that did not set _tlv[1] to the actual
> >>> size, it would be buggy, and just needed to be fixed to do so.
> >>
> >> As I described, TLV feature of ALSA control interface is not only
> >> used to transfer threshold level information, but also arbitrary
> >> data for I/O by developers in ALSA SoC part. The '_tlv[1]' protocol
> >> is not necessarily kept by them.
> > 
> > can you explain what you mean by 'to transfer threshold level information'
> > 
> > And on this discussion, IIUC, we should fill tlv[1] with size being returned
> > right? For the asoc part, I think we should fix snd_soc_bytes_tlv_callback()
> > to update this.
> 
> 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
> 
> When implementing I/O operation, in my understanding, this situation
> often occurs, depending on driver implementation. Fortunately, current
> implementation of WM-ADSP is free from this concern, but it's better for
> us not to expect this luck always.

Thanks for the detailed explanation,

IIUC, the fix would be to ensure that drivers or ASoC does return the length
value in snd_ctl_tlv.tlv[1] per the API expectations. As I said, in ASoC we
tend to move code to core, so snd_soc_bytes_tlv_callback should be updated

Thanks
-- 
~Vinod