Re: Bug caused by 53965c79c2db (USB: Fix device driver race)

[Bastien Nocera <hadess@hadess.net>]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

On Wed, 2020-10-21 at 09:08 -0400, M. Vefa Bicakci wrote:
> On 21/10/2020 07.53, Bastien Nocera wrote:
> [Snipped by Vefa]
> > 
> > I have no idea why there isn't a match function. Patch v1 had a
> > huge
> > table:
> > https://marc.info/?l=linux-usb&m=157062863431186&w=2
> > and v2 already didn't had that comment, but no .match function:
> > https://marc.info/?l=linux-usb&m=157114990905421&w=2
> > 
> > I'll prepare a patch that adds a match function. I'll let you
> > (Vefa)
> > look at which of your patches need backporting though, as I'm
> > really
> > quite a bit lost in the different patch sets and branches :/
> 
> Hello Bastien,
> 
> Having found the root cause of the issue by going through Pany's
> logs and having proposed a solution, I was hoping to get credit
> for the resolution of the issue by authoring the patch.

I don't care either way. Attached are the 2 patches I had made and was
in the process of compiling and testing, feel free to adapt them,
change the authorship, etc.

Note that you mentioned you'd need to "replace the ID table with
a match function", which will mean that the driver isn't automatically
loaded when a device gets plugged in. So that wouldn't have worked.

Let me know when you've made your patch, as I'll need to update this
bug when there's something to test:
https://bugzilla.redhat.com/show_bug.cgi?id=1878347

Cheers

[-- Attachment #2: 0002-USB-apple-mfi-fastcharge-don-t-probe-unhandled-devic.patch --]
[-- Type: text/x-patch, Size: 2334 bytes --]

From 6652af5b46f39d9690d72dc11902b36a44c242a1 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Wed, 21 Oct 2020 14:31:58 +0200
Subject: [PATCH 2/2] USB: apple-mfi-fastcharge: don't probe unhandled devices

Contrary to the comment above the id table, we didn't implement a match
function. This meant that every single Apple device that was already
plugged in to the computer would have its device driver reprobed
when the apple-mfi-fastcharge driver was loaded, eg. the SD card reader
could be reprobed when the apple-mfi-fastcharge after pivoting root
during boot up and the module became available.

Make sure that the driver probe isn't being run for unsupported
devices by adding a match function that checks the product ID, in
addition to the id_table checking the vendor ID.

Fixes: 249fa8217b84 ("USB: Add driver to control USB fast charge for iOS devices")
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/usb/misc/apple-mfi-fastcharge.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c b/drivers/usb/misc/apple-mfi-fastcharge.c
index b403094a6b3a..579d8c84de42 100644
--- a/drivers/usb/misc/apple-mfi-fastcharge.c
+++ b/drivers/usb/misc/apple-mfi-fastcharge.c
@@ -163,17 +163,23 @@ static const struct power_supply_desc apple_mfi_fc_desc = {
 	.property_is_writeable  = apple_mfi_fc_property_is_writeable
 };
 
+static bool mfi_fc_match(struct usb_device *udev)
+{
+	int idProduct;
+
+	idProduct = le16_to_cpu(udev->descriptor.idProduct);
+	/* See comment above mfi_fc_id_table[] */
+	return (idProduct >= 0x1200 && idProduct <= 0x12ff);
+}
+
 static int mfi_fc_probe(struct usb_device *udev)
 {
 	struct power_supply_config battery_cfg = {};
 	struct mfi_device *mfi = NULL;
-	int err, idProduct;
+	int err;
 
-	idProduct = le16_to_cpu(udev->descriptor.idProduct);
-	/* See comment above mfi_fc_id_table[] */
-	if (idProduct < 0x1200 || idProduct > 0x12ff) {
+	if (!mfi_fc_match(udev))
 		return -ENODEV;
-	}
 
 	mfi = kzalloc(sizeof(struct mfi_device), GFP_KERNEL);
 	if (!mfi) {
@@ -220,6 +226,7 @@ static struct usb_device_driver mfi_fc_driver = {
 	.probe =	mfi_fc_probe,
 	.disconnect =	mfi_fc_disconnect,
 	.id_table =	mfi_fc_id_table,
+	.match =	mfi_fc_match,
 	.generic_subclass = 1,
 };
 
-- 
2.28.0


[-- Attachment #3: 0001-usbcore-driver-Check-both-id_table-and-match-when-bo.patch --]
[-- Type: text/x-patch, Size: 1940 bytes --]

From f772bb71891b5090472a744f07dbe268b5d4081b Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Wed, 21 Oct 2020 15:04:33 +0200
Subject: [PATCH 1/2] usbcore/driver: Check both id_table and match() when both
 available

When a USB device driver has both an id_table and a match() function, make
sure to check both to find a match, first matching the id_table, then
checking the match() function.

This makes it possible to have module autoloading done through the
id_table when devices are plugged in, before checking for further
device eligibility in the match() function.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/usb/core/driver.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 98b7449c11f3..575275e72d65 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -845,6 +845,7 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
 	if (is_usb_device(dev)) {
 		struct usb_device *udev;
 		struct usb_device_driver *udrv;
+		bool has_matched_id = false;
 
 		/* interface drivers never match devices */
 		if (!is_usb_device_driver(drv))
@@ -853,8 +854,11 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
 		udev = to_usb_device(dev);
 		udrv = to_usb_device_driver(drv);
 
-		if (udrv->id_table)
-			return usb_device_match_id(udev, udrv->id_table) != NULL;
+		if (udrv->id_table) {
+			if (usb_device_match_id(udev, udrv->id_table) == NULL)
+				return 0;
+			has_matched_id = true;
+		}
 
 		if (udrv->match)
 			return udrv->match(udev);
@@ -863,6 +867,8 @@ static int usb_device_match(struct device *dev, struct device_driver *drv)
 		 * id_table or a match function, then let the driver's probe
 		 * function decide.
 		 */
+		if (has_matched_id)
+			return 0;
 		return 1;
 
 	} else if (is_usb_interface(dev)) {
-- 
2.28.0