Re: [PATCH] USB: Always select config with the highest supported UAC version

[Alexander Tsoy <alexander@xxxxxxx>]

В Вт, 13/02/2024 в 13:02 +0100, Takashi Iwai пишет:
> On Tue, 13 Feb 2024 12:05:47 +0100,
> Greg Kroah-Hartman wrote:
> > On Mon, Feb 12, 2024 at 06:28:48PM +0300, Alexander Tsoy wrote:
> > >  
> > > -			/* 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?
> 
> I believe this assumption is acceptable.  It's all about the protocol
> number from 1 to 3, so far.  If UAC4 is ever supported in future,
> it'll be highly probably the number 4.  (If not and keeping the same
> protocol number 3, we'll need a different check in anyway.)
> And the other numbers are excluded already in is_supported_uac()
> check.
> 
> > > +			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?
> 
> The condition looks cryptic, though, yes.
> 
> Maybe the check should be factored out, e.g.
> 
> /* return true if the new config has a higher priority then the old
> config */
> static bool check_uac_desc_priority(struct usb_host_config *old,
> 				struct usb_host_config *new)
> {
> 	if (!is_supported_uac(new))
> 		return false;
> 
> 	if (!is_supported_uac(old))
> 		return true;
> 
> 	/*
> 	 * Assume that bInterfaceProtocol value is always growing;
> 	 * so far, it's true from UAC1 to UAC3 (1..3)
> 	 */
> 	if (new->bInterfaceProtocol > old->bInterfaceProtocol)
> 		return true;
> 
> 	return false;
> }
> 

Thank you both for response! I'll try to simplify the logic.