On Sat, 15 Jun 2024 10:02:35 +0200, Takashi Sakamoto wrote: > > On Sat, Jun 15, 2024 at 09:28:50AM +0200, Takashi Iwai wrote: > > > In the commit coment, I can see "that's the only way to filter out the > > > invalid values", however it not so good idea, since the ALSA control core > > > function loses transparency against control elements somehow. > > > > Transparency? The sanity check of input values is done in each driver > > side, hence some overhead is more or less always present, depending on > > the implementation. > > > > > Furthermore, I can see "there is no corresponding driver", however it is > > > suspicious somehow. It would be smart to charge the validation > > > implementation for user-defined control element set if forcing it. > > > > The context there implies that, in the case of user elements, all > > handled in sound/core/control.c, and there is no other dedicated > > driver code handling the control put for those controls, hence > > sound/core/control.c is the only place where we can address the > > issue. > > If you can force the validation to _all_ of the existing drivers by any > kind of mechanism, it would be. Actually, not. We can have such driver > which handles the write request without such validation, and control core > allows it. The kernel configuration is to ease the detection of such > drivers (and applications) in application runtime. Therefore the > transparency would be lost by the patch. In principle, the validation should be done for *every* kcontrol. The lack of the validation was ignored so far with a naive assumption that the driver treats properly nevertheless. But since we're checking it more strictly in kselftest, the problem became more obvious, and this is a corresponding fix for user control element part. HD-audio driver had another issues and they are fixed in other patches of this series. > Assuming that two control element exist in a sound card, which has the > same information and TLV response, except for the flag of > SNDRV_CTL_ELEM_ACCESS_USER. For the same value data, one operation with > SNDRV_CTL_IOCTL_ELEM_WRITE is successful, and another operation with > SNDRV_CTL_ELEM_ACCESS_USER is failed. When encountering this issue, > the programmer of the application suspect the bug pertaining to the latter > control, then the programmer find the latter has > SNDRV_CTL_ELEM_ACCESS_USER. Then the programmer would judge that 'I got > it, it is a bug of user-defined control element set' even if the program > includes the bug for min/max/step computation and the underlying sound > driver includes the bug not to validate value data. No, it's a wrong understanding, other way round: the driver must validate the values by itself. > The patch loses transparency in the above step. Without the patch, both > operations finish with the equivalent result. > > Nevertheless, I think the validation is itself preferable. The validation is not "preferable" but rather "mandatory". > In my opinion, > the validation before/after the call of 'snd_kcontrol_put_t' would result > in the different argument. The 'validate-before-call' is the argument of > control core function, while 'validate-after-call is the argument of > implementation of user-defined element set. The patch should belong to the > latter to extend current implementation of user-defined element set. > Thus I suggest to put the validation into the put callback function, > regardless of the optimization to which you address. I don't get the argument, sorry. If you have a better point, please submit an incremental patch. thanks, Takashi
