Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* Bug caused by 53965c79c2db (USB: Fix device driver race)
@ 2020-10-17 16:07 Pany
  2020-10-17 20:02 ` Alan Stern
  0 siblings, 1 reply; 21+ messages in thread
From: Pany @ 2020-10-17 16:07 UTC (permalink / raw)
  To: linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-17 16:07 Bug caused by 53965c79c2db (USB: Fix device driver race) Pany
@ 2020-10-17 20:02 ` Alan Stern
  2020-10-19  9:36   ` Pany
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Stern @ 2020-10-17 20:02 UTC (permalink / raw)
  To: Pany; +Cc: linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-17 20:02 ` Alan Stern
@ 2020-10-19  9:36   ` Pany
  2020-10-19 17:40     ` Alan Stern
  0 siblings, 1 reply; 21+ messages in thread
From: Pany @ 2020-10-19  9:36 UTC (permalink / raw)
  To: Alan Stern; +Cc: linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-19  9:36   ` Pany
@ 2020-10-19 17:40     ` Alan Stern
  2020-10-20 12:03       ` M. Vefa Bicakci
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Stern @ 2020-10-19 17:40 UTC (permalink / raw)
  To: Pany; +Cc: Bastien Nocera, M. Vefa Bicakci, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-19 17:40     ` Alan Stern
@ 2020-10-20 12:03       ` M. Vefa Bicakci
  2020-10-20 15:28         ` Alan Stern
  2020-10-21  3:17         ` Pany
  0 siblings, 2 replies; 21+ messages in thread
From: M. Vefa Bicakci @ 2020-10-20 12:03 UTC (permalink / raw)
  To: Alan Stern, Pany; +Cc: Bastien Nocera, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-20 12:03       ` M. Vefa Bicakci
@ 2020-10-20 15:28         ` Alan Stern
  2020-10-21  4:18           ` M. Vefa Bicakci
  2020-10-21  3:17         ` Pany
  1 sibling, 1 reply; 21+ messages in thread
From: Alan Stern @ 2020-10-20 15:28 UTC (permalink / raw)
  To: M. Vefa Bicakci; +Cc: Pany, Bastien Nocera, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-20 12:03       ` M. Vefa Bicakci
  2020-10-20 15:28         ` Alan Stern
@ 2020-10-21  3:17         ` Pany
  2020-10-21  4:18           ` M. Vefa Bicakci
  1 sibling, 1 reply; 21+ messages in thread
From: Pany @ 2020-10-21  3:17 UTC (permalink / raw)
  To: M. Vefa Bicakci; +Cc: Alan Stern, Bastien Nocera, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-20 15:28         ` Alan Stern
@ 2020-10-21  4:18           ` M. Vefa Bicakci
  2020-10-21 11:53             ` Bastien Nocera
  0 siblings, 1 reply; 21+ messages in thread
From: M. Vefa Bicakci @ 2020-10-21  4:18 UTC (permalink / raw)
  To: Alan Stern; +Cc: Pany, Bastien Nocera, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21  3:17         ` Pany
@ 2020-10-21  4:18           ` M. Vefa Bicakci
  2020-10-21  5:19             ` Pany
  0 siblings, 1 reply; 21+ messages in thread
From: M. Vefa Bicakci @ 2020-10-21  4:18 UTC (permalink / raw)
  To: Pany; +Cc: Alan Stern, Bastien Nocera, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21  4:18           ` M. Vefa Bicakci
@ 2020-10-21  5:19             ` Pany
  0 siblings, 0 replies; 21+ messages in thread
From: Pany @ 2020-10-21  5:19 UTC (permalink / raw)
  To: M. Vefa Bicakci; +Cc: Alan Stern, Bastien Nocera, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21  4:18           ` M. Vefa Bicakci
@ 2020-10-21 11:53             ` Bastien Nocera
  2020-10-21 12:02               ` Bastien Nocera
  2020-10-21 13:08               ` M. Vefa Bicakci
  0 siblings, 2 replies; 21+ messages in thread
From: Bastien Nocera @ 2020-10-21 11:53 UTC (permalink / raw)
  To: M. Vefa Bicakci, Alan Stern; +Cc: Pany, linux-usb

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 11:53             ` Bastien Nocera
@ 2020-10-21 12:02               ` Bastien Nocera
  2020-10-21 12:29                 ` Alan Stern
  2020-10-21 13:01                 ` Bastien Nocera
  2020-10-21 13:08               ` M. Vefa Bicakci
  1 sibling, 2 replies; 21+ messages in thread
From: Bastien Nocera @ 2020-10-21 12:02 UTC (permalink / raw)
  To: M. Vefa Bicakci, Alan Stern; +Cc: Pany, linux-usb

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,



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 12:02               ` Bastien Nocera
@ 2020-10-21 12:29                 ` Alan Stern
  2020-10-21 12:31                   ` Bastien Nocera
  2020-10-21 13:01                 ` Bastien Nocera
  1 sibling, 1 reply; 21+ messages in thread
From: Alan Stern @ 2020-10-21 12:29 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: M. Vefa Bicakci, Pany, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 12:29                 ` Alan Stern
@ 2020-10-21 12:31                   ` Bastien Nocera
  0 siblings, 0 replies; 21+ messages in thread
From: Bastien Nocera @ 2020-10-21 12:31 UTC (permalink / raw)
  To: Alan Stern; +Cc: M. Vefa Bicakci, Pany, linux-usb

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 12:02               ` Bastien Nocera
  2020-10-21 12:29                 ` Alan Stern
@ 2020-10-21 13:01                 ` Bastien Nocera
  1 sibling, 0 replies; 21+ messages in thread
From: Bastien Nocera @ 2020-10-21 13:01 UTC (permalink / raw)
  To: M. Vefa Bicakci, Alan Stern; +Cc: Pany, linux-usb

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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 11:53             ` Bastien Nocera
  2020-10-21 12:02               ` Bastien Nocera
@ 2020-10-21 13:08               ` M. Vefa Bicakci
  2020-10-21 13:18                 ` Bastien Nocera
  1 sibling, 1 reply; 21+ messages in thread
From: M. Vefa Bicakci @ 2020-10-21 13:08 UTC (permalink / raw)
  To: Bastien Nocera, Alan Stern; +Cc: Pany, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 13:08               ` M. Vefa Bicakci
@ 2020-10-21 13:18                 ` Bastien Nocera
  2020-10-21 13:21                   ` Bastien Nocera
                                     ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Bastien Nocera @ 2020-10-21 13:18 UTC (permalink / raw)
  To: M. Vefa Bicakci, Alan Stern; +Cc: Pany, linux-usb


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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 13:18                 ` Bastien Nocera
@ 2020-10-21 13:21                   ` Bastien Nocera
  2020-10-21 20:11                   ` Alan Stern
  2020-10-21 20:49                   ` M. Vefa Bicakci
  2 siblings, 0 replies; 21+ messages in thread
From: Bastien Nocera @ 2020-10-21 13:21 UTC (permalink / raw)
  To: M. Vefa Bicakci, Alan Stern; +Cc: Pany, linux-usb

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 13:18                 ` Bastien Nocera
  2020-10-21 13:21                   ` Bastien Nocera
@ 2020-10-21 20:11                   ` Alan Stern
  2020-10-21 20:49                     ` M. Vefa Bicakci
  2020-10-21 20:49                   ` M. Vefa Bicakci
  2 siblings, 1 reply; 21+ messages in thread
From: Alan Stern @ 2020-10-21 20:11 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: M. Vefa Bicakci, Pany, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 13:18                 ` Bastien Nocera
  2020-10-21 13:21                   ` Bastien Nocera
  2020-10-21 20:11                   ` Alan Stern
@ 2020-10-21 20:49                   ` M. Vefa Bicakci
  2 siblings, 0 replies; 21+ messages in thread
From: M. Vefa Bicakci @ 2020-10-21 20:49 UTC (permalink / raw)
  To: Bastien Nocera; +Cc: Alan Stern, Pany, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Bug caused by 53965c79c2db (USB: Fix device driver race)
  2020-10-21 20:11                   ` Alan Stern
@ 2020-10-21 20:49                     ` M. Vefa Bicakci
  0 siblings, 0 replies; 21+ messages in thread
From: M. Vefa Bicakci @ 2020-10-21 20:49 UTC (permalink / raw)
  To: Alan Stern; +Cc: Bastien Nocera, Pany, linux-usb

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-17 16:07 Bug caused by 53965c79c2db (USB: Fix device driver race) Pany
2020-10-17 20:02 ` Alan Stern
2020-10-19  9:36   ` Pany
2020-10-19 17:40     ` Alan Stern
2020-10-20 12:03       ` M. Vefa Bicakci
2020-10-20 15:28         ` Alan Stern
2020-10-21  4:18           ` M. Vefa Bicakci
2020-10-21 11:53             ` Bastien Nocera
2020-10-21 12:02               ` Bastien Nocera
2020-10-21 12:29                 ` Alan Stern
2020-10-21 12:31                   ` Bastien Nocera
2020-10-21 13:01                 ` Bastien Nocera
2020-10-21 13:08               ` M. Vefa Bicakci
2020-10-21 13:18                 ` Bastien Nocera
2020-10-21 13:21                   ` Bastien Nocera
2020-10-21 20:11                   ` Alan Stern
2020-10-21 20:49                     ` M. Vefa Bicakci
2020-10-21 20:49                   ` M. Vefa Bicakci
2020-10-21  3:17         ` Pany
2020-10-21  4:18           ` M. Vefa Bicakci
2020-10-21  5:19             ` Pany

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git