On Mon, Feb 12, 2024 at 06:28:48PM +0300, Alexander Tsoy wrote:
> Config with the highest supported UAC version is always preferable because
> it usually provides more features.
>
> Main motivation for this change is to improve support for several UAC2
> devices which have UAC1 config as the first one for compatibility reasons.
> UAC2 mode provides a wider range of supported sampling rates on these
> devices. Also when UAC4 support is implemented, it will need just one
> additional case line without changing the logic.
>
> Signed-off-by: Alexander Tsoy <alexander@xxxxxxx>
> ---
> drivers/usb/core/generic.c | 39 +++++++++++++++++++++++++-------------
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index b134bff5c3fe..8aeb180e1cf9 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -48,9 +48,16 @@ static bool is_audio(struct usb_interface_descriptor *desc)
> return desc->bInterfaceClass == USB_CLASS_AUDIO;
> }
>
> -static bool is_uac3_config(struct usb_interface_descriptor *desc)
> +static bool is_supported_uac(struct usb_interface_descriptor *desc)
> {
> - return desc->bInterfaceProtocol == UAC_VERSION_3;
> + switch(desc->bInterfaceProtocol) {
> + case UAC_VERSION_1:
> + case UAC_VERSION_2:
> + case UAC_VERSION_3:
> + return true;
> + default:
> + return false;
> + }
> }
>
> int usb_choose_configuration(struct usb_device *udev)
> @@ -135,22 +142,28 @@ int usb_choose_configuration(struct usb_device *udev)
> }
>
> /*
> - * Select first configuration as default for audio so that
> - * devices that don't comply with UAC3 protocol are supported.
> - * But, still iterate through other configurations and
> - * select UAC3 compliant config if present.
> + * Iterate through audio configurations and select the first of
> + * the highest supported UAC versions. Select the first config
> + * if no supported UAC configs are found.
> */
> if (desc && is_audio(desc)) {
> - /* Always prefer the first found UAC3 config */
> - if (is_uac3_config(desc)) {
> - best = c;
> - break;
> - }
> + struct usb_interface_descriptor *best_desc = NULL;
> +
> + if (best)
> + best_desc = &best->intf_cache[0]->altsetting->desc;
Are you sure you always have a intf_cache[0] pointer? What about
altsetting? Remember, invalid descriptors happen all the time, and are
a known "attack vector" against the USB stack.
>
> - /* If there is no UAC3 config, prefer the first config */
> - else if (i == 0)
> + if (i == 0)
> best = c;
>
> + /* Assume that bInterfaceProtocol value is always
> + * growing when UAC versions are incremented, so that
> + * the direct comparison is possible. */
How do we know this assumption is always true? What happens when it is not?
> + else if (is_supported_uac(desc) && best_desc &&
> + (!is_supported_uac(best_desc) ||
> + (desc->bInterfaceProtocol >
> + best_desc->bInterfaceProtocol)))
> + best = c;
I really can't understand this if logic, sorry, can you describe it
better so that we can maintain it over time?
thanks,
greg k-h
