Re: [PATCH v2 2/3] usb: core: Allow subclassed USB drivers to override usb_choose_configuration()

[Alan Stern]

On Fri, Dec 01, 2023 at 10:29:51AM -0800, Douglas Anderson wrote:
> For some USB devices we might want to do something different for
> usb_choose_configuration(). One example here is the r8152 driver where
> we want to end up using the vendor driver with the preferred
> interface.
> 
> The r8152 driver tried to make things work by implementing a USB
> generic_subclass driver and then overriding the normal config
> selection after it happened. This is less than ideal and also caused
> breakage if someone deauthorized and re-authorized the USB device
> because the USB core ended up going back to it's default logic for
> choosing the best config. I made an attempt to fix this [1] but it was
> a bit ugly.
> 
> Let's do this better and allow USB generic_subclass drivers to
> override usb_choose_configuration().
> 
> [1] https://lore.kernel.org/r/20231130154337.1.Ie00e07f07f87149c9ce0b27ae4e26991d307e14b@changeid
> 
> Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---

Reviewed-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

> Changes in v2:
> - ("Allow subclassed USB drivers to override ...") new for v2.
> 
>  drivers/usb/core/generic.c | 7 +++++++
>  include/linux/usb.h        | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index 740342a2812a..dcb897158228 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -59,10 +59,17 @@ int usb_choose_configuration(struct usb_device *udev)
>  	int num_configs;
>  	int insufficient_power = 0;
>  	struct usb_host_config *c, *best;
> +	struct usb_device_driver *udriver = to_usb_device_driver(udev->dev.driver);
>  
>  	if (usb_device_is_owned(udev))
>  		return 0;
>  
> +	if (udriver->choose_configuration) {
> +		i = udriver->choose_configuration(udev);
> +		if (i >= 0)
> +			return i;
> +	}
> +
>  	best = NULL;
>  	c = udev->config;
>  	num_configs = udev->descriptor.bNumConfigurations;
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 8c61643acd49..618e5a0b1a22 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1264,6 +1264,9 @@ struct usb_driver {
>   *	module is being unloaded.
>   * @suspend: Called when the device is going to be suspended by the system.
>   * @resume: Called when the device is being resumed by the system.
> + * @choose_configuration: If non-NULL, called instead of the default
> + *	usb_choose_configuration(). If this returns an error then we'll go
> + *	on to call the normal usb_choose_configuration().
>   * @dev_groups: Attributes attached to the device that will be created once it
>   *	is bound to the driver.
>   * @drvwrap: Driver-model core structure wrapper.
> @@ -1287,6 +1290,9 @@ struct usb_device_driver {
>  
>  	int (*suspend) (struct usb_device *udev, pm_message_t message);
>  	int (*resume) (struct usb_device *udev, pm_message_t message);
> +
> +	int (*choose_configuration) (struct usb_device *udev);
> +
>  	const struct attribute_group **dev_groups;
>  	struct usbdrv_wrap drvwrap;
>  	const struct usb_device_id *id_table;
> -- 
> 2.43.0.rc2.451.g8631bc7472-goog
>