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

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

[Pany]

Hi,

I installed fedora 32 into an SD card, with which I booted my Macbook.
It had worked well before, until I upgraded the kernel from 5.8.5 to
5.8.6 [1].

With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
would be stuck at "A start job is running for
/dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
photo of screen [3]).

By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
the journal [4].

This issue has been bisected to commit
53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
race")

If I revert this commit, the kernel 5.8.6 would boot successfully.

Some background:
- The SDXC card slot on my Macbook Pro [5] is managed by USB controller [6]
- About the SD Card Reader, "lsusb -vv" shows:
```
Bus 002 Device 002: ID 05ac:8406 Apple, Inc. Internal Memory Card Reader
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               3.00
  bDeviceClass            0
  bDeviceSubClass         0
  bDeviceProtocol         0
  bMaxPacketSize0         9
  idVendor           0x05ac Apple, Inc.
  idProduct          0x8406 Internal Memory Card Reader
  bcdDevice            8.20
  iManufacturer           3 Apple
  iProduct                4 Card Reader
  iSerial                 5 000000000820
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x002c
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower              896mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass         8 Mass Storage
      bInterfaceSubClass      6 SCSI
      bInterfaceProtocol     80 Bulk-Only
      iInterface              0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               4
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0400  1x 1024 bytes
        bInterval               0
        bMaxBurst               4
Binary Object Store Descriptor:
  bLength                 5
  bDescriptorType        15
  wTotalLength       0x0016
  bNumDeviceCaps          2
  USB 2.0 Extension Device Capability:
    bLength                 7
    bDescriptorType        16
    bDevCapabilityType      2
    bmAttributes   0x00000002
      HIRD Link Power Management (LPM) Supported
  SuperSpeed USB Device Capability:
    bLength                10
    bDescriptorType        16
    bDevCapabilityType      3
    bmAttributes         0x02
      Latency Tolerance Messages (LTM) Supported
    wSpeedsSupported   0x000e
      Device can operate at Full Speed (12Mbps)
      Device can operate at High Speed (480Mbps)
      Device can operate at SuperSpeed (5Gbps)
    bFunctionalitySupport   1
      Lowest fully-functional device speed is Full Speed (12Mbps)
    bU1DevExitLat          10 micro seconds
    bU2DevExitLat        2047 micro seconds
can't get debug descriptor: Resource temporarily unavailable
Device Status:     0x001c
  (Bus Powered)
  U1 Enabled
  U2 Enabled
  Latency Tolerance Messaging (LTM) Enabled
```

I'm looking forward to getting help from you guys. Thanks!

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1878347
[2] https://koji.fedoraproject.org/koji/buildinfo?buildID=1605304
[3] https://bugzilla.redhat.com/attachment.cgi?id=1714633
[4] https://bugzilla.redhat.com/attachment.cgi?id=1717469
[5] https://support.apple.com/kb/SP715?locale=en_US
[6] https://support.apple.com/en-us/HT204384#4
-- 
Regards,
Pany
pany@fedoraproject.org

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

[Alan Stern]

On Sat, Oct 17, 2020 at 04:07:11PM +0000, Pany wrote:
> Hi,
> 
> I installed fedora 32 into an SD card, with which I booted my Macbook.
> It had worked well before, until I upgraded the kernel from 5.8.5 to
> 5.8.6 [1].
> 
> With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
> would be stuck at "A start job is running for
> /dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
> photo of screen [3]).
> 
> By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
> the journal [4].
> 
> This issue has been bisected to commit
> 53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
> race")
> 
> If I revert this commit, the kernel 5.8.6 would boot successfully.

This should have been fixed in 5.8.14 or 5.8.15 (5.8.14 was released on 
the same day that the fix was installed; I'm not sure which came first).
At any rate it certainly should work with the most recent 5.8.y kernel.

Alan Stern

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

[Pany]

On Sat, Oct 17, 2020 at 8:02 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sat, Oct 17, 2020 at 04:07:11PM +0000, Pany wrote:
> > Hi,
> >
> > I installed fedora 32 into an SD card, with which I booted my Macbook.
> > It had worked well before, until I upgraded the kernel from 5.8.5 to
> > 5.8.6 [1].
> >
> > With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
> > would be stuck at "A start job is running for
> > /dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
> > photo of screen [3]).
> >
> > By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
> > the journal [4].
> >
> > This issue has been bisected to commit
> > 53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
> > race")
> >
> > If I revert this commit, the kernel 5.8.6 would boot successfully.
>
> This should have been fixed in 5.8.14 or 5.8.15 (5.8.14 was released on
> the same day that the fix was installed; I'm not sure which came first).
> At any rate it certainly should work with the most recent 5.8.y kernel.
>
> Alan Stern

I tried, but neither 5.8.14 nor 5.8.15 worked for me.

-- 
Regards,
Pany
pany@fedoraproject.org

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

[Alan Stern]

On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
> On Sat, Oct 17, 2020 at 8:02 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Sat, Oct 17, 2020 at 04:07:11PM +0000, Pany wrote:
> > > Hi,
> > >
> > > I installed fedora 32 into an SD card, with which I booted my Macbook.
> > > It had worked well before, until I upgraded the kernel from 5.8.5 to
> > > 5.8.6 [1].
> > >
> > > With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
> > > would be stuck at "A start job is running for
> > > /dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
> > > photo of screen [3]).
> > >
> > > By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
> > > the journal [4].
> > >
> > > This issue has been bisected to commit
> > > 53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
> > > race")
> > >
> > > If I revert this commit, the kernel 5.8.6 would boot successfully.
> >
> > This should have been fixed in 5.8.14 or 5.8.15 (5.8.14 was released on
> > the same day that the fix was installed; I'm not sure which came first).
> > At any rate it certainly should work with the most recent 5.8.y kernel.
> >
> > Alan Stern
> 
> I tried, but neither 5.8.14 nor 5.8.15 worked for me.

Hmmm.  Looking at the system log you captured, it appears that the 
problem the SD card reader's driver getting reprobed incorrectly.  The 
details aren't clear, but the log shows the device getting probed twice, 
once as sdb and once as sdc.  If the system tried to mount one of the 
sdb partitions as the root, and then sdb disappeared, that could explain 
what you see.

I don't know why this is happening.  But you can try adding some 
debugging messages of your own.  In drivers/usb/core/driver.c, the 
routine __usb_bus_reprobe_drivers() should never reach the line that 
calls device_reprobe() unless the USBIP or apple-mfi-fastcharge driver 
is installed -- and neither of those should be involved with the card 
reader device.  You can add a line saying:

	dev_info(dev, "new driver %s\n", new_udriver->name);

at that point and see what it produces in the log.

Alan Stern

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

[M. Vefa Bicakci]

On 19/10/2020 13.40, Alan Stern wrote:
> On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
>> On Sat, Oct 17, 2020 at 8:02 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>>>
>>> On Sat, Oct 17, 2020 at 04:07:11PM +0000, Pany wrote:
>>>> Hi,
>>>>
>>>> I installed fedora 32 into an SD card, with which I booted my Macbook.
>>>> It had worked well before, until I upgraded the kernel from 5.8.5 to
>>>> 5.8.6 [1].
>>>>
>>>> With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
>>>> would be stuck at "A start job is running for
>>>> /dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
>>>> photo of screen [3]).
>>>>
>>>> By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
>>>> the journal [4].
>>>>
>>>> This issue has been bisected to commit
>>>> 53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
>>>> race")
>>>>
>>>> If I revert this commit, the kernel 5.8.6 would boot successfully.
>>>
>>> This should have been fixed in 5.8.14 or 5.8.15 (5.8.14 was released on
>>> the same day that the fix was installed; I'm not sure which came first).
>>> At any rate it certainly should work with the most recent 5.8.y kernel.
>>>
>>> Alan Stern
>>
>> I tried, but neither 5.8.14 nor 5.8.15 worked for me.
> 
> Hmmm.  Looking at the system log you captured, it appears that the
> problem the SD card reader's driver getting reprobed incorrectly.  The
> details aren't clear, but the log shows the device getting probed twice,
> once as sdb and once as sdc.  If the system tried to mount one of the
> sdb partitions as the root, and then sdb disappeared, that could explain
> what you see.
> 
> I don't know why this is happening.  But you can try adding some
> debugging messages of your own.  In drivers/usb/core/driver.c, the
> routine __usb_bus_reprobe_drivers() should never reach the line that
> calls device_reprobe() unless the USBIP or apple-mfi-fastcharge driver
> is installed -- and neither of those should be involved with the card
> reader device.  You can add a line saying:
> 
> 	dev_info(dev, "new driver %s\n", new_udriver->name);
> 
> at that point and see what it produces in the log.

Hello all,

Sorry for my lateness!

I reviewed Pany's logs, and the issue appears to be related to the
automatic loading of the apple-mfi-fastcharge USB driver, which triggers
a re-probe of the SD card reader, as pointed out earlier.

This happens because the id_table of the aforementioned USB driver
(mfi_fc_id_table) matches all USB products manufactured by Apple:

  35 static const struct usb_device_id mfi_fc_id_table[] = {
  36         { .idVendor = APPLE_VENDOR_ID,
  37           .match_flags = USB_DEVICE_ID_MATCH_VENDOR },
  38         {},
  39 };
  ...
218 static struct usb_device_driver mfi_fc_driver = {
219         .name =         "apple-mfi-fastcharge",
220         .probe =        mfi_fc_probe,
221         .disconnect =   mfi_fc_disconnect,
222         .id_table =     mfi_fc_id_table,
223         .generic_subclass = 1,
224 };


... and Pany's system has multiple USB devices manufactured by Apple
(including the SD card reader), with the vendor code 0x05ac, which is
the value included by the id_table of the apple-mfi-fastcharge driver:

Sep 29 15:22:48 fedora.local kernel: usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device found, idVendor=05ac, idProduct=8406, bcdDevice= 8.20
Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device strings: Mfr=3, Product=4, SerialNumber=5
Sep 29 15:22:48 fedora.local kernel: usb 2-3: Product: Card Reader
Sep 29 15:22:48 fedora.local kernel: usb 2-3: Manufacturer: Apple
...
Sep 29 15:22:48 fedora.local kernel: usb 1-5: new full-speed USB device number 3 using xhci_hcd
Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device found, idVendor=05ac, idProduct=0273, bcdDevice= 6.22
Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device strings: Mfr=1, Product=2, SerialNumber=3
Sep 29 15:22:48 fedora.local kernel: usb 1-5: Product: Apple Internal Keyboard / Trackpad
Sep 29 15:22:48 fedora.local kernel: usb 1-5: Manufacturer: Apple Inc.
...

One way to fix this issue would be to implement a match function in the
apple-mfi-fastcharge driver, possibly instead of an id_table, so that it
does not match devices that it cannot bind to. This may require other
changes in the USB core that I cannot fathom at the moment.

Pany, in the mean-time, could you add the following string to your kernel's
command line (i.e., via GRUB, or the actual boot-loader that you use) and
let us know whether it helps to *work around* this issue with the latest
versions of 5.8.y kernels?

	module_blacklist=apple-mfi-fastcharge

To emphasize, I am only suggesting this as a work-around, not as an actual
solution.

My time is a bit limited due to having restarted full-time employment,
but I can work on this issue during the weekend.

Thanks!

Vefa

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

[Alan Stern]

On Tue, Oct 20, 2020 at 08:03:23AM -0400, M. Vefa Bicakci wrote:
> On 19/10/2020 13.40, Alan Stern wrote:
> > On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
> > > On Sat, Oct 17, 2020 at 8:02 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > 
> > > > On Sat, Oct 17, 2020 at 04:07:11PM +0000, Pany wrote:
> > > > > Hi,
> > > > > 
> > > > > I installed fedora 32 into an SD card, with which I booted my Macbook.
> > > > > It had worked well before, until I upgraded the kernel from 5.8.5 to
> > > > > 5.8.6 [1].
> > > > > 
> > > > > With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
> > > > > would be stuck at "A start job is running for
> > > > > /dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
> > > > > photo of screen [3]).
> > > > > 
> > > > > By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
> > > > > the journal [4].
> > > > > 
> > > > > This issue has been bisected to commit
> > > > > 53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
> > > > > race")
> > > > > 
> > > > > If I revert this commit, the kernel 5.8.6 would boot successfully.
> > > > 
> > > > This should have been fixed in 5.8.14 or 5.8.15 (5.8.14 was released on
> > > > the same day that the fix was installed; I'm not sure which came first).
> > > > At any rate it certainly should work with the most recent 5.8.y kernel.
> > > > 
> > > > Alan Stern
> > > 
> > > I tried, but neither 5.8.14 nor 5.8.15 worked for me.
> > 
> > Hmmm.  Looking at the system log you captured, it appears that the
> > problem the SD card reader's driver getting reprobed incorrectly.  The
> > details aren't clear, but the log shows the device getting probed twice,
> > once as sdb and once as sdc.  If the system tried to mount one of the
> > sdb partitions as the root, and then sdb disappeared, that could explain
> > what you see.
> > 
> > I don't know why this is happening.  But you can try adding some
> > debugging messages of your own.  In drivers/usb/core/driver.c, the
> > routine __usb_bus_reprobe_drivers() should never reach the line that
> > calls device_reprobe() unless the USBIP or apple-mfi-fastcharge driver
> > is installed -- and neither of those should be involved with the card
> > reader device.  You can add a line saying:
> > 
> > 	dev_info(dev, "new driver %s\n", new_udriver->name);
> > 
> > at that point and see what it produces in the log.
> 
> Hello all,
> 
> Sorry for my lateness!
> 
> I reviewed Pany's logs, and the issue appears to be related to the
> automatic loading of the apple-mfi-fastcharge USB driver, which triggers
> a re-probe of the SD card reader, as pointed out earlier.
> 
> This happens because the id_table of the aforementioned USB driver
> (mfi_fc_id_table) matches all USB products manufactured by Apple:
> 
>  35 static const struct usb_device_id mfi_fc_id_table[] = {
>  36         { .idVendor = APPLE_VENDOR_ID,
>  37           .match_flags = USB_DEVICE_ID_MATCH_VENDOR },
>  38         {},
>  39 };
>  ...
> 218 static struct usb_device_driver mfi_fc_driver = {
> 219         .name =         "apple-mfi-fastcharge",
> 220         .probe =        mfi_fc_probe,
> 221         .disconnect =   mfi_fc_disconnect,
> 222         .id_table =     mfi_fc_id_table,
> 223         .generic_subclass = 1,
> 224 };
> 
> 
> ... and Pany's system has multiple USB devices manufactured by Apple
> (including the SD card reader), with the vendor code 0x05ac, which is
> the value included by the id_table of the apple-mfi-fastcharge driver:
> 
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device found, idVendor=05ac, idProduct=8406, bcdDevice= 8.20
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device strings: Mfr=3, Product=4, SerialNumber=5
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: Product: Card Reader
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: Manufacturer: Apple
> ...
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: new full-speed USB device number 3 using xhci_hcd
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device found, idVendor=05ac, idProduct=0273, bcdDevice= 6.22
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: Product: Apple Internal Keyboard / Trackpad
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: Manufacturer: Apple Inc.
> ...
> 
> One way to fix this issue would be to implement a match function in the
> apple-mfi-fastcharge driver, possibly instead of an id_table, so that it
> does not match devices that it cannot bind to. This may require other
> changes in the USB core that I cannot fathom at the moment.

How about matching on the vendor ID and the product ID?  That would be 
an easy addition to the ID table.  Do the fastcharge devices have a well 
known product ID?

Alan Stern

> Pany, in the mean-time, could you add the following string to your kernel's
> command line (i.e., via GRUB, or the actual boot-loader that you use) and
> let us know whether it helps to *work around* this issue with the latest
> versions of 5.8.y kernels?
> 
> 	module_blacklist=apple-mfi-fastcharge
> 
> To emphasize, I am only suggesting this as a work-around, not as an actual
> solution.
> 
> My time is a bit limited due to having restarted full-time employment,
> but I can work on this issue during the weekend.
> 
> Thanks!
> 
> Vefa

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

[Pany]

Thank you so much for guiding me!

On Tue, Oct 20, 2020 at 12:04 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
>
> On 19/10/2020 13.40, Alan Stern wrote:
> > On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
> >> On Sat, Oct 17, 2020 at 8:02 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >>>
> >>> On Sat, Oct 17, 2020 at 04:07:11PM +0000, Pany wrote:
> >>>> Hi,
> >>>>
> >>>> I installed fedora 32 into an SD card, with which I booted my Macbook.
> >>>> It had worked well before, until I upgraded the kernel from 5.8.5 to
> >>>> 5.8.6 [1].
> >>>>
> >>>> With kernel-5.8.6-200.fc32.x86_64.rpm [2] installed, the boot process
> >>>> would be stuck at "A start job is running for
> >>>> /dev/mapper/fedora_localhost_--live-home (<time> / no limit)" (See the
> >>>> photo of screen [3]).
> >>>>
> >>>> By appending "systemd.debug-shell=1" to the kernel cmdline, I saved
> >>>> the journal [4].
> >>>>
> >>>> This issue has been bisected to commit
> >>>> 53965c79c2dbdc898ffc7fdbab37e920594f5e14 ("USB: Fix device driver
> >>>> race")
> >>>>
> >>>> If I revert this commit, the kernel 5.8.6 would boot successfully.
> >>>
> >>> This should have been fixed in 5.8.14 or 5.8.15 (5.8.14 was released on
> >>> the same day that the fix was installed; I'm not sure which came first).
> >>> At any rate it certainly should work with the most recent 5.8.y kernel.
> >>>
> >>> Alan Stern
> >>
> >> I tried, but neither 5.8.14 nor 5.8.15 worked for me.
> >
> > Hmmm.  Looking at the system log you captured, it appears that the
> > problem the SD card reader's driver getting reprobed incorrectly.  The
> > details aren't clear, but the log shows the device getting probed twice,
> > once as sdb and once as sdc.  If the system tried to mount one of the
> > sdb partitions as the root, and then sdb disappeared, that could explain
> > what you see.
> >
> > I don't know why this is happening.  But you can try adding some
> > debugging messages of your own.  In drivers/usb/core/driver.c, the
> > routine __usb_bus_reprobe_drivers() should never reach the line that
> > calls device_reprobe() unless the USBIP or apple-mfi-fastcharge driver
> > is installed -- and neither of those should be involved with the card
> > reader device.  You can add a line saying:
> >
> >       dev_info(dev, "new driver %s\n", new_udriver->name);
> >
> > at that point and see what it produces in the log.

I added a line and made a patch[1], but I had no idea whether
dev_info() had been inserted to the right place (I'm not good at C
language).

Then I compiled the kernel with build id `usbdebug` on copr[2] (a
build system provided by Fedora), here is the build log[3].
With following kernel cmdline, I captured the journal for
kernel-5.8.15-201.usbdebug.fc32 [4]:

kernel: Command line:
BOOT_IMAGE=(hd5,gpt3)/vmlinuz-5.8.15-201.usbdebug.fc32.x86_64
root=/dev/mapper/fedora_localhost--live-root ro
resume=/dev/mapper/fedora_localhost--live-swap
rd.lvm.lv=fedora_localhost-live/root
rd.luks.uuid=luks-65d9ed28-ea08-4ea5-a1dd-7b2b086f5e09
rd.lvm.lv=fedora_localhost-live/swap
module-blacklist=apple-mfi-fastcharge systemd.debug-shell=1

>
> Hello all,
>
> Sorry for my lateness!
>
> I reviewed Pany's logs, and the issue appears to be related to the
> automatic loading of the apple-mfi-fastcharge USB driver, which triggers
> a re-probe of the SD card reader, as pointed out earlier.
>
> This happens because the id_table of the aforementioned USB driver
> (mfi_fc_id_table) matches all USB products manufactured by Apple:
>
>   35 static const struct usb_device_id mfi_fc_id_table[] = {
>   36         { .idVendor = APPLE_VENDOR_ID,
>   37           .match_flags = USB_DEVICE_ID_MATCH_VENDOR },
>   38         {},
>   39 };
>   ...
> 218 static struct usb_device_driver mfi_fc_driver = {
> 219         .name =         "apple-mfi-fastcharge",
> 220         .probe =        mfi_fc_probe,
> 221         .disconnect =   mfi_fc_disconnect,
> 222         .id_table =     mfi_fc_id_table,
> 223         .generic_subclass = 1,
> 224 };
>
>
> ... and Pany's system has multiple USB devices manufactured by Apple
> (including the SD card reader), with the vendor code 0x05ac, which is
> the value included by the id_table of the apple-mfi-fastcharge driver:
>
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device found, idVendor=05ac, idProduct=8406, bcdDevice= 8.20
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device strings: Mfr=3, Product=4, SerialNumber=5
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: Product: Card Reader
> Sep 29 15:22:48 fedora.local kernel: usb 2-3: Manufacturer: Apple
> ...
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: new full-speed USB device number 3 using xhci_hcd
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device found, idVendor=05ac, idProduct=0273, bcdDevice= 6.22
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: Product: Apple Internal Keyboard / Trackpad
> Sep 29 15:22:48 fedora.local kernel: usb 1-5: Manufacturer: Apple Inc.
> ...
>
> One way to fix this issue would be to implement a match function in the
> apple-mfi-fastcharge driver, possibly instead of an id_table, so that it
> does not match devices that it cannot bind to. This may require other
> changes in the USB core that I cannot fathom at the moment.
>
> Pany, in the mean-time, could you add the following string to your kernel's
> command line (i.e., via GRUB, or the actual boot-loader that you use) and
> let us know whether it helps to *work around* this issue with the latest
> versions of 5.8.y kernels?
>
>         module_blacklist=apple-mfi-fastcharge

And I also installed the official built kernel-5.8.15-201.fc32.x86_64.rpm [5].

Adding module_blacklist=apple-mfi-fastcharge to the GRUB entry did not
succeed in the kernel booting.

With following kernel cmdline, I captured the journal [6]:

kernel: Command line:
BOOT_IMAGE=(hd5,gpt3)/vmlinuz-5.8.15-201.fc32.x86_64
root=/dev/mapper/fedora_localhost--live-root ro
resume=/dev/mapper/fedora_localhost--live-swap
rd.lvm.lv=fedora_localhost-live/root
rd.luks.uuid=luks-65d9ed28-ea08-4ea5-a1dd-7b2b086f5e09
rd.lvm.lv=fedora_localhost-live/swap
module_blacklist=apple-mfi-fastcharge systemd.debug-shell=1

>
> To emphasize, I am only suggesting this as a work-around, not as an actual
> solution.
>
> My time is a bit limited due to having restarted full-time employment,
> but I can work on this issue during the weekend.
>
> Thanks!
>
> Vefa

[1] https://pany.fedorapeople.org/0001-usb-driver-debug.patch
[2] https://copr.fedorainfracloud.org/coprs/pany/kernel-macbook/build/1713250/
[3] https://download.copr.fedorainfracloud.org/results/pany/kernel-macbook/fedora-32-x86_64/01713250-kernel/builder-live.log.gz
[4] https://pany.fedorapeople.org/kernel-5.8.15-module_blacklist=apple-mfi-fastcharge-dev_info.txt
[5] https://koji.fedoraproject.org/koji/buildinfo?buildID=1625939
[6] https://pany.fedorapeople.org/kernel-5.8.15-module_blacklist=apple-mfi-fastcharge.txt

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

[M. Vefa Bicakci]

On 20/10/2020 11.28, Alan Stern wrote:
> On Tue, Oct 20, 2020 at 08:03:23AM -0400, M. Vefa Bicakci wrote:
>> On 19/10/2020 13.40, Alan Stern wrote:
>>> On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
[Snipped by Vefa]
>>
>> ... and Pany's system has multiple USB devices manufactured by Apple
>> (including the SD card reader), with the vendor code 0x05ac, which is
>> the value included by the id_table of the apple-mfi-fastcharge driver:
>>
>> Sep 29 15:22:48 fedora.local kernel: usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
>> Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device found, idVendor=05ac, idProduct=8406, bcdDevice= 8.20
>> Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device strings: Mfr=3, Product=4, SerialNumber=5
>> Sep 29 15:22:48 fedora.local kernel: usb 2-3: Product: Card Reader
>> Sep 29 15:22:48 fedora.local kernel: usb 2-3: Manufacturer: Apple
>> ...
>> Sep 29 15:22:48 fedora.local kernel: usb 1-5: new full-speed USB device number 3 using xhci_hcd
>> Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device found, idVendor=05ac, idProduct=0273, bcdDevice= 6.22
>> Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>> Sep 29 15:22:48 fedora.local kernel: usb 1-5: Product: Apple Internal Keyboard / Trackpad
>> Sep 29 15:22:48 fedora.local kernel: usb 1-5: Manufacturer: Apple Inc.
>> ...
>>
>> One way to fix this issue would be to implement a match function in the
>> apple-mfi-fastcharge driver, possibly instead of an id_table, so that it
>> does not match devices that it cannot bind to. This may require other
>> changes in the USB core that I cannot fathom at the moment.
> 
> How about matching on the vendor ID and the product ID?  That would be
> an easy addition to the ID table.  Do the fastcharge devices have a well
> known product ID?
> 
> Alan Stern

Hello Alan,

Thank you for the feedback! Judging from the driver's code, it looks like
the product identifiers are known, but there unfortunately appear to be
256 possible product identifiers (i.e., 0x00->0xFF):


  23 /* The product ID is defined as starting with 0x12nn, as per the
  24  * "Choosing an Apple Device USB Configuration" section in
  25  * release R9 (2012) of the "MFi Accessory Hardware Specification"
  26  *
  27  * To distinguish an Apple device, a USB host can check the device
  28  * descriptor of attached USB devices for the following fields:
  29  * - Vendor ID: 0x05AC
  30  * - Product ID: 0x12nn
  31  *
  32  * Those checks will be done in .match() and .probe().
  33  */
...
166 static int mfi_fc_probe(struct usb_device *udev)
167 {
...
171
172         idProduct = le16_to_cpu(udev->descriptor.idProduct);
173         /* See comment above mfi_fc_id_table[] */
174         if (idProduct < 0x1200 || idProduct > 0x12ff) {
175                 return -ENODEV;
176         }

A quick look at drivers/usb/core/driver.c::usb_match_device indicates
that it is not yet possible to specify ranges in ID tables of USB
drivers, so I think that we would need to replace the ID table with
a match function. Interestingly enough, the comment block quoted
above mentions the use of a match function as well.

Thank you,

Vefa

As a reminder to myself, removing the ID table from the apple-mfi-fastcharge
driver and implementing a match function in this driver will likely require
cherry-picking the following commit to 5.8.y:
	0ed9498f9ecf ("USB: Simplify USB ID table match")

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

[M. Vefa Bicakci]

On 20/10/2020 23.17, Pany wrote:
> Thank you so much for guiding me!
> On Tue, Oct 20, 2020 at 12:04 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
[Snipped by Vefa]
>> Pany, in the mean-time, could you add the following string to your kernel's
>> command line (i.e., via GRUB, or the actual boot-loader that you use) and
>> let us know whether it helps to *work around* this issue with the latest
>> versions of 5.8.y kernels?
>>
>>          module_blacklist=apple-mfi-fastcharge
> 
> And I also installed the official built kernel-5.8.15-201.fc32.x86_64.rpm [5].
> 
> Adding module_blacklist=apple-mfi-fastcharge to the GRUB entry did not
> succeed in the kernel booting.
> 
> With following kernel cmdline, I captured the journal [6]:
> 
> kernel: Command line:
> BOOT_IMAGE=(hd5,gpt3)/vmlinuz-5.8.15-201.fc32.x86_64
> root=/dev/mapper/fedora_localhost--live-root ro
> resume=/dev/mapper/fedora_localhost--live-swap
> rd.lvm.lv=fedora_localhost-live/root
> rd.luks.uuid=luks-65d9ed28-ea08-4ea5-a1dd-7b2b086f5e09
> rd.lvm.lv=fedora_localhost-live/swap
> module_blacklist=apple-mfi-fastcharge systemd.debug-shell=1

Hello Pany,

My apologies, the original kernel command line entry I mentioned was
incorrect; the module name needs to be specified with underscore characters
("_") instead of dash ("-") characters. Could you try the following as well?

   module_blacklist=apple_mfi_fastcharge

Using this string in the kernel command line causes "modprobe apple-mfi-fastcharge"
to fail with -EPERM on my system, and hence I am hoping that this should
at least unblock your day-to-day use of recent kernels.

Sorry again for the mistake in my earlier suggestion.

Vefa

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

[Pany]

Thank you Vefa!

Appending module_blacklist=apple_mfi_fastcharge works for me!

Actually I checked the output of "sudo lsmod | grep fastcharge" with
the working kernel, and I should have noticed that module name
apple_mfi_fastcharge, but I missed it.

Thank you for your suggestion again. If you need more information or
debug log, please let me know.

On Wed, Oct 21, 2020 at 4:19 AM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
>
> On 20/10/2020 23.17, Pany wrote:
> > Thank you so much for guiding me!
> > On Tue, Oct 20, 2020 at 12:04 PM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
> [Snipped by Vefa]
> >> Pany, in the mean-time, could you add the following string to your kernel's
> >> command line (i.e., via GRUB, or the actual boot-loader that you use) and
> >> let us know whether it helps to *work around* this issue with the latest
> >> versions of 5.8.y kernels?
> >>
> >>          module_blacklist=apple-mfi-fastcharge
> >
> > And I also installed the official built kernel-5.8.15-201.fc32.x86_64.rpm [5].
> >
> > Adding module_blacklist=apple-mfi-fastcharge to the GRUB entry did not
> > succeed in the kernel booting.
> >
> > With following kernel cmdline, I captured the journal [6]:
> >
> > kernel: Command line:
> > BOOT_IMAGE=(hd5,gpt3)/vmlinuz-5.8.15-201.fc32.x86_64
> > root=/dev/mapper/fedora_localhost--live-root ro
> > resume=/dev/mapper/fedora_localhost--live-swap
> > rd.lvm.lv=fedora_localhost-live/root
> > rd.luks.uuid=luks-65d9ed28-ea08-4ea5-a1dd-7b2b086f5e09
> > rd.lvm.lv=fedora_localhost-live/swap
> > module_blacklist=apple-mfi-fastcharge systemd.debug-shell=1
>
> Hello Pany,
>
> My apologies, the original kernel command line entry I mentioned was
> incorrect; the module name needs to be specified with underscore characters
> ("_") instead of dash ("-") characters. Could you try the following as well?
>
>    module_blacklist=apple_mfi_fastcharge
>
> Using this string in the kernel command line causes "modprobe apple-mfi-fastcharge"
> to fail with -EPERM on my system, and hence I am hoping that this should
> at least unblock your day-to-day use of recent kernels.
>
> Sorry again for the mistake in my earlier suggestion.
>
> Vefa

-- 
Regards,
Pany
pany@fedoraproject.org

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

[Bastien Nocera]

On Wed, 2020-10-21 at 00:18 -0400, M. Vefa Bicakci wrote:
> On 20/10/2020 11.28, Alan Stern wrote:
> > On Tue, Oct 20, 2020 at 08:03:23AM -0400, M. Vefa Bicakci wrote:
> > > On 19/10/2020 13.40, Alan Stern wrote:
> > > > On Mon, Oct 19, 2020 at 09:36:00AM +0000, Pany wrote:
> [Snipped by Vefa]
> > > 
> > > ... and Pany's system has multiple USB devices manufactured by
> > > Apple
> > > (including the SD card reader), with the vendor code 0x05ac,
> > > which is
> > > the value included by the id_table of the apple-mfi-fastcharge
> > > driver:
> > > 
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: new SuperSpeed Gen
> > > 1 USB device number 2 using xhci_hcd
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device
> > > found, idVendor=05ac, idProduct=8406, bcdDevice= 8.20
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: New USB device
> > > strings: Mfr=3, Product=4, SerialNumber=5
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: Product: Card
> > > Reader
> > > Sep 29 15:22:48 fedora.local kernel: usb 2-3: Manufacturer: Apple
> > > ...
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: new full-speed USB
> > > device number 3 using xhci_hcd
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device
> > > found, idVendor=05ac, idProduct=0273, bcdDevice= 6.22
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: New USB device
> > > strings: Mfr=1, Product=2, SerialNumber=3
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: Product: Apple
> > > Internal Keyboard / Trackpad
> > > Sep 29 15:22:48 fedora.local kernel: usb 1-5: Manufacturer: Apple
> > > Inc.
> > > ...
> > > 
> > > One way to fix this issue would be to implement a match function
> > > in the
> > > apple-mfi-fastcharge driver, possibly instead of an id_table, so
> > > that it
> > > does not match devices that it cannot bind to. This may require
> > > other
> > > changes in the USB core that I cannot fathom at the moment.
> > 
> > How about matching on the vendor ID and the product ID?  That would
> > be
> > an easy addition to the ID table.  Do the fastcharge devices have a
> > well
> > known product ID?
> > 
> > Alan Stern
> 
> Hello Alan,
> 
> Thank you for the feedback! Judging from the driver's code, it looks
> like
> the product identifiers are known, but there unfortunately appear to
> be
> 256 possible product identifiers (i.e., 0x00->0xFF):
> 
> 
>   23 /* The product ID is defined as starting with 0x12nn, as per the
>   24  * "Choosing an Apple Device USB Configuration" section in
>   25  * release R9 (2012) of the "MFi Accessory Hardware
> Specification"
>   26  *
>   27  * To distinguish an Apple device, a USB host can check the
> device
>   28  * descriptor of attached USB devices for the following fields:
>   29  * - Vendor ID: 0x05AC
>   30  * - Product ID: 0x12nn
>   31  *
>   32  * Those checks will be done in .match() and .probe().
>   33  */
> ...
> 166 static int mfi_fc_probe(struct usb_device *udev)
> 167 {
> ...
> 171
> 172         idProduct = le16_to_cpu(udev->descriptor.idProduct);
> 173         /* See comment above mfi_fc_id_table[] */
> 174         if (idProduct < 0x1200 || idProduct > 0x12ff) {
> 175                 return -ENODEV;
> 176         }
> 
> A quick look at drivers/usb/core/driver.c::usb_match_device indicates
> that it is not yet possible to specify ranges in ID tables of USB
> drivers, so I think that we would need to replace the ID table with
> a match function. Interestingly enough, the comment block quoted
> above mentions the use of a match function as well.

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

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

[Bastien Nocera]

On Wed, 2020-10-21 at 13:53 +0200, Bastien Nocera wrote:
<snip>
> 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 :/

Something like that (untested):

diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c b/drivers/usb/misc/apple-mfi-fastcharge.c
index b403094a6b3a..bb89dde018b1 100644
--- a/drivers/usb/misc/apple-mfi-fastcharge.c
+++ b/drivers/usb/misc/apple-mfi-fastcharge.c
@@ -163,17 +163,26 @@ 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, idVendor;
+
+       idVendor = le16_to_cpu(udev->descriptor.idVendor);
+       idProduct = le16_to_cpu(udev->descriptor.idProduct);
+       /* See comment above mfi_fc_id_table[] */
+       return (idVendor == APPLE_VENDOR_ID &&
+               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_probe(udev))
                return -ENODEV;
-       }
 
        mfi = kzalloc(sizeof(struct mfi_device), GFP_KERNEL);
        if (!mfi) {
@@ -217,6 +226,7 @@ static void mfi_fc_disconnect(struct usb_device *udev)
 
 static struct usb_device_driver mfi_fc_driver = {
        .name =         "apple-mfi-fastcharge",
+       .match =        mfi_fc_match,
        .probe =        mfi_fc_probe,
        .disconnect =   mfi_fc_disconnect,
        .id_table =     mfi_fc_id_table,

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

[Alan Stern]

On Wed, Oct 21, 2020 at 02:02:55PM +0200, Bastien Nocera wrote:
> On Wed, 2020-10-21 at 13:53 +0200, Bastien Nocera wrote:
> <snip>
> > 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 :/
> 
> Something like that (untested):
> 
> diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c b/drivers/usb/misc/apple-mfi-fastcharge.c
> index b403094a6b3a..bb89dde018b1 100644
> --- a/drivers/usb/misc/apple-mfi-fastcharge.c
> +++ b/drivers/usb/misc/apple-mfi-fastcharge.c
> @@ -163,17 +163,26 @@ 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, idVendor;
> +
> +       idVendor = le16_to_cpu(udev->descriptor.idVendor);
> +       idProduct = le16_to_cpu(udev->descriptor.idProduct);
> +       /* See comment above mfi_fc_id_table[] */
> +       return (idVendor == APPLE_VENDOR_ID &&
> +               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_probe(udev))

That should be mfi_fc_match(udev).

Alan Stern

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

[Bastien Nocera]

On Wed, 2020-10-21 at 08:29 -0400, Alan Stern wrote:
> On Wed, Oct 21, 2020 at 02:02:55PM +0200, Bastien Nocera wrote:
> > On Wed, 2020-10-21 at 13:53 +0200, Bastien Nocera wrote:
> > <snip>
> > > 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 :/
> > 
> > Something like that (untested):
> > 
> > diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c
> > b/drivers/usb/misc/apple-mfi-fastcharge.c
> > index b403094a6b3a..bb89dde018b1 100644
> > --- a/drivers/usb/misc/apple-mfi-fastcharge.c
> > +++ b/drivers/usb/misc/apple-mfi-fastcharge.c
> > @@ -163,17 +163,26 @@ 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, idVendor;
> > +
> > +       idVendor = le16_to_cpu(udev->descriptor.idVendor);
> > +       idProduct = le16_to_cpu(udev->descriptor.idProduct);
> > +       /* See comment above mfi_fc_id_table[] */
> > +       return (idVendor == APPLE_VENDOR_ID &&
> > +               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_probe(udev))
> 
> That should be mfi_fc_match(udev).

*facepalm*

Thanks

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

[Bastien Nocera]

On Wed, 2020-10-21 at 14:02 +0200, Bastien Nocera wrote:
> On Wed, 2020-10-21 at 13:53 +0200, Bastien Nocera wrote:
> <snip>
> > 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 :/
> 
> Something like that (untested):
> 
> diff --git a/drivers/usb/misc/apple-mfi-fastcharge.c
> b/drivers/usb/misc/apple-mfi-fastcharge.c
> index b403094a6b3a..bb89dde018b1 100644
> --- a/drivers/usb/misc/apple-mfi-fastcharge.c
> +++ b/drivers/usb/misc/apple-mfi-fastcharge.c
> @@ -163,17 +163,26 @@ 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, idVendor;
> +
> +       idVendor = le16_to_cpu(udev->descriptor.idVendor);
> +       idProduct = le16_to_cpu(udev->descriptor.idProduct);
> +       /* See comment above mfi_fc_id_table[] */
> +       return (idVendor == APPLE_VENDOR_ID &&
> +               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_probe(udev))

Even with the s/mfi_fc_probe/mfi_fc_match/ fix, I don't think this
patch will work.

The code in drivers/usb/core/driver.c usb_device_match() will look at
the ID table first, and check whether there's a match and exit if there
was, then check for the result of the ->match function.

But apple-mfi-fastcharge.ko will not get automatically loaded if
there's no ID table. And its match function at all won't get called if
there's an ID table.

So we'd need to check for id_table matching *and* match function
matching.

Is my reasoning correct here?

I have a local patch, which I'll test shortly.

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

[M. Vefa Bicakci]

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.

Vefa

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

[Bastien Nocera]

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

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

[Bastien Nocera]

On Wed, 2020-10-21 at 15:18 +0200, Bastien Nocera wrote:
> 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.

There are also "Co-developed-by", and "Suggested-by" tags which you can
use, as per Documentation/process/submitting-patches.rst

Cheers

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

[Alan Stern]

On Wed, Oct 21, 2020 at 03:18:06PM +0200, Bastien Nocera wrote:
> 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

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

Another note: The patch description should include a pointer to Pany's
RedHat Bugzilla bug report.

Alan Stern

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

[M. Vefa Bicakci]

On 21/10/2020 09.18, Bastien Nocera wrote:
> 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

Hello Bastien,

Sorry about my delayed reply. Thank you, and I appreciate your understanding.
I intend to continue the work starting later today, and I will make
sure that you are credited by using the co-developed-by tag in the
patches, as you have mentioned in your more recent e-mail in this thread.

I will investigate/verify the ID table vs. match function aspect
as well; I understand that my suggestion could be incorrect.

Thanks again,

Vefa

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

[M. Vefa Bicakci]

On 21/10/2020 16.11, Alan Stern wrote:
> On Wed, Oct 21, 2020 at 03:18:06PM +0200, Bastien Nocera wrote:
>> 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
> 
>>  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>
>> ---
> 
> Another note: The patch description should include a pointer to Pany's
> RedHat Bugzilla bug report.
> 
> Alan Stern

Hello Alan,

Thank you for the feedback. I will make sure that a link to the bug report
is in the patch description.

Vefa

[PATCH 0/2] Patches to prevent re-probing all Apple USB devices on apple-mfi-fastcharge load

[M. Vefa Bicakci]

Hello all,

This thread is intended to continue the thread at [1], where Pany
reported an issue involving the apple-mfi-fastcharge driver. The
bug in question causes a re-probe of all Apple USB devices when the
driver in question is loaded, including an Apple SD card reader,
which in turn causes a boot-up failure. (The original bug report
is in the RedHat Bugzilla at [2].)

These two patches aim to resolve this issue by implementing a match
function in the apple-mfi-fastcharge driver to ensure that this driver
is only matched with devices it is intended to be used with, and by
modifying the USB core to ensure that when a device driver has an
ID table and a match function, they are both taken into account.
(The previous behaviour did not check the result of the driver's
match() if the driver's id_table matched the device.)

These changes have been verified to not cause regressions on a
5.8.16-based kernel by (1) verifying apple-mfi-fastcharge behaviour
with an iPhone, (2) running the usbip regression test to ensure that
the changes in USB core are harmless to usbip, and (3) using the
patched kernel with other USB devices (a keyboard, a mouse and a
webcam). Greg Kroah-Hartman's usb-next tree has been verified to
compile with these patches and was briefly runtime tested as well.
(The base commit in usb-next is listed below.)

Credits: The first patch was co-developed by Bastien Nocera and
myself, with the main idea (i.e., the use of id_table *and* the
match function) coming from Bastien. The second patch was
developed solely by Bastien, and I only added a few tags to the
patch description.

Bastien, sorry for the delay!

Thank you,

Vefa

[1] https://lore.kernel.org/linux-usb/CAE3RAxt0WhBEz8zkHrVO5RiyEOasayy1QUAjsv-pB0fAbY1GSw@mail.gmail.com/
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1878347

Cc: Bastien Nocera <hadess@hadess.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Pany <pany@fedoraproject.org>

Bastien Nocera (2):
  usbcore: Check both id_table and match() when both available
  USB: apple-mfi-fastcharge: don't probe unhandled devices

 drivers/usb/core/driver.c               | 30 +++++++++++++++++--------
 drivers/usb/core/generic.c              |  4 +---
 drivers/usb/core/usb.h                  |  2 ++
 drivers/usb/misc/apple-mfi-fastcharge.c | 17 +++++++++-----
 4 files changed, 36 insertions(+), 17 deletions(-)


base-commit: 270315b8235e3d10c2e360cff56c2f9e0915a252
-- 
2.26.2

[PATCH 1/2] usbcore: Check both id_table and match() when both available

[M. Vefa Bicakci]

From: Bastien Nocera <hadess@hadess.net>

From: Bastien Nocera <hadess@hadess.net>

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>
Cc: <stable@vger.kernel.org> # 5.8
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Co-developed-by: M. Vefa Bicakci <m.v.b@runbox.com>
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
---
 drivers/usb/core/driver.c  | 30 +++++++++++++++++++++---------
 drivers/usb/core/generic.c |  4 +---
 drivers/usb/core/usb.h     |  2 ++
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 98b7449c11f3..4dfa44d6cc3c 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -839,6 +839,22 @@ const struct usb_device_id *usb_device_match_id(struct usb_device *udev,
 	return NULL;
 }
 
+bool usb_driver_applicable(struct usb_device *udev,
+			   struct usb_device_driver *udrv)
+{
+	if (udrv->id_table && udrv->match)
+		return usb_device_match_id(udev, udrv->id_table) != NULL &&
+		       udrv->match(udev);
+
+	if (udrv->id_table)
+		return usb_device_match_id(udev, udrv->id_table) != NULL;
+
+	if (udrv->match)
+		return udrv->match(udev);
+
+	return false;
+}
+
 static int usb_device_match(struct device *dev, struct device_driver *drv)
 {
 	/* devices and interfaces are handled separately */
@@ -853,17 +869,14 @@ 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->match)
-			return udrv->match(udev);
-
 		/* If the device driver under consideration does not have a
 		 * id_table or a match function, then let the driver's probe
 		 * function decide.
 		 */
-		return 1;
+		if (!udrv->id_table && !udrv->match)
+			return 1;
+
+		return usb_driver_applicable(udev, udrv);
 
 	} else if (is_usb_interface(dev)) {
 		struct usb_interface *intf;
@@ -941,8 +954,7 @@ static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
 		return 0;
 
 	udev = to_usb_device(dev);
-	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
-	    (!new_udriver->match || new_udriver->match(udev) == 0))
+	if (!usb_driver_applicable(udev, new_udriver))
 		return 0;
 
 	ret = device_reprobe(dev);
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 22c887f5c497..26f9fb9f67ca 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -205,9 +205,7 @@ static int __check_for_non_generic_match(struct device_driver *drv, void *data)
 	udrv = to_usb_device_driver(drv);
 	if (udrv == &usb_generic_driver)
 		return 0;
-	if (usb_device_match_id(udev, udrv->id_table) != NULL)
-		return 1;
-	return (udrv->match && udrv->match(udev));
+	return usb_driver_applicable(udev, udrv);
 }
 
 static bool usb_generic_driver_match(struct usb_device *udev)
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index c893f54a3420..82538daac8b8 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -74,6 +74,8 @@ extern int usb_match_device(struct usb_device *dev,
 			    const struct usb_device_id *id);
 extern const struct usb_device_id *usb_device_match_id(struct usb_device *udev,
 				const struct usb_device_id *id);
+extern bool usb_driver_applicable(struct usb_device *udev,
+				  struct usb_device_driver *udrv);
 extern void usb_forced_unbind_intf(struct usb_interface *intf);
 extern void usb_unbind_and_rebind_marked_interfaces(struct usb_device *udev);
 
-- 
2.26.2

[PATCH 2/2] USB: apple-mfi-fastcharge: don't probe unhandled devices

[M. Vefa Bicakci]

From: Bastien Nocera <hadess@hadess.net>

From: Bastien Nocera <hadess@hadess.net>

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>
Reported-by: Pany <pany@fedoraproject.org>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1878347
Link: https://lore.kernel.org/linux-usb/CAE3RAxt0WhBEz8zkHrVO5RiyEOasayy1QUAjsv-pB0fAbY1GSw@mail.gmail.com/
Cc: <stable@vger.kernel.org> # 5.8
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
[m.v.b: Add Link and Reported-by tags to the commit message]
Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
---
 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.26.2

Re: [PATCH 1/2] usbcore: Check both id_table and match() when both available

[M. Vefa Bicakci]

On 22/10/2020 09.55, M. Vefa Bicakci wrote:
> From: Bastien Nocera <hadess@hadess.net>
> 
> From: Bastien Nocera <hadess@hadess.net>

Ah, sorry for this mistake. This is the first time I sent patches
authored by another person, with git-send-email. I should have
tested with my own e-mail address initially.

I will fix this mistake with the next patch set version.

Thank you,

Vefa

Re: [PATCH 1/2] usbcore: Check both id_table and match() when both available

[M. Vefa Bicakci]

On 23/10/2020 03.08, Andy Shevchenko wrote:
> 
> 
> On Thursday, October 22, 2020, M. Vefa Bicakci <m.v.b@runbox.com <mailto:m.v.b@runbox.com>> wrote:
> 
>     On 22/10/2020 09.55, M. Vefa Bicakci wrote:
> 
>         From: Bastien Nocera <hadess@hadess.net <mailto:hadess@hadess.net>>
> 
>         From: Bastien Nocera <hadess@hadess.net <mailto:hadess@hadess.net>>
> 
> 
>     Ah, sorry for this mistake. This is the first time I sent patches
>     authored by another person, with git-send-email. I should have
>     tested with my own e-mail address initially.
> 
>     I will fix this mistake with the next patch set version.
> 
> 
> 
> Also you may use BugLink tag instead of Link.

Thank you, Andy! I will revise the patch description with
the next patch set to use the BugLink tag.

Re: [PATCH 1/2] usbcore: Check both id_table and match() when both available

[Bastien Nocera]

On Thu, 2020-10-22 at 09:55 -0400, M. Vefa Bicakci wrote:
> From: Bastien Nocera <hadess@hadess.net>
> 
> From: Bastien Nocera <hadess@hadess.net>
> 
> 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>
> Cc: <stable@vger.kernel.org> # 5.8
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Co-developed-by: M. Vefa Bicakci <m.v.b@runbox.com>
> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>

You can also add my:
Tested-by: Bastien Nocera <hadess@hadess.net>

Re: [PATCH 2/2] USB: apple-mfi-fastcharge: don't probe unhandled devices

[Bastien Nocera]

On Thu, 2020-10-22 at 09:55 -0400, M. Vefa Bicakci wrote:
> From: Bastien Nocera <hadess@hadess.net>
> 
> From: Bastien Nocera <hadess@hadess.net>
> 
> 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>
> Reported-by: Pany <pany@fedoraproject.org>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1878347
> Link: 
> https://lore.kernel.org/linux-usb/CAE3RAxt0WhBEz8zkHrVO5RiyEOasayy1QUAjsv-pB0fAbY1GSw@mail.gmail.com/
> Cc: <stable@vger.kernel.org> # 5.8
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> [m.v.b: Add Link and Reported-by tags to the commit message]
> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>

And along with the 1/2 patch:
Tested-by: Bastien Nocera <hadess@hadess.net>

Re: [PATCH 1/2] usbcore: Check both id_table and match() when both available

[Pany]

On Tue, Oct 27, 2020 at 6:25 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2020-10-22 at 09:55 -0400, M. Vefa Bicakci wrote:
> > From: Bastien Nocera <hadess@hadess.net>
> >
> > From: Bastien Nocera <hadess@hadess.net>
> >
> > 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>
> > Cc: <stable@vger.kernel.org> # 5.8
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Co-developed-by: M. Vefa Bicakci <m.v.b@runbox.com>
> > Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
>
> You can also add my:
> Tested-by: Bastien Nocera <hadess@hadess.net>
>

This patch works well for me.
Tested-by: Pan (Pany) YUAN <pany@fedoraproject.org>

-- 
Regards,
Pany
pany@fedoraproject.org

Re: [PATCH 2/2] USB: apple-mfi-fastcharge: don't probe unhandled devices

[Pany]

On Tue, Oct 27, 2020 at 2:03 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2020-10-22 at 09:55 -0400, M. Vefa Bicakci wrote:
> > From: Bastien Nocera <hadess@hadess.net>
> >
> > From: Bastien Nocera <hadess@hadess.net>
> >
> > 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>
> > Reported-by: Pany <pany@fedoraproject.org>
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1878347
> > Link:
> > https://lore.kernel.org/linux-usb/CAE3RAxt0WhBEz8zkHrVO5RiyEOasayy1QUAjsv-pB0fAbY1GSw@mail.gmail.com/
> > Cc: <stable@vger.kernel.org> # 5.8
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > [m.v.b: Add Link and Reported-by tags to the commit message]
> > Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
>
> And along with the 1/2 patch:
> Tested-by: Bastien Nocera <hadess@hadess.net>
>

This patch works well for me.
Tested-by: Pan (Pany) YUAN <pany@fedoraproject.org>

-- 
Regards,
Pany
pany@fedoraproject.org

Re: [PATCH 1/2] usbcore: Check both id_table and match() when both available

[M. Vefa Bicakci]

On 28/10/2020 00.00, Pany wrote:
> On Tue, Oct 27, 2020 at 6:25 PM Bastien Nocera <hadess@hadess.net> wrote:
>>
>> On Thu, 2020-10-22 at 09:55 -0400, M. Vefa Bicakci wrote:
>>> From: Bastien Nocera <hadess@hadess.net>
>>>
>>> From: Bastien Nocera <hadess@hadess.net>
>>>
>>> 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>
>>> Cc: <stable@vger.kernel.org> # 5.8
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> Co-developed-by: M. Vefa Bicakci <m.v.b@runbox.com>
>>> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
>>
>> You can also add my:
>> Tested-by: Bastien Nocera <hadess@hadess.net>
>>
> 
> This patch works well for me.
> Tested-by: Pan (Pany) YUAN <pany@fedoraproject.org>

I realize that I am a bit late to do this, but I would like to thank
Bastien and Pany for their efforts in testing (Bastien and Pany) and
co-developing (Bastien) the patches.

Thanks as well to Greg Kroah-Hartman for committing the patches to
the usb-linus branch of the usb git tree and for fixing up the patch
descriptions while doing so.

Thanks everyone!

Vefa

Re: [PATCH 1/2] usbcore: Check both id_table and match() when both available

[Pany]

On Thu, Oct 29, 2020 at 3:34 AM M. Vefa Bicakci <m.v.b@runbox.com> wrote:
>
> On 28/10/2020 00.00, Pany wrote:
> > On Tue, Oct 27, 2020 at 6:25 PM Bastien Nocera <hadess@hadess.net> wrote:
> >>
> >> On Thu, 2020-10-22 at 09:55 -0400, M. Vefa Bicakci wrote:
> >>> From: Bastien Nocera <hadess@hadess.net>
> >>>
> >>> From: Bastien Nocera <hadess@hadess.net>
> >>>
> >>> 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>
> >>> Cc: <stable@vger.kernel.org> # 5.8
> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> Cc: Alan Stern <stern@rowland.harvard.edu>
> >>> Co-developed-by: M. Vefa Bicakci <m.v.b@runbox.com>
> >>> Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
> >>
> >> You can also add my:
> >> Tested-by: Bastien Nocera <hadess@hadess.net>
> >>
> >
> > This patch works well for me.
> > Tested-by: Pan (Pany) YUAN <pany@fedoraproject.org>
>
> I realize that I am a bit late to do this, but I would like to thank
> Bastien and Pany for their efforts in testing (Bastien and Pany) and
> co-developing (Bastien) the patches.
>
> Thanks as well to Greg Kroah-Hartman for committing the patches to
> the usb-linus branch of the usb git tree and for fixing up the patch
> descriptions while doing so.
>
> Thanks everyone!
>
> Vefa

Thanks to Vefa, Bastien, and everyone for all your efforts! The patch
is perfect.

I’m so honored to be part of this. Thanks again.

-- 
Regards,
Pany
pany@fedoraproject.org