Re: usb:gadget:u_audio: Regression in [v3,3/3] usb: gadget: u_audio: add real feedback implementation - wMaxPacketSize calculation

[Greg KH <greg@kroah.com>]

On Tue, Jul 13, 2021 at 12:22:38PM +0200, Pavel Hofman wrote:
> Hi,
> 
> I am testing the three Ruslan's async feedback patches as modified by Jerome
> and I hit a regression in windows 10 enumeration.
> 
> Ruslan posted an already accepted patch https://github.com/torvalds/linux/commit/789ea77310f0200c84002884ffd628e2baf3ad8a#diff-876615ece7fb56ce8d45fc70623bef9caa2548e810426f217fb785ffa10b33d5

We do not use github for kernel stuff, just reference the git commit id
properly and all is good.

> which allowed win10 enumeration.
> 
> Ruslan's async feedback patchset kept the change: https://patchwork.kernel.org/project/linux-usb/patch/1614603943-11668-5-git-send-email-ruslan.bilovol@gmail.com/

I do not understand this reference at all.


> 
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index 72b42f8..91b22fb 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -506,6 +506,10 @@  static int set_ep_max_packet_size(const struct
> f_uac2_opts *uac2_opts,
> 
>  	max_size_bw = num_channels(chmask) * ssize *
>  		((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1);
> +
> +	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
> +		max_size_bw = max_size_bw * FBACK_FREQ_MAX / 100;
> +
>  	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>  						    max_size_ep));
> 
> 
> Jerome's rebase patch which was accepted recently changed the functionality
> to the original code:
> https://patchwork.kernel.org/project/linux-usb/patch/20210603220104.1216001-4-jbrunet@baylibre.com/
> 
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index 321e6c05ba93..ae29ff2b2b68 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -584,8 +584,11 @@  static int set_ep_max_packet_size(const struct
> f_uac2_opts *uac2_opts,
>  		ssize = uac2_opts->c_ssize;
>  	}
> 
> +	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
> +		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
> +
>  	max_size_bw = num_channels(chmask) * ssize *
> -		((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1);
> +		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>  	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>  						    max_size_ep));
> 
> With this version my Win10 does not enumerate the USB AUDIO device, even if
> it has only EP-IN capability (i.e. is_playback = true). For my setup the
> EP-IN wMaxPacketSize is 192bytes vs. 196bytes in Ruslan's version, causing
> win10 reporting "The specified range could not be found in the range list."
> 
> A simple change makes Win10 enumerate both playback-only as well as duplex
> playback/capture async device:
> 
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index ae29ff2b2b68..5ba778814796 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -588,7 +588,7 @@ static int set_ep_max_packet_size(const struct
> f_uac2_opts *uac2_opts,
>                 srate = srate * (1000 + uac2_opts->fb_max) / 1000;
> 
>         max_size_bw = num_channels(chmask) * ssize *
> -               DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval -
> 1)));
> +               (DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval -
> 1))) + 1);
>         ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>                                                     max_size_ep));
> 
> 
> I do not know if this is the most correct solution but IMO a similar patch
> should be applied. I can send a proper patch mail if this solution is
> acceptable.

Just send the patch and we will go from there.

thanks,

greg k-h