| Message ID | 1472514285-3769-2-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 30 Aug 2016 01:44:42 +0200, Takashi Sakamoto wrote: > > TLV feature of control interface is originally introduced at > commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for > additional information like dB scale") and commit 8aa9b586e420 ("[ALSA] > Control API - more robust TLV implementation"). In this time, > snd_kcontrol_tlv_rw_t is for generating and transferring information about > threshold level for applications. > > This feature can transfer arbitrary data in a shape of an array with > members of unsigned int type, therefore it can be used to deliver quite > large arbitrary data from user space to in-kernel drivers via ALSA control > character device. Focusing on this nature, commit 7523a271682f ("ASoC: > core: add a helper for extended byte controls using TLV") introduced > snd_soc_bytes_tlv_callback() just for I/O operations. > > In this case, typically, APIs return operated length, while TLV feature > can't. This is inconvenient to applications. The ASoC TLV (ab)usage still takes / receives the length field of TLV. What's missing there? Takashi
On Aug 30 2016 14:29, Takashi Iwai wrote: > On Tue, 30 Aug 2016 01:44:42 +0200, > Takashi Sakamoto wrote: >> >> TLV feature of control interface is originally introduced at >> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for >> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA] >> Control API - more robust TLV implementation"). In this time, >> snd_kcontrol_tlv_rw_t is for generating and transferring information about >> threshold level for applications. >> >> This feature can transfer arbitrary data in a shape of an array with >> members of unsigned int type, therefore it can be used to deliver quite >> large arbitrary data from user space to in-kernel drivers via ALSA control >> character device. Focusing on this nature, commit 7523a271682f ("ASoC: >> core: add a helper for extended byte controls using TLV") introduced >> snd_soc_bytes_tlv_callback() just for I/O operations. >> >> In this case, typically, APIs return operated length, while TLV feature >> can't. This is inconvenient to applications. > > The ASoC TLV (ab)usage still takes / receives the length field of > TLV. What's missing there? I don't get exactly what you mean. The issue in which I'm interested is that applications cannot get to know length of actual processed bytes. Nothing others. When pure threshold level information is transferred, applications can get its length of TLV packet payload, because we have a loose protocol to store the length of data in second element of the payload. The size of 2 elements plus the length equals to the length of TLV packet payload. When using TLV feature just for I/O, this protocol is not kept, as we can see implementation of 'soc_bytes_ext' operation. Thus, the number of processed bytes should be returned to applications, by any ways. This patchset uses 'length' field in TLV packet header. Regards Takashi Sakamoto
On Tue, 30 Aug 2016 08:19:46 +0200, Takashi Sakamoto wrote: > > On Aug 30 2016 14:29, Takashi Iwai wrote: > > On Tue, 30 Aug 2016 01:44:42 +0200, > > Takashi Sakamoto wrote: > >> > >> TLV feature of control interface is originally introduced at > >> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for > >> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA] > >> Control API - more robust TLV implementation"). In this time, > >> snd_kcontrol_tlv_rw_t is for generating and transferring information about > >> threshold level for applications. > >> > >> This feature can transfer arbitrary data in a shape of an array with > >> members of unsigned int type, therefore it can be used to deliver quite > >> large arbitrary data from user space to in-kernel drivers via ALSA control > >> character device. Focusing on this nature, commit 7523a271682f ("ASoC: > >> core: add a helper for extended byte controls using TLV") introduced > >> snd_soc_bytes_tlv_callback() just for I/O operations. > >> > >> In this case, typically, APIs return operated length, while TLV feature > >> can't. This is inconvenient to applications. > > > > The ASoC TLV (ab)usage still takes / receives the length field of > > TLV. What's missing there? > > I don't get exactly what you mean. > > The issue in which I'm interested is that applications cannot get to > know length of actual processed bytes. Nothing others. > > When pure threshold level information is transferred, applications can > get its length of TLV packet payload, because we have a loose protocol > to store the length of data in second element of the payload. The size > of 2 elements plus the length equals to the length of TLV packet > payload. > > When using TLV feature just for I/O, this protocol is not kept, as we > can see implementation of 'soc_bytes_ext' operation. Thus, the number > of processed bytes should be returned to applications, by any > ways. This patchset uses 'length' field in TLV packet header. The current way of ASoC ext ctl is intended to be a workaround to pass a large data via ctl API. Nothing more than that. If it's used for receiving arbitrary size of data from the driver, it's a buggy usage. Takashi
Takashi Sakamoto wrote: > [...] APIs return operated length, while TLV feature > can't. This is inconvenient to applications. > > This commit enables control core to return operated length of TLV feature. > This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to > size variable so that each implementation of the prototype can modify the > variable with operated length. I'll use this function as an example: > --- a/sound/usb/mixer.c > +++ b/sound/usb/mixer.c > @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel, > * TLV callback for mixer volume controls > */ > int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, > - unsigned int size, unsigned int __user *_tlv) > + unsigned int *size, unsigned int __user *_tlv) > { > struct usb_mixer_elem_info *cval = kcontrol->private_data; > DECLARE_TLV_DB_MINMAX(scale, 0, 0); > > - if (size < sizeof(scale)) > + if (*size < sizeof(scale)) > return -ENOMEM; > scale[2] = cval->dBmin; > scale[3] = cval->dBmax; > if (copy_to_user(_tlv, scale, sizeof(scale))) > return -EFAULT; > + > + *size = sizeof(scale); > + > return 0; > } 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. Regards, Clemens
On Aug 30 2016 16:05, Clemens Ladisch wrote: > Takashi Sakamoto wrote: >> [...] APIs return operated length, while TLV feature >> can't. This is inconvenient to applications. >> >> This commit enables control core to return operated length of TLV feature. >> This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to >> size variable so that each implementation of the prototype can modify the >> variable with operated length. > > I'll use this function as an example: > >> --- a/sound/usb/mixer.c >> +++ b/sound/usb/mixer.c >> @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel, >> * TLV callback for mixer volume controls >> */ >> int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, >> - unsigned int size, unsigned int __user *_tlv) >> + unsigned int *size, unsigned int __user *_tlv) >> { >> struct usb_mixer_elem_info *cval = kcontrol->private_data; >> DECLARE_TLV_DB_MINMAX(scale, 0, 0); >> >> - if (size < sizeof(scale)) >> + if (*size < sizeof(scale)) >> return -ENOMEM; >> scale[2] = cval->dBmin; >> scale[3] = cval->dBmax; >> if (copy_to_user(_tlv, scale, sizeof(scale))) >> return -EFAULT; >> + >> + *size = sizeof(scale); >> + >> return 0; >> } > > 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. Regards Takashi Sakamoto
On Aug 30 2016 15:59, Takashi Iwai wrote: > On Tue, 30 Aug 2016 08:19:46 +0200, > Takashi Sakamoto wrote: >> >> On Aug 30 2016 14:29, Takashi Iwai wrote: >>> On Tue, 30 Aug 2016 01:44:42 +0200, >>> Takashi Sakamoto wrote: >>>> >>>> TLV feature of control interface is originally introduced at >>>> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for >>>> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA] >>>> Control API - more robust TLV implementation"). In this time, >>>> snd_kcontrol_tlv_rw_t is for generating and transferring information about >>>> threshold level for applications. >>>> >>>> This feature can transfer arbitrary data in a shape of an array with >>>> members of unsigned int type, therefore it can be used to deliver quite >>>> large arbitrary data from user space to in-kernel drivers via ALSA control >>>> character device. Focusing on this nature, commit 7523a271682f ("ASoC: >>>> core: add a helper for extended byte controls using TLV") introduced >>>> snd_soc_bytes_tlv_callback() just for I/O operations. >>>> >>>> In this case, typically, APIs return operated length, while TLV feature >>>> can't. This is inconvenient to applications. >>> >>> The ASoC TLV (ab)usage still takes / receives the length field of >>> TLV. What's missing there? >> >> I don't get exactly what you mean. >> >> The issue in which I'm interested is that applications cannot get to >> know length of actual processed bytes. Nothing others. >> >> When pure threshold level information is transferred, applications can >> get its length of TLV packet payload, because we have a loose protocol >> to store the length of data in second element of the payload. The size >> of 2 elements plus the length equals to the length of TLV packet >> payload. >> >> When using TLV feature just for I/O, this protocol is not kept, as we >> can see implementation of 'soc_bytes_ext' operation. Thus, the number >> of processed bytes should be returned to applications, by any >> ways. This patchset uses 'length' field in TLV packet header. > > The current way of ASoC ext ctl is intended to be a workaround to pass > a large data via ctl API. Nothing more than that. If it's used for > receiving arbitrary size of data from the driver, it's a buggy usage. It's not workaround, it's actually used. It's better to consider as one of supported ways and pay enough attention so that applications don't get disadvantages from it. The way to use is depending on actual implementation of each driver. It's better not to force policy of the usage to driver developers, I think. Regards Takashi Sakamoto
On Tue, 30 Aug 2016 09:13:54 +0200, Takashi Sakamoto wrote: > > On Aug 30 2016 15:59, Takashi Iwai wrote: > > On Tue, 30 Aug 2016 08:19:46 +0200, > > Takashi Sakamoto wrote: > >> > >> On Aug 30 2016 14:29, Takashi Iwai wrote: > >>> On Tue, 30 Aug 2016 01:44:42 +0200, > >>> Takashi Sakamoto wrote: > >>>> > >>>> TLV feature of control interface is originally introduced at > >>>> commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for > >>>> additional information like dB scale") and commit 8aa9b586e420 ("[ALSA] > >>>> Control API - more robust TLV implementation"). In this time, > >>>> snd_kcontrol_tlv_rw_t is for generating and transferring information about > >>>> threshold level for applications. > >>>> > >>>> This feature can transfer arbitrary data in a shape of an array with > >>>> members of unsigned int type, therefore it can be used to deliver quite > >>>> large arbitrary data from user space to in-kernel drivers via ALSA control > >>>> character device. Focusing on this nature, commit 7523a271682f ("ASoC: > >>>> core: add a helper for extended byte controls using TLV") introduced > >>>> snd_soc_bytes_tlv_callback() just for I/O operations. > >>>> > >>>> In this case, typically, APIs return operated length, while TLV feature > >>>> can't. This is inconvenient to applications. > >>> > >>> The ASoC TLV (ab)usage still takes / receives the length field of > >>> TLV. What's missing there? > >> > >> I don't get exactly what you mean. > >> > >> The issue in which I'm interested is that applications cannot get to > >> know length of actual processed bytes. Nothing others. > >> > >> When pure threshold level information is transferred, applications can > >> get its length of TLV packet payload, because we have a loose protocol > >> to store the length of data in second element of the payload. The size > >> of 2 elements plus the length equals to the length of TLV packet > >> payload. > >> > >> When using TLV feature just for I/O, this protocol is not kept, as we > >> can see implementation of 'soc_bytes_ext' operation. Thus, the number > >> of processed bytes should be returned to applications, by any > >> ways. This patchset uses 'length' field in TLV packet header. > > > > The current way of ASoC ext ctl is intended to be a workaround to pass > > a large data via ctl API. Nothing more than that. If it's used for > > receiving arbitrary size of data from the driver, it's a buggy usage. > > It's not workaround, it's actually used. ... more than passing a large data? If yes, it must be a bug. > It's better to consider as > one of supported ways and pay enough attention so that applications > don't get disadvantages from it. > > The way to use is depending on actual implementation of each > driver. It's better not to force policy of the usage to driver > developers, I think. I don't agree, sorry. The soc ext ctrl was introduced just for passing a large set of data. Nothing more than that. If we need to handle the processed data size in return, TLV is no right API to use at all. It just needs to use another API. TLV callback can notify whether it handles all or it fails. That's enough for the control elements. What else do you expect? Takashi
Takashi Sakamoto wrote: > On Aug 30 2016 16:05, Clemens Ladisch wrote: >> 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. > > 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. Which specific ASoC driver does this? Regards, Clemens
On Aug 30 2016 17:04, Clemens Ladisch wrote: > Takashi Sakamoto wrote: >> On Aug 30 2016 16:05, Clemens Ladisch wrote: >>> 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. >> >> 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. > > Which specific ASoC driver does this? As of 4.8, soc-wm-adsp does it, with a help of soc-core. Regards Takashi Sakamoto
On Tue, Aug 30, 2016 at 04:09:33PM +0900, Takashi Sakamoto wrote: > On Aug 30 2016 16:05, Clemens Ladisch wrote: > >Takashi Sakamoto wrote: > >>[...] APIs return operated length, while TLV feature > >>can't. This is inconvenient to applications. > >> > >>This commit enables control core to return operated length of TLV feature. > >>This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to > >>size variable so that each implementation of the prototype can modify the > >>variable with operated length. > > > >I'll use this function as an example: > > > >>--- a/sound/usb/mixer.c > >>+++ b/sound/usb/mixer.c > >>@@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel, > >> * TLV callback for mixer volume controls > >> */ > >> int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, > >>- unsigned int size, unsigned int __user *_tlv) > >>+ unsigned int *size, unsigned int __user *_tlv) > >> { > >> struct usb_mixer_elem_info *cval = kcontrol->private_data; > >> DECLARE_TLV_DB_MINMAX(scale, 0, 0); > >> > >>- if (size < sizeof(scale)) > >>+ if (*size < sizeof(scale)) > >> return -ENOMEM; > >> scale[2] = cval->dBmin; > >> scale[3] = cval->dBmax; > >> if (copy_to_user(_tlv, scale, sizeof(scale))) > >> return -EFAULT; > >>+ > >>+ *size = sizeof(scale); > >>+ > >> return 0; > >> } > > > >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. Thanks
On Aug 30 2016 23:51, Vinod Koul wrote: > On Tue, Aug 30, 2016 at 04:09:33PM +0900, Takashi Sakamoto wrote: >> On Aug 30 2016 16:05, Clemens Ladisch wrote: >>> Takashi Sakamoto wrote: >>>> [...] APIs return operated length, while TLV feature >>>> can't. This is inconvenient to applications. >>>> >>>> This commit enables control core to return operated length of TLV feature. >>>> This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to >>>> size variable so that each implementation of the prototype can modify the >>>> variable with operated length. >>> >>> I'll use this function as an example: >>> >>>> --- a/sound/usb/mixer.c >>>> +++ b/sound/usb/mixer.c >>>> @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel, >>>> * TLV callback for mixer volume controls >>>> */ >>>> int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, >>>> - unsigned int size, unsigned int __user *_tlv) >>>> + unsigned int *size, unsigned int __user *_tlv) >>>> { >>>> struct usb_mixer_elem_info *cval = kcontrol->private_data; >>>> DECLARE_TLV_DB_MINMAX(scale, 0, 0); >>>> >>>> - if (size < sizeof(scale)) >>>> + if (*size < sizeof(scale)) >>>> return -ENOMEM; >>>> scale[2] = cval->dBmin; >>>> scale[3] = cval->dBmax; >>>> if (copy_to_user(_tlv, scale, sizeof(scale))) >>>> return -EFAULT; >>>> + >>>> + *size = sizeof(scale); >>>> + >>>> return 0; >>>> } >>> >>> 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. Regards Takashi Sakamoto
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
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 >> >> 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 Instead of it, I proposed to use 'struct snd_ctl_tlv.length' for the length of TLV packet payload, because the original protocol is too rough as I/O operations. The issue I suggested for ALSA SoC part is one of actual examples. We can generate similar issues on user-defined control element set. Regards Takashi Sakamoto
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. Thanks, Charles
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
Takashi Iwai wrote: > TLV has never been and (will never be) an API to handle a > generic binary stream. It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB or _COEFFICIENTS, or to reserve a range of TLVT values for driver- defined types. (But we cannot change soc-wm-adsp to support only proper TLV data, because this would introduce a regression.) Anyway, the separate length value could be useful only for drivers (like soc-wm-adsp) that use a binary stream where TLV data should have been used. But the software that writes these coefficients to the WM ADSP driver is very hardware specific anyway, and it presumably already works without knowing the returned length value. So there is no case where this patchset _actually_ improves the interface. Regards, Clemens
On Wed, 31 Aug 2016 13:54:37 +0200, Clemens Ladisch wrote: > > Takashi Iwai wrote: > > TLV has never been and (will never be) an API to handle a > > generic binary stream. > > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB > or _COEFFICIENTS, or to reserve a range of TLVT values for driver- > defined types. (But we cannot change soc-wm-adsp to support only proper > TLV data, because this would introduce a regression.) Well, passing the random hw-specific data is fine, but what I meant is that a "stream" isn't suitable with TLV. (And I don't mean it's impossible, either :) Takashi
On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote: > Takashi Iwai wrote: > > TLV has never been and (will never be) an API to handle a > > generic binary stream. > > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB > or _COEFFICIENTS, or to reserve a range of TLVT values for driver- > defined types. (But we cannot change soc-wm-adsp to support only proper > TLV data, because this would introduce a regression.) > It's worth pointing out there are more users than just us, the Intel Skylake stuff being I guess the most important user. > > Anyway, the separate length value could be useful only for drivers (like > soc-wm-adsp) that use a binary stream where TLV data should have been > used. But the software that writes these coefficients to the WM ADSP > driver is very hardware specific anyway, and it presumably already works > without knowing the returned length value. So there is no case where > this patchset _actually_ improves the interface. > The software that writes these coefficients to wm_adsp is not hardware specific at all its just regular amixer and tinymix. These TLV controls are just like any other ALSA control, normal ALSA controls don't return the length that was written because the control is a certain size and the write either succeeds or fails, the same is true here. Thanks, Charles
Charles Keepax wrote: > On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote: >> Anyway, the separate length value could be useful only for drivers (like >> soc-wm-adsp) that use a binary stream where TLV data should have been >> used. But the software that writes these coefficients to the WM ADSP >> driver is very hardware specific anyway, and it presumably already works >> without knowing the returned length value. So there is no case where >> this patchset _actually_ improves the interface. > > The software that writes these coefficients to wm_adsp is > not hardware specific at all its just regular amixer and > tinymix. As far as I can see, neither amixer nor tinymix support writing or "command"ing TLV data. Do you mean alsactl or alsaucm? (And I notice that when alsa-lib's UCM loads TLV data from a file, it does check that the second word contains the correct size. Is this value also correct when reading TLV from these controls?) > These TLV controls are just like any other ALSA control You're treating it like one, but actually TLV is not a control type but metadata attached to a control. Regards, Clemens
On Wed, Aug 31, 2016 at 03:24:31PM +0200, Clemens Ladisch wrote: > Charles Keepax wrote: > > On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote: > >> Anyway, the separate length value could be useful only for drivers (like > >> soc-wm-adsp) that use a binary stream where TLV data should have been > >> used. But the software that writes these coefficients to the WM ADSP > >> driver is very hardware specific anyway, and it presumably already works > >> without knowing the returned length value. So there is no case where > >> this patchset _actually_ improves the interface. > > > > The software that writes these coefficients to wm_adsp is > > not hardware specific at all its just regular amixer and > > tinymix. > > As far as I can see, neither amixer nor tinymix support writing > or "command"ing TLV data. Do you mean alsactl or alsaucm? > > (And I notice that when alsa-lib's UCM loads TLV data from a file, > it does check that the second word contains the correct size. Is > this value also correct when reading TLV from these controls?) > Certainly tinyalsa does: https://github.com/tinyalsa/tinyalsa/commit/45b2d047b8c2f4d9d1d87244f7f981db8234c906 I thought the Intel guys added support to amixer as well although I may have been mistaken about that. I will try to find some time to look at that a little more closely. I guess my main concern here was the "very hardware specific" the intention is not to require hardware specific user-space code to use these controls. > > These TLV controls are just like any other ALSA control > > You're treating it like one, but actually TLV is not a control type but > metadata attached to a control. > Yes but that metadata is just being used to transfer the actual data for the control in this case. Personally I would just expect that control to function the same way as any other binary control for the user, just it is >512 bytes. Different things happen on the back end but it would be nice if it felt the same to the person using it. Which is mostly the case though tinyalsa at the moment. Thanks, Charles
On Aug 31 2016 21:08, Takashi Iwai wrote: > On Wed, 31 Aug 2016 13:54:37 +0200, > Clemens Ladisch wrote: >> >> Takashi Iwai wrote: >>> TLV has never been and (will never be) an API to handle a >>> generic binary stream. >> >> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB >> or _COEFFICIENTS, or to reserve a range of TLVT values for driver- >> defined types. (But we cannot change soc-wm-adsp to support only proper >> TLV data, because this would introduce a regression.) > > Well, passing the random hw-specific data is fine, but what I meant is > that a "stream" isn't suitable with TLV. (And I don't mean it's > impossible, either :) I'm few interested in whether it's binary stream or not, or hardware-specific or not. The returned length is the most, in a point of application developers. I'll show you two cases. 1.Let's see current implementation of 'snd_soc_bytes_tlv_callback()' in sound/soc/soc-ops.c. http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772 Here, 'size' argument is struct snd_ctl_tlv.length and 'tlv' argument is &struct snd_ctl_tlv.tlv[2]. The size of actual I/O is smaller value between the length and hardcoded 'params->max'. When applications give larger buffer to TLV feature of ALSA control interface, then the early part of the buffer is filled. On the other hand, applications cannot get to know the actual length. 2.Let's see core implementation user-defined control element set. http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c?h=v4.8-rc4#n1140 The 'size' and 'tlv' arguments are the same as the first example. When write operation, the given buffer and length is stored to a control element set. When read operation, the given buffer is filled by the stored data with the stored length. On the other hand, applications cannot get to know the actual length. When we're considering just about pure threshold information, there's no concern about the length, due to struct snd_ctl_tlv.tlv[1]. But now we've already decided to use this feature to transfer arbitrary length of data. It's better to consider about what is better for applications. In this point, I believe that my patchset is not fishy at all. In the end of this message, I put a sample program for second example. Regards Takashi Sakamoto ----- 8< ----- #include <stdio.h> #include <stdlib.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> #include <sys/ioctl.h> #include <unistd.h> #include <string.h> #include <errno.h> #include <sound/asound.h> int main(void) { struct snd_ctl_tlv *packet; int fd; struct snd_ctl_elem_info info = {0}; packet = malloc(100); if (packet == NULL) { printf("malloc(3): %s\n", strerror(ENOMEM)); goto end_malloc; } fd = open("/dev/snd/controlC0", O_RDONLY); if (fd < 0) { printf("open(2): %s\n", strerror(errno)); goto end_malloc; } /* Add my control element set. Don't forget to remove them. */ info.id.iface = SNDRV_CTL_ELEM_IFACE_MIXER; info.owner = 100; strcpy((char *)info.id.name, "sample"); info.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_WRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ | SNDRV_CTL_ELEM_ACCESS_TLV_WRITE; info.count = 128; info.type = SNDRV_CTL_ELEM_TYPE_INTEGER; info.value.integer.min = 0; info.value.integer.max = 100; info.value.integer.step = 1; if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_ADD, &info) < 0) { printf("ioctl(ADD): %s\n", strerror(errno)); goto end_malloc; } /* In a case to store arbitrary data. */ packet->numid = info.id.numid; packet->length = 6 * sizeof(packet->tlv[0]); packet->tlv[0] = 'a'; packet->tlv[1] = 'b'; packet->tlv[2] = 'c'; packet->tlv[3] = 'd'; packet->tlv[4] = 'e'; packet->tlv[5] = 'f'; if (ioctl(fd, SNDRV_CTL_IOCTL_TLV_WRITE, packet) < 0) { printf("ioctl(TLV_WRITE): %s\n", strerror(errno)); goto end_addition; } /* 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)); end_addition: /* For a combination of PulseAudio and alsa-lib 1.1.1. */ sleep(2); if (ioctl(fd, SNDRV_CTL_IOCTL_ELEM_REMOVE, &info.id) < 0) printf("ioctl(REMOVE): %s\n", strerror(errno)); end_malloc: free(packet); return EXIT_SUCCESS; }
On Wed, 31 Aug 2016 17:26:14 +0200, Takashi Sakamoto wrote: > > On Aug 31 2016 21:08, Takashi Iwai wrote: > > On Wed, 31 Aug 2016 13:54:37 +0200, > > Clemens Ladisch wrote: > >> > >> Takashi Iwai wrote: > >>> TLV has never been and (will never be) an API to handle a > >>> generic binary stream. > >> > >> It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB > >> or _COEFFICIENTS, or to reserve a range of TLVT values for driver- > >> defined types. (But we cannot change soc-wm-adsp to support only proper > >> TLV data, because this would introduce a regression.) > > > > Well, passing the random hw-specific data is fine, but what I meant is > > that a "stream" isn't suitable with TLV. (And I don't mean it's > > impossible, either :) > > I'm few interested in whether it's binary stream or not, or > hardware-specific or not. The returned length is the most, in a point of > application developers. I'll show you two cases. > > 1.Let's see current implementation of 'snd_soc_bytes_tlv_callback()' in > sound/soc/soc-ops.c. > > http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772 > > Here, 'size' argument is struct snd_ctl_tlv.length and 'tlv' argument is > &struct snd_ctl_tlv.tlv[2]. > > The size of actual I/O is smaller value between the length and hardcoded > 'params->max'. > > When applications give larger buffer to TLV feature of ALSA control > interface, then the early part of the buffer is filled. On the other > hand, applications cannot get to know the actual length. > > 2.Let's see core implementation user-defined control element set. > > http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c?h=v4.8-rc4#n1140 > > The 'size' and 'tlv' arguments are the same as the first example. > > When write operation, the given buffer and length is stored to a control > element set. When read operation, the given buffer is filled by the > stored data with the stored length. On the other hand, applications > cannot get to know the actual length. > > When we're considering just about pure threshold information, there's no > concern about the length, due to struct snd_ctl_tlv.tlv[1]. But now > we've already decided to use this feature to transfer arbitrary length > of data. It's better to consider about what is better for applications. Which application do you have in mind? Applications would access via either alsa-lib or tinyalsa. And these libraries do already care about how to access via TLV. What makes better what, practically seen? Takashi
On Wed, Aug 31, 2016 at 03:18:47PM +0100, Charles Keepax wrote: > On Wed, Aug 31, 2016 at 03:24:31PM +0200, Clemens Ladisch wrote: > > Charles Keepax wrote: > > > On Wed, Aug 31, 2016 at 01:54:37PM +0200, Clemens Ladisch wrote: > > >> Anyway, the separate length value could be useful only for drivers (like > > >> soc-wm-adsp) that use a binary stream where TLV data should have been > > >> used. But the software that writes these coefficients to the WM ADSP > > >> driver is very hardware specific anyway, and it presumably already works > > >> without knowing the returned length value. So there is no case where > > >> this patchset _actually_ improves the interface. > > > > > > The software that writes these coefficients to wm_adsp is > > > not hardware specific at all its just regular amixer and > > > tinymix. > > > > As far as I can see, neither amixer nor tinymix support writing > > or "command"ing TLV data. Do you mean alsactl or alsaucm? > > > > (And I notice that when alsa-lib's UCM loads TLV data from a file, > > it does check that the second word contains the correct size. Is > > this value also correct when reading TLV from these controls?) > > > > Certainly tinyalsa does: > > https://github.com/tinyalsa/tinyalsa/commit/45b2d047b8c2f4d9d1d87244f7f981db8234c906 > > I thought the Intel guys added support to amixer as well although > I may have been mistaken about that. I will try to find some time > to look at that a little more closely. I guess my main concern > here was the "very hardware specific" the intention is not to > require hardware specific user-space code to use these controls. Note yet, unfortunately :( I have been trying to do that for amixer. I should be able to complete at least before our uConf :) > > > > These TLV controls are just like any other ALSA control > > > > You're treating it like one, but actually TLV is not a control type but > > metadata attached to a control. > > > > Yes but that metadata is just being used to transfer the actual > data for the control in this case. Personally I would just expect > that control to function the same way as any other binary control > for the user, just it is >512 bytes. Different things happen on > the back end but it would be nice if it felt the same to the > person using it. Which is mostly the case though tinyalsa at the > moment. > > Thanks, > Charles
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. 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. I already wrote patchset for alternative TLV APIs to alsa-lib: https://github.com/takaswie/alsa-lib/tree/new-tlv-api In the remote branch, you can see new APIs at commit 5f13de, which allows applications to receive the length of actually handled data: https://github.com/takaswie/alsa-lib/commit/5f13deacfc65d26d6acbb066da0f2c35538f7497 > What makes better what, practically seen? Already described in this message. If you're still against to this patchset, I'm OK to cancel this discussion. But in this case, involuntarily, when my friends are going to use TLV feature for their ALSA applications (I hope it's unlikely), I'll recommend them not to use it, because it certainly confuses application developers and bring them to particular-hardware-specific something with the lack of transparency. That's a waste of their expensive time. [1] Apparently, Vinod Koul didn't read the comment before joining in this discussion. He did the same behavior to my former patchset. http://mailman.alsa-project.org/pipermail/alsa-devel/2016-July/110335.html I don't prefer prompt responses just to external stimulus, because we're not wild animals but human being with reasons. I use my private time to this project and hope to avoid wasting my life for this kind of unreasonable communication, because good communication is based on enough understandings in each side. [2] Charles Keepax still seem to have interests only in hardware to which he's currently related. He presumably has no good view for API designs for long term usage. http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112378.html [3] Clemens got near to where I stand. He already gave me his idea to this patchset, with an alternative idea. I'm really appreciate for his judgment, even if it's against my idea. His advices have often helped me, then I respect his way to work for this subsystem. http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112373.html [4] [alsa-devel] [PATCH 1/4] ALSA: control: return payload length for TLV operation http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html [5] http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772 Regards Takashi Sakamoto
On Fri, 02 Sep 2016 13:30:33 +0200, 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]. 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. > 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. > > 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...? > 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. > > I already wrote patchset for alternative TLV APIs to alsa-lib: > https://github.com/takaswie/alsa-lib/tree/new-tlv-api > > In the remote branch, you can see new APIs at commit 5f13de, which > allows applications to receive the length of actually handled data: > https://github.com/takaswie/alsa-lib/commit/5f13deacfc65d26d6acbb066da0f2c35538f7497 > > > > What makes better what, practically seen? > > Already described in this message. > > If you're still against to this patchset, I'm OK to cancel this discussion. > > But in this case, involuntarily, when my friends are going to use TLV > feature for their ALSA applications (I hope it's unlikely), I'll > recommend them not to use it, because it certainly confuses application > developers and bring them to particular-hardware-specific something with > the lack of transparency. That's a waste of their expensive time. The usage of TLV for this type of communication has *NEVER* been recommended. That's why I wrote it was an "(ab)usage". It was accepted just because the proposed usage fitted with the current TLV access pattern. If it were for more generic purpose, it wouldn't have been accepted from the beginning. The extended API is merely to pass a large blob over the control API. That's the only purpose, and wouldn't be more than that. thanks, Takashi > [1] Apparently, Vinod Koul didn't read the comment before joining in > this discussion. He did the same behavior to my former patchset. > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-July/110335.html > > I don't prefer prompt responses just to external stimulus, because we're > not wild animals but human being with reasons. I use my private time to > this project and hope to avoid wasting my life for this kind of > unreasonable communication, because good communication is based on > enough understandings in each side. > > [2] Charles Keepax still seem to have interests only in hardware to > which he's currently related. He presumably has no good view for API > designs for long term usage. > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112378.html > > [3] Clemens got near to where I stand. He already gave me his idea to > this patchset, with an alternative idea. I'm really appreciate for his > judgment, even if it's against my idea. His advices have often helped > me, then I respect his way to work for this subsystem. > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112373.html > > [4] [alsa-devel] [PATCH 1/4] ALSA: control: return payload length for > TLV operation > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/112398.html > > [5] > http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/soc/soc-ops.c?h=v4.8-rc4#n772 > > > Regards > > Takashi Sakamoto >
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. >>> 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 >> 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. >> >> I already wrote patchset for alternative TLV APIs to alsa-lib: >> https://github.com/takaswie/alsa-lib/tree/new-tlv-api >> >> In the remote branch, you can see new APIs at commit 5f13de, which >> allows applications to receive the length of actually handled data: >> https://github.com/takaswie/alsa-lib/commit/5f13deacfc65d26d6acbb066da0f2c35538f7497 >> >> >>> What makes better what, practically seen? >> >> Already described in this message. >> >> If you're still against to this patchset, I'm OK to cancel this discussion. >> >> But in this case, involuntarily, when my friends are going to use TLV >> feature for their ALSA applications (I hope it's unlikely), I'll >> recommend them not to use it, because it certainly confuses application >> developers and bring them to particular-hardware-specific something with >> the lack of transparency. That's a waste of their expensive time. > > The usage of TLV for this type of communication has *NEVER* been > recommended. That's why I wrote it was an "(ab)usage". It was > accepted just because the proposed usage fitted with the current TLV > access pattern. If it were for more generic purpose, it wouldn't have > been accepted from the beginning. > > The extended API is merely to pass a large blob over the control API. > That's the only purpose, and wouldn't be more than that. You missed my intention, again. Please be in user space, then consider about TLV ioctls and the state of buffers, length. TLV feature supports read, write and command operations. I'm sorry but it's time to go to bed. I got tired because it's the last day of daily work in a week... Thanks Takashi Sakamoto
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
On Fri, 02 Sep 2016 17:19:07 +0200, Takashi Iwai wrote: > > 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. s/write size/write side/ Takashi
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. 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. > 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. Thanks, Charles
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
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
On Sep 5 2016 05:45, Takashi Iwai wrote: > 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. OK. I welcome to abandon this patchset ;) > 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. +1 > 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. +1 > (Well, it's a philosophical argument: what one would expect for an > element that has neither read nor write access...?) It's an element with no sense for applications. A waste of codes in kernel land. > 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. The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit time to switch my mind for this issue. Regards Takashi Sakamoto
On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote: > On Sep 5 2016 05:45, Takashi Iwai wrote: > >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. > > OK. I welcome to abandon this patchset ;) > > >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. > > +1 > > >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. > > +1 > > >(Well, it's a philosophical argument: what one would expect for an > > element that has neither read nor write access...?) > > It's an element with no sense for applications. A waste of codes in kernel > land. > > >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. > > The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit > time to switch my mind for this issue. Perhaps we should add this as a topic for discussion at the Audio mini-conference? If the general feeling is that this feature is badly designed we should certainly be looking at what we can do to improve it. I do very much like the idea of the additional access flag as I said. Wrapping the data in a TLV structure we would have to think about a little more though as the code has shipped in several kernel versions at this point. We are starting to have a few customers use a 4.4 kernel which does include these controls, all our previous backports had used a system of partitioning the controls up into multiple 512 byte controls as these binary TLV controls were not supported. So there is a little bit of friction to major changes to the ABI, but I don't think its insurmountable as long as the functionality remains the same. But we need to get Intel involved in that discussion too, as they have used these controls quite widely as well. Thanks, Charles
On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote: > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote: > > On Sep 5 2016 05:45, Takashi Iwai wrote: > > >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. > > > > OK. I welcome to abandon this patchset ;) > > > > >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. > > > > +1 > > > > >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. > > > > +1 > > > > >(Well, it's a philosophical argument: what one would expect for an > > > element that has neither read nor write access...?) > > > > It's an element with no sense for applications. A waste of codes in kernel > > land. > > > > >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. > > > > The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit > > time to switch my mind for this issue. > > Perhaps we should add this as a topic for discussion at the Audio > mini-conference? If the general feeling is that this feature is > badly designed we should certainly be looking at what we can do > to improve it. > > I do very much like the idea of the additional access flag as I > said. Wrapping the data in a TLV structure we would have to think > about a little more though as the code has shipped in several > kernel versions at this point. > > We are starting to have a few customers use a 4.4 kernel which > does include these controls, all our previous backports had used > a system of partitioning the controls up into multiple 512 byte > controls as these binary TLV controls were not supported. So > there is a little bit of friction to major changes to the ABI, > but I don't think its insurmountable as long as the functionality > remains the same. > > But we need to get Intel involved in that discussion too, as they > have used these controls quite widely as well. Sorry havent been able to follow this yet :( But yes current Skylake Chromebooks ship with this code so we cant break it. I am not sure what is the issue with API though. (sorry haven't read the thread yet). The tlv was designed to allow people send bytes larger than 512 down to kernel. The Type cna be anything (implementation specific, though we haven't used it yet), length the blob length and then the bytes blob. We provide a tunnel and pass these to DSP. They maybe module coefficients, hotwording blobs etc. Sure, we can this at u-conf :) F2F is always much better ... Thanks
On Mon, 12 Sep 2016 17:25:31 +0200, Vinod Koul wrote: > > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote: > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote: > > > On Sep 5 2016 05:45, Takashi Iwai wrote: > > > >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. > > > > > > OK. I welcome to abandon this patchset ;) > > > > > > >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. > > > > > > +1 > > > > > > >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. > > > > > > +1 > > > > > > >(Well, it's a philosophical argument: what one would expect for an > > > > element that has neither read nor write access...?) > > > > > > It's an element with no sense for applications. A waste of codes in kernel > > > land. > > > > > > >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. > > > > > > The 'abuse' is a part of my understanding of ALSA SoC part. I need a bit > > > time to switch my mind for this issue. > > > > Perhaps we should add this as a topic for discussion at the Audio > > mini-conference? If the general feeling is that this feature is > > badly designed we should certainly be looking at what we can do > > to improve it. > > > > I do very much like the idea of the additional access flag as I > > said. Wrapping the data in a TLV structure we would have to think > > about a little more though as the code has shipped in several > > kernel versions at this point. > > > > We are starting to have a few customers use a 4.4 kernel which > > does include these controls, all our previous backports had used > > a system of partitioning the controls up into multiple 512 byte > > controls as these binary TLV controls were not supported. So > > there is a little bit of friction to major changes to the ABI, > > but I don't think its insurmountable as long as the functionality > > remains the same. > > > > But we need to get Intel involved in that discussion too, as they > > have used these controls quite widely as well. > > Sorry havent been able to follow this yet :( > > > But yes current Skylake Chromebooks ship with this code so we cant break it. > > I am not sure what is the issue with API though. (sorry haven't read the > thread yet). The tlv was designed to allow people send bytes larger than 512 > down to kernel. The Type cna be anything (implementation specific, though we > haven't used it yet), length the blob length and then the bytes blob. Yes, and this part is missing in wm_adsp driver. It passes the blob without TLV encoding, i.e. starting from the offset zero without type and length encoding. > We provide a tunnel and pass these to DSP. They maybe module coefficients, > hotwording blobs etc. So, does Intel driver pass the blob in TLV format? Then we have two different implementations. Also, still another point is to be decided: is passing an arbitrary size via info callback for an element without read/write access bits (but with TLV bit) a right behavior? > Sure, we can this at u-conf :) F2F is always much better ... Well, the u-conf is still over a month ahead. I think we should keep discussing on ML before u-conf. thanks, Takashi
On Mon, Sep 12, 2016 at 05:28:58PM +0200, Takashi Iwai wrote: > On Mon, 12 Sep 2016 17:25:31 +0200, > Vinod Koul wrote: > > > > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote: > > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote: > > > > On Sep 5 2016 05:45, Takashi Iwai wrote: > > > > Sorry havent been able to follow this yet :( > > > > > > But yes current Skylake Chromebooks ship with this code so we cant break it. > > > > I am not sure what is the issue with API though. (sorry haven't read the > > thread yet). The tlv was designed to allow people send bytes larger than 512 > > down to kernel. The Type cna be anything (implementation specific, though we > > haven't used it yet), length the blob length and then the bytes blob. > > Yes, and this part is missing in wm_adsp driver. It passes the blob > without TLV encoding, i.e. starting from the offset zero without type > and length encoding. > > > We provide a tunnel and pass these to DSP. They maybe module coefficients, > > hotwording blobs etc. > > So, does Intel driver pass the blob in TLV format? Then we have two > different implementations. > So looking at the Skylake code it does look like certainly the read opertaion returns a TLV header, its a bit less clear with the write operation it looks like it does use one but sometimes it will be passed straight through to the firmware. So looks more like we really should take the pain of the ABI change and update wm_adsp to be consistent, two implementations is not good. Although I do feel we should add/strip the header in ASoC rather than expecting the driver callbacks to do so, seems odd to push that requirement into end drivers, it has certainly taken me by surpise. Also I guess with the current tinyalsa implementation the end user has to add/strip the headers whereas really I would have thought tinyalsa should be doing that. > Also, still another point is to be decided: is passing an arbitrary > size via info callback for an element without read/write access bits > (but with TLV bit) a right behavior? > So I guess the question would be if you couldn't read the controls size from info how would you find out the control size? Thanks, Charles
On Mon, 12 Sep 2016 18:03:16 +0200, Charles Keepax wrote: > > On Mon, Sep 12, 2016 at 05:28:58PM +0200, Takashi Iwai wrote: > > On Mon, 12 Sep 2016 17:25:31 +0200, > > Vinod Koul wrote: > > > > > > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote: > > > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote: > > > > > On Sep 5 2016 05:45, Takashi Iwai wrote: > > > > > > Sorry havent been able to follow this yet :( > > > > > > > > > But yes current Skylake Chromebooks ship with this code so we cant break it. > > > > > > I am not sure what is the issue with API though. (sorry haven't read the > > > thread yet). The tlv was designed to allow people send bytes larger than 512 > > > down to kernel. The Type cna be anything (implementation specific, though we > > > haven't used it yet), length the blob length and then the bytes blob. > > > > Yes, and this part is missing in wm_adsp driver. It passes the blob > > without TLV encoding, i.e. starting from the offset zero without type > > and length encoding. > > > > > We provide a tunnel and pass these to DSP. They maybe module coefficients, > > > hotwording blobs etc. > > > > So, does Intel driver pass the blob in TLV format? Then we have two > > different implementations. > > > > So looking at the Skylake code it does look like certainly the > read opertaion returns a TLV header, its a bit less clear with > the write operation it looks like it does use one but sometimes > it will be passed straight through to the firmware. > > So looks more like we really should take the pain of the ABI > change and update wm_adsp to be consistent, two implementations > is not good. Although I do feel we should add/strip the header in > ASoC rather than expecting the driver callbacks to do so, seems > odd to push that requirement into end drivers, it has certainly > taken me by surpise. Also I guess with the current tinyalsa > implementation the end user has to add/strip the headers whereas > really I would have thought tinyalsa should be doing that. If we can align the behavior, it'd be great. And, if we can fix the wm_adsp side, we can have the common handler of TLV inside the ASoC caller side, as one of Sakamoto-san's patches showed. > > Also, still another point is to be decided: is passing an arbitrary > > size via info callback for an element without read/write access bits > > (but with TLV bit) a right behavior? > > > > So I guess the question would be if you couldn't read the > controls size from info how would you find out the control size? Well, I'm not against the idea to expose the size in callback. The behavior without read/write bits is just undefined, and we need a clear definition to avoid further confusion. I guess the introduction of a new flag would be the start. thanks, Takashi
On Mon, Sep 12, 2016 at 06:28:56PM +0200, Takashi Iwai wrote: > On Mon, 12 Sep 2016 18:03:16 +0200, > Charles Keepax wrote: > > > > On Mon, Sep 12, 2016 at 05:28:58PM +0200, Takashi Iwai wrote: > > > On Mon, 12 Sep 2016 17:25:31 +0200, > > > Vinod Koul wrote: > > > > > > > > On Mon, Sep 12, 2016 at 01:37:37PM +0100, Charles Keepax wrote: > > > > > On Tue, Sep 06, 2016 at 12:30:35PM +0900, Takashi Sakamoto wrote: > > > Also, still another point is to be decided: is passing an arbitrary > > > size via info callback for an element without read/write access bits > > > (but with TLV bit) a right behavior? > > > > > > > So I guess the question would be if you couldn't read the > > controls size from info how would you find out the control size? > > Well, I'm not against the idea to expose the size in callback. The > behavior without read/write bits is just undefined, and we need a > clear definition to avoid further confusion. I guess the introduction > of a new flag would be the start. The trouble with that is you have to do a read/write to find out the size you need to read or write though. So you end up doing weird stuff like a dummy zero length read or something, which feels a bit icky. One other thing to consider is that if we add the new flag it might be nice to allow normal read/write operations on the controls as well (only accessing the first 512 bytes of the control). Certainly for the ADSP control I would have liked to have done this to preserve better backwards compatibility with older user-spaces. Thanks, Charles
diff --git a/include/sound/control.h b/include/sound/control.h index 21d047f..848940c 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -32,7 +32,7 @@ typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol, int op_flag, /* SNDRV_CTL_TLV_OP_XXX */ - unsigned int size, + unsigned int *size, unsigned int __user *tlv); enum { diff --git a/include/sound/soc.h b/include/sound/soc.h index 6144882..cfd1ca1 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -647,7 +647,7 @@ int snd_soc_bytes_put(struct snd_kcontrol *kcontrol, int snd_soc_bytes_info_ext(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *ucontrol); int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv); + unsigned int *size, unsigned int __user *tlv); int snd_soc_info_xr_sx(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo); int snd_soc_get_xr_sx(struct snd_kcontrol *kcontrol, diff --git a/sound/core/control.c b/sound/core/control.c index fb096cb..485963e 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1139,7 +1139,7 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol, static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, + unsigned int *size, unsigned int __user *tlv) { struct user_element *ue = kcontrol->private_data; @@ -1147,19 +1147,19 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol, void *new_data; if (op_flag == SNDRV_CTL_TLV_OP_WRITE) { - if (size > 1024 * 128) /* sane value */ + if (*size > 1024 * 128) /* sane value */ return -EINVAL; - new_data = memdup_user(tlv, size); + new_data = memdup_user(tlv, *size); if (IS_ERR(new_data)) return PTR_ERR(new_data); mutex_lock(&ue->card->user_ctl_lock); - change = ue->tlv_data_size != size; + change = ue->tlv_data_size != *size; if (!change) - change = memcmp(ue->tlv_data, new_data, size); + change = memcmp(ue->tlv_data, new_data, *size); kfree(ue->tlv_data); ue->tlv_data = new_data; - ue->tlv_data_size = size; + ue->tlv_data_size = *size; mutex_unlock(&ue->card->user_ctl_lock); } else { int ret = 0; @@ -1169,12 +1169,13 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol, ret = -ENXIO; goto err_unlock; } - if (size < ue->tlv_data_size) { + if (*size < ue->tlv_data_size) { ret = -ENOSPC; goto err_unlock; } if (copy_to_user(tlv, ue->tlv_data, ue->tlv_data_size)) ret = -EFAULT; + *size = ue->tlv_data_size; err_unlock: mutex_unlock(&ue->card->user_ctl_lock); if (ret) @@ -1466,7 +1467,15 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file, err = -EPERM; goto __kctl_end; } - err = kctl->tlv.c(kctl, op_flag, tlv.length, _tlv->tlv); + len = tlv.length; + err = kctl->tlv.c(kctl, op_flag, &len, _tlv->tlv); + if (err < 0) + goto __kctl_end; + /* Return operated bytes. */ + if (put_user(len, &_tlv->length)) { + err = -EFAULT; + goto __kctl_end; + } if (err > 0) { struct snd_ctl_elem_id id = kctl->id; up_read(&card->controls_rwsem); @@ -1483,7 +1492,8 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file, err = -ENOMEM; goto __kctl_end; } - if (copy_to_user(_tlv->tlv, kctl->tlv.p, len)) + if (copy_to_user(_tlv->tlv, kctl->tlv.p, len) || + put_user(len, &_tlv->length)) err = -EFAULT; } __kctl_end: diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index bb12615..dfc84ab 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2516,7 +2516,7 @@ static int pcm_chmap_ctl_get(struct snd_kcontrol *kcontrol, * expands the pre-defined channel maps in a form of TLV */ static int pcm_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol); const struct snd_pcm_chmap_elem *map; @@ -2525,27 +2525,27 @@ static int pcm_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, if (snd_BUG_ON(!info->chmap)) return -EINVAL; - if (size < 8) + if (*size < 8) return -ENOMEM; if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv)) return -EFAULT; - size -= 8; + *size -= 8; dst = tlv + 2; for (map = info->chmap; map->channels; map++) { int chs_bytes = map->channels * 4; if (!valid_chmap_channels(info, map->channels)) continue; - if (size < 8) + if (*size < 8) return -ENOMEM; if (put_user(SNDRV_CTL_TLVT_CHMAP_FIXED, dst) || put_user(chs_bytes, dst + 1)) return -EFAULT; dst += 2; - size -= 8; + *size -= 8; count += 8; - if (size < chs_bytes) + if (*size < chs_bytes) return -ENOMEM; - size -= chs_bytes; + *size -= chs_bytes; count += chs_bytes; for (c = 0; c < map->channels; c++) { if (put_user(map->map[c], dst)) @@ -2555,6 +2555,7 @@ static int pcm_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, } if (put_user(count, tlv + 1)) return -EFAULT; + *size = count + sizeof(unsigned int) * 2; return 0; } diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c index 6c58e6f..03f4681 100644 --- a/sound/core/vmaster.c +++ b/sound/core/vmaster.c @@ -220,7 +220,7 @@ static int slave_put(struct snd_kcontrol *kcontrol, } static int slave_tlv_cmd(struct snd_kcontrol *kcontrol, - int op_flag, unsigned int size, + int op_flag, unsigned int *size, unsigned int __user *tlv) { struct link_slave *slave = snd_kcontrol_chip(kcontrol); diff --git a/sound/hda/hdmi_chmap.c b/sound/hda/hdmi_chmap.c index 81acc20..3163387 100644 --- a/sound/hda/hdmi_chmap.c +++ b/sound/hda/hdmi_chmap.c @@ -661,7 +661,7 @@ static int spk_mask_from_spk_alloc(int spk_alloc) } static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol); struct hdac_chmap *chmap = info->private_data; @@ -672,11 +672,11 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, int type; int spk_alloc, spk_mask; - if (size < 8) + if (*size < 8) return -ENOMEM; if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv)) return -EFAULT; - size -= 8; + *size -= 8; dst = tlv + 2; spk_alloc = chmap->ops.get_spk_alloc(chmap->hdac, pcm_idx); @@ -703,7 +703,7 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, chmap, cap, chs); if (type < 0) return -ENODEV; - if (size < 8) + if (*size < 8) return -ENOMEM; if (put_user(type, dst) || @@ -711,13 +711,13 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, return -EFAULT; dst += 2; - size -= 8; + *size -= 8; count += 8; - if (size < chs_bytes) + if (*size < chs_bytes) return -ENOMEM; - size -= chs_bytes; + *size -= chs_bytes; count += chs_bytes; chmap->ops.cea_alloc_to_tlv_chmap(chmap, cap, tlv_chmap, chs); @@ -731,6 +731,8 @@ static int hdmi_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, if (put_user(count, tlv + 1)) return -EFAULT; + *size = count + sizeof(unsigned int) * 2; + return 0; } diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 9913be8..06d87e7 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -1420,7 +1420,7 @@ EXPORT_SYMBOL_GPL(snd_hda_mixer_amp_volume_put); * set up via HDA_COMPOSE_AMP_VAL*() or related macros. */ int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *_tlv) + unsigned int *size, unsigned int __user *_tlv) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); hda_nid_t nid = get_amp_nid(kcontrol); @@ -1429,7 +1429,7 @@ int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag, bool min_mute = get_amp_min_mute(kcontrol); u32 caps, val1, val2; - if (size < 4 * sizeof(unsigned int)) + if (*size < 4 * sizeof(unsigned int)) return -ENOMEM; caps = query_amp_caps(codec, nid, dir); val2 = (caps & AC_AMPCAP_STEP_SIZE) >> AC_AMPCAP_STEP_SIZE_SHIFT; @@ -1447,6 +1447,9 @@ int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag, return -EFAULT; if (put_user(val2, _tlv + 3)) return -EFAULT; + + *size = sizeof(unsigned int) * 4; + return 0; } EXPORT_SYMBOL_GPL(snd_hda_mixer_amp_tlv); @@ -1737,13 +1740,14 @@ static int get_kctl_0dB_offset(struct hda_codec *codec, { int _tlv[4]; const int *tlv = NULL; + unsigned int size = sizeof(_tlv); int val = -1; if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) { /* FIXME: set_fs() hack for obtaining user-space TLV data */ mm_segment_t fs = get_fs(); set_fs(get_ds()); - if (!kctl->tlv.c(kctl, 0, sizeof(_tlv), _tlv)) + if (!kctl->tlv.c(kctl, 0, &size, _tlv)) tlv = _tlv; set_fs(fs); } else if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) @@ -2207,7 +2211,7 @@ EXPORT_SYMBOL_GPL(snd_hda_mixer_bind_ctls_put); * set up via HDA_BIND_VOL() macro. */ int snd_hda_mixer_bind_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct hda_bind_ctls *c; diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index d0e066e..35ea160 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -113,7 +113,7 @@ int snd_hda_mixer_amp_volume_get(struct snd_kcontrol *kcontrol, int snd_hda_mixer_amp_volume_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol); int snd_hda_mixer_amp_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv); + unsigned int *size, unsigned int __user *tlv); int snd_hda_mixer_amp_switch_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo); int snd_hda_mixer_amp_switch_get(struct snd_kcontrol *kcontrol, @@ -218,7 +218,7 @@ int snd_hda_mixer_bind_ctls_get(struct snd_kcontrol *kcontrol, int snd_hda_mixer_bind_ctls_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol); int snd_hda_mixer_bind_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv); + unsigned int *size, unsigned int __user *tlv); #define HDA_BIND_VOL(xname, bindrec) \ { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \ diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 9ceb2bc..bc8f342 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -3868,7 +3868,7 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol, } static int ca0132_volume_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct ca0132_spec *spec = codec->spec; diff --git a/sound/pci/lola/lola_mixer.c b/sound/pci/lola/lola_mixer.c index e7fe15d..ee92c31 100644 --- a/sound/pci/lola/lola_mixer.c +++ b/sound/pci/lola/lola_mixer.c @@ -553,14 +553,14 @@ static int lola_analog_vol_put(struct snd_kcontrol *kcontrol, } static int lola_analog_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct lola *chip = snd_kcontrol_chip(kcontrol); int dir = kcontrol->private_value; unsigned int val1, val2; struct lola_pin *pin; - if (size < 4 * sizeof(unsigned int)) + if (*size < 4 * sizeof(unsigned int)) return -ENOMEM; pin = &chip->pin[dir].pins[0]; @@ -577,6 +577,9 @@ static int lola_analog_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, return -EFAULT; if (put_user(val2, tlv + 3)) return -EFAULT; + + *size = 4 * sizeof(unsigned int); + return 0; } diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c index a513a34..b2f3aa0 100644 --- a/sound/soc/soc-ops.c +++ b/sound/soc/soc-ops.c @@ -770,20 +770,21 @@ int snd_soc_bytes_info_ext(struct snd_kcontrol *kcontrol, EXPORT_SYMBOL_GPL(snd_soc_bytes_info_ext); int snd_soc_bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct soc_bytes_ext *params = (void *)kcontrol->private_value; - unsigned int count = size < params->max ? size : params->max; int ret = -ENXIO; + *size = min_t(int, *size, params->max); + switch (op_flag) { case SNDRV_CTL_TLV_OP_READ: if (params->get) - ret = params->get(kcontrol, tlv, count); + ret = params->get(kcontrol, tlv, *size); break; case SNDRV_CTL_TLV_OP_WRITE: if (params->put) - ret = params->put(kcontrol, tlv, count); + ret = params->put(kcontrol, tlv, *size); break; } return ret; diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 2f8c388..52a47bc 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -535,17 +535,20 @@ int snd_usb_set_cur_mix_value(struct usb_mixer_elem_info *cval, int channel, * TLV callback for mixer volume controls */ int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *_tlv) + unsigned int *size, unsigned int __user *_tlv) { struct usb_mixer_elem_info *cval = kcontrol->private_data; DECLARE_TLV_DB_MINMAX(scale, 0, 0); - if (size < sizeof(scale)) + if (*size < sizeof(scale)) return -ENOMEM; scale[2] = cval->dBmin; scale[3] = cval->dBmax; if (copy_to_user(_tlv, scale, sizeof(scale))) return -EFAULT; + + *size = sizeof(scale); + return 0; } diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h index 3417ef3..ce4e236 100644 --- a/sound/usb/mixer.h +++ b/sound/usb/mixer.h @@ -84,7 +84,7 @@ void snd_usb_mixer_elem_init_std(struct usb_mixer_elem_list *list, int unitid); int snd_usb_mixer_vol_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *_tlv); + unsigned int *size, unsigned int __user *_tlv); #ifdef CONFIG_PM int snd_usb_mixer_suspend(struct usb_mixer_interface *mixer); diff --git a/sound/usb/stream.c b/sound/usb/stream.c index 8e9548bc..373d077 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -136,7 +136,7 @@ static bool have_dup_chmap(struct snd_usb_substream *subs, } static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, - unsigned int size, unsigned int __user *tlv) + unsigned int *size, unsigned int __user *tlv) { struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol); struct snd_usb_substream *subs = info->private_data; @@ -144,11 +144,11 @@ static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, unsigned int __user *dst; int count = 0; - if (size < 8) + if (*size < 8) return -ENOMEM; if (put_user(SNDRV_CTL_TLVT_CONTAINER, tlv)) return -EFAULT; - size -= 8; + *size -= 8; dst = tlv + 2; list_for_each_entry(fp, &subs->fmt_list, list) { int i, ch_bytes; @@ -159,7 +159,7 @@ static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, continue; /* copy the entry */ ch_bytes = fp->chmap->channels * 4; - if (size < 8 + ch_bytes) + if (*size < 8 + ch_bytes) return -ENOMEM; if (put_user(SNDRV_CTL_TLVT_CHMAP_FIXED, dst) || put_user(ch_bytes, dst + 1)) @@ -171,10 +171,13 @@ static int usb_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag, } count += 8 + ch_bytes; - size -= 8 + ch_bytes; + *size -= 8 + ch_bytes; } if (put_user(count, tlv + 1)) return -EFAULT; + + *size = count + sizeof(unsigned int) * 2; + return 0; }
TLV feature of control interface is originally introduced at commit 42750b04c5ba ("[ALSA] Control API - TLV implementation for additional information like dB scale") and commit 8aa9b586e420 ("[ALSA] Control API - more robust TLV implementation"). In this time, snd_kcontrol_tlv_rw_t is for generating and transferring information about threshold level for applications. This feature can transfer arbitrary data in a shape of an array with members of unsigned int type, therefore it can be used to deliver quite large arbitrary data from user space to in-kernel drivers via ALSA control character device. Focusing on this nature, commit 7523a271682f ("ASoC: core: add a helper for extended byte controls using TLV") introduced snd_soc_bytes_tlv_callback() just for I/O operations. In this case, typically, APIs return operated length, while TLV feature can't. This is inconvenient to applications. This commit enables control core to return operated length of TLV feature. This changes the prototype of 'snd_kcontrol_tlv_rw_t' to get a pointer to size variable so that each implementation of the prototype can modify the variable with operated length. This commit also changes related codes. Below modules are modified: - snd - hda-core - hda-codec - hda-codec-ca0132 - lola - usb-audio - soc-core Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- include/sound/control.h | 2 +- include/sound/soc.h | 2 +- sound/core/control.c | 28 +++++++++++++++++++--------- sound/core/pcm_lib.c | 15 ++++++++------- sound/core/vmaster.c | 2 +- sound/hda/hdmi_chmap.c | 16 +++++++++------- sound/pci/hda/hda_codec.c | 12 ++++++++---- sound/pci/hda/hda_local.h | 4 ++-- sound/pci/hda/patch_ca0132.c | 2 +- sound/pci/lola/lola_mixer.c | 7 +++++-- sound/soc/soc-ops.c | 9 +++++---- sound/usb/mixer.c | 7 +++++-- sound/usb/mixer.h | 2 +- sound/usb/stream.c | 13 ++++++++----- 14 files changed, 74 insertions(+), 47 deletions(-)