[PATCH v2 0/3] net: usb: r8152: Fix lost config across deauthorize+authorize

[PATCH v2 0/3] net: usb: r8152: Fix lost config across deauthorize+authorize

[Douglas Anderson]

This series fixes problems introduced by commit ec51fbd1b8a2 ("r8152:
add USB device driver for config selection") where the r8152 device
would stop functioning if you deauthorized it (by writing 0 to the
"authorized" field in sysfs) and then reauthorized it (by writing a
1).

In v1 this was just a single patch [1], but it's now a 3-patch series
and solves the problem in a much cleaner way, as suggested by Alan
Stern.

Since these three patches straddle the USB subsystem and the
networking subsystem then maintainers will (obviously) need to work
out a way for them to land. I don't have any strong suggestions here
so I'm happy to let the maintainers propose what they think will work
best.

[1] https://lore.kernel.org/r/20231130154337.1.Ie00e07f07f87149c9ce0b27ae4e26991d307e14b@changeid

Changes in v2:
- ("Don't force USB generic_subclass drivers to define ...") new for v2.
- ("Allow subclassed USB drivers to override ...") new for v2.
- ("Choose our USB config with choose_configuration()...) new for v2.

Douglas Anderson (3):
  usb: core: Don't force USB generic_subclass drivers to define probe()
  usb: core: Allow subclassed USB drivers to override
    usb_choose_configuration()
  r8152: Choose our USB config with choose_configuration() rather than
    probe()

 drivers/net/usb/r8152.c    | 16 +++++-----------
 drivers/usb/core/driver.c  |  5 ++++-
 drivers/usb/core/generic.c |  7 +++++++
 include/linux/usb.h        |  6 ++++++
 4 files changed, 22 insertions(+), 12 deletions(-)

-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v2 1/3] usb: core: Don't force USB generic_subclass drivers to define probe()

[Douglas Anderson]

There's no real reason that subclassed USB drivers _need_ to define
probe() since they might want to subclass for some other reason. Make
it optional to define probe() if we're a generic_subclass.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("Don't force USB generic_subclass drivers to define ...") new for v2.

 drivers/usb/core/driver.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f58a0299fb3b..1dc0c0413043 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -290,7 +290,10 @@ static int usb_probe_device(struct device *dev)
 	 * specialised device drivers prior to setting the
 	 * use_generic_driver bit.
 	 */
-	error = udriver->probe(udev);
+	if (udriver->probe)
+		error = udriver->probe(udev);
+	else if (!udriver->generic_subclass)
+		error = -EINVAL;
 	if (error == -ENODEV && udriver != &usb_generic_driver &&
 	    (udriver->id_table || udriver->match)) {
 		udev->use_generic_driver = 1;
-- 
2.43.0.rc2.451.g8631bc7472-goog

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

[Douglas Anderson]

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@rowland.harvard.edu>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

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

[PATCH net v2 3/3] r8152: Choose our USB config with choose_configuration() rather than probe()

[Douglas Anderson]

If you deauthorize the r8152 device (by writing 0 to the "authorized"
field in sysfs) and then reauthorize it (by writing a 1) then it no
longer works. This is because when you do the above we lose the
special configuration that we set in rtl8152_cfgselector_probe().
Deauthorizing causes the config to be set to -1 and then reauthorizing
runs the default logic for choosing the best config.

I made an attempt to fix it so that the config is kept across
deauthorizing / reauthorizing [1] but it was a bit ugly.

Let's instead use the new USB core feature to override
choose_configuration().

This patch relies upon the patches ("usb: core: Don't force USB
generic_subclass drivers to define probe()") and ("usb: core: Allow
subclassed USB drivers to override usb_choose_configuration()")

[1] https://lore.kernel.org/r/20231130154337.1.Ie00e07f07f87149c9ce0b27ae4e26991d307e14b@changeid

Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("Choose our USB config with choose_configuration()...) new for v2.

 drivers/net/usb/r8152.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2c5c1e91ded6..0da723d11326 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -10053,7 +10053,7 @@ static struct usb_driver rtl8152_driver = {
 	.disable_hub_initiated_lpm = 1,
 };
 
-static int rtl8152_cfgselector_probe(struct usb_device *udev)
+static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
 {
 	struct usb_host_config *c;
 	int i, num_configs;
@@ -10080,19 +10080,13 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
 	if (i == num_configs)
 		return -ENODEV;
 
-	if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
-		dev_err(&udev->dev, "Failed to set configuration %d\n",
-			c->desc.bConfigurationValue);
-		return -ENODEV;
-	}
-
-	return 0;
+	return c->desc.bConfigurationValue;
 }
 
 static struct usb_device_driver rtl8152_cfgselector_driver = {
-	.name =		MODULENAME "-cfgselector",
-	.probe =	rtl8152_cfgselector_probe,
-	.id_table =	rtl8152_table,
+	.name =	MODULENAME "-cfgselector",
+	.choose_configuration = rtl8152_cfgselector_choose_configuration,
+	.id_table = rtl8152_table,
 	.generic_subclass = 1,
 	.supports_autosuspend = 1,
 };
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v2 1/3] usb: core: Don't force USB generic_subclass drivers to define probe()

[Alan Stern]

On Fri, Dec 01, 2023 at 10:29:50AM -0800, Douglas Anderson wrote:
> There's no real reason that subclassed USB drivers _need_ to define
> probe() since they might want to subclass for some other reason. Make
> it optional to define probe() if we're a generic_subclass.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

> Changes in v2:
> - ("Don't force USB generic_subclass drivers to define ...") new for v2.
> 
>  drivers/usb/core/driver.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f58a0299fb3b..1dc0c0413043 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -290,7 +290,10 @@ static int usb_probe_device(struct device *dev)
>  	 * specialised device drivers prior to setting the
>  	 * use_generic_driver bit.
>  	 */
> -	error = udriver->probe(udev);
> +	if (udriver->probe)
> +		error = udriver->probe(udev);
> +	else if (!udriver->generic_subclass)
> +		error = -EINVAL;
>  	if (error == -ENODEV && udriver != &usb_generic_driver &&
>  	    (udriver->id_table || udriver->match)) {
>  		udev->use_generic_driver = 1;
> -- 
> 2.43.0.rc2.451.g8631bc7472-goog
> 

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@rowland.harvard.edu>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

> 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
> 

Re: [PATCH v2 1/3] usb: core: Don't force USB generic_subclass drivers to define probe()

[Grant Grundler]

On Fri, Dec 1, 2023 at 10:31 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> There's no real reason that subclassed USB drivers _need_ to define
> probe() since they might want to subclass for some other reason. Make
> it optional to define probe() if we're a generic_subclass.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Grant Grundler <grundler@chromium.org>

Thanks for pursuing this Doug!

cheers,
grant

> ---
>
> Changes in v2:
> - ("Don't force USB generic_subclass drivers to define ...") new for v2.
>
>  drivers/usb/core/driver.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f58a0299fb3b..1dc0c0413043 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -290,7 +290,10 @@ static int usb_probe_device(struct device *dev)
>          * specialised device drivers prior to setting the
>          * use_generic_driver bit.
>          */
> -       error = udriver->probe(udev);
> +       if (udriver->probe)
> +               error = udriver->probe(udev);
> +       else if (!udriver->generic_subclass)
> +               error = -EINVAL;
>         if (error == -ENODEV && udriver != &usb_generic_driver &&
>             (udriver->id_table || udriver->match)) {
>                 udev->use_generic_driver = 1;
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>

Re: [PATCH net v2 3/3] r8152: Choose our USB config with choose_configuration() rather than probe()

[Grant Grundler]

On Fri, Dec 1, 2023 at 10:31 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> If you deauthorize the r8152 device (by writing 0 to the "authorized"
> field in sysfs) and then reauthorize it (by writing a 1) then it no
> longer works. This is because when you do the above we lose the
> special configuration that we set in rtl8152_cfgselector_probe().
> Deauthorizing causes the config to be set to -1 and then reauthorizing
> runs the default logic for choosing the best config.
>
> I made an attempt to fix it so that the config is kept across
> deauthorizing / reauthorizing [1] but it was a bit ugly.
>
> Let's instead use the new USB core feature to override
> choose_configuration().
>
> This patch relies upon the patches ("usb: core: Don't force USB
> generic_subclass drivers to define probe()") and ("usb: core: Allow
> subclassed USB drivers to override usb_choose_configuration()")
>
> [1] https://lore.kernel.org/r/20231130154337.1.Ie00e07f07f87149c9ce0b27ae4e26991d307e14b@changeid
>
> Fixes: ec51fbd1b8a2 ("r8152: add USB device driver for config selection")
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Grant Grundler <grundler@chromium.org>

> ---
>
> Changes in v2:
> - ("Choose our USB config with choose_configuration()...) new for v2.
>
>  drivers/net/usb/r8152.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 2c5c1e91ded6..0da723d11326 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -10053,7 +10053,7 @@ static struct usb_driver rtl8152_driver = {
>         .disable_hub_initiated_lpm = 1,
>  };
>
> -static int rtl8152_cfgselector_probe(struct usb_device *udev)
> +static int rtl8152_cfgselector_choose_configuration(struct usb_device *udev)
>  {
>         struct usb_host_config *c;
>         int i, num_configs;
> @@ -10080,19 +10080,13 @@ static int rtl8152_cfgselector_probe(struct usb_device *udev)
>         if (i == num_configs)
>                 return -ENODEV;
>
> -       if (usb_set_configuration(udev, c->desc.bConfigurationValue)) {
> -               dev_err(&udev->dev, "Failed to set configuration %d\n",
> -                       c->desc.bConfigurationValue);
> -               return -ENODEV;
> -       }
> -
> -       return 0;
> +       return c->desc.bConfigurationValue;
>  }
>
>  static struct usb_device_driver rtl8152_cfgselector_driver = {
> -       .name =         MODULENAME "-cfgselector",
> -       .probe =        rtl8152_cfgselector_probe,
> -       .id_table =     rtl8152_table,
> +       .name = MODULENAME "-cfgselector",
> +       .choose_configuration = rtl8152_cfgselector_choose_configuration,
> +       .id_table = rtl8152_table,
>         .generic_subclass = 1,
>         .supports_autosuspend = 1,
>  };
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>

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

[Bjørn Mork]

Douglas Anderson <dianders@chromium.org> writes:

> 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@rowland.harvard.edu>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Brilliant!  Thanks for doing this.  It is obviously what I should have
done in the first place if I had been smart enough.


Bjørn

Re: [PATCH v2 0/3] net: usb: r8152: Fix lost config across deauthorize+authorize

[Jakub Kicinski]

On Fri,  1 Dec 2023 10:29:49 -0800 Douglas Anderson wrote:
> Since these three patches straddle the USB subsystem and the
> networking subsystem then maintainers will (obviously) need to work
> out a way for them to land. I don't have any strong suggestions here
> so I'm happy to let the maintainers propose what they think will work
> best.

No strong preference here, on a quick read it seems more like a USB
change than networking change, tho, so I'll defer to Greg unless told
otherwise.
-- 
pw-bot: not-applicable

Re: [PATCH net v2 3/3] r8152: Choose our USB config with choose_configuration() rather than probe()

[Jakub Kicinski]

On Fri,  1 Dec 2023 10:29:52 -0800 Douglas Anderson wrote:
> If you deauthorize the r8152 device (by writing 0 to the "authorized"
> field in sysfs) and then reauthorize it (by writing a 1) then it no
> longer works. This is because when you do the above we lose the
> special configuration that we set in rtl8152_cfgselector_probe().
> Deauthorizing causes the config to be set to -1 and then reauthorizing
> runs the default logic for choosing the best config.
> 
> I made an attempt to fix it so that the config is kept across
> deauthorizing / reauthorizing [1] but it was a bit ugly.
> 
> Let's instead use the new USB core feature to override
> choose_configuration().
> 
> This patch relies upon the patches ("usb: core: Don't force USB
> generic_subclass drivers to define probe()") and ("usb: core: Allow
> subclassed USB drivers to override usb_choose_configuration()")

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH v2 0/3] net: usb: r8152: Fix lost config across deauthorize+authorize

[Greg Kroah-Hartman]

On Mon, Dec 04, 2023 at 06:27:27PM -0800, Jakub Kicinski wrote:
> On Fri,  1 Dec 2023 10:29:49 -0800 Douglas Anderson wrote:
> > Since these three patches straddle the USB subsystem and the
> > networking subsystem then maintainers will (obviously) need to work
> > out a way for them to land. I don't have any strong suggestions here
> > so I'm happy to let the maintainers propose what they think will work
> > best.
> 
> No strong preference here, on a quick read it seems more like a USB
> change than networking change, tho, so I'll defer to Greg unless told
> otherwise.

I took these in my tree already, sorry for not saying anything here.

thanks,

greg k-h