[PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Paul Menzel]

Currently, on the AMD board Asus F2A85-M Pro there is a 100 ms delay as
the USB bus of each of the two OHCI PCI devices is reset. As a 50 ms
delay is done per the USB specification.

Commit c6187597 (OHCI: final fix for NVIDIA problems (I hope))
unconditionally does the bus reset for
 all chipsets, while it was only doen for NVIDIA chipsets before.

As it should not be needed for non-NVIDIA chipsets, only do the reset
for Nvidia devices.

Tested on Asus F2A85-M PRO and ASRock E350M1. The USB keyboard works and
the LUKS passphrase can be e
ntered.

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
---
 drivers/usb/host/pci-quirks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 3625a5c1a41b..f6b1a9bbe301 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -784,7 +784,7 @@ static void quirk_usb_handoff_ohci(struct pci_dev *pdev)
 	writel((u32) ~0, base + OHCI_INTRDISABLE);
 
 	/* Reset the USB bus, if the controller isn't already in RESET */
-	if (control & OHCI_HCFS) {
+	if ((pdev->vendor == PCI_VENDOR_ID_NVIDIA) && (control & OHCI_HCFS)) {
 		/* Go into RESET, preserving RWC (and possibly IR) */
 		writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
 		readl(base + OHCI_CONTROL);
-- 
2.17.1

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Alan Stern]

On Sun, 1 Jul 2018, Paul Menzel wrote:

> Currently, on the AMD board Asus F2A85-M Pro there is a 100 ms delay as
> the USB bus of each of the two OHCI PCI devices is reset. As a 50 ms
> delay is done per the USB specification.
> 
> Commit c6187597 (OHCI: final fix for NVIDIA problems (I hope))
> unconditionally does the bus reset for
>  all chipsets, while it was only doen for NVIDIA chipsets before.

I don't follow this at all.  Prior to that commit, the bus reset (i.e.,

	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);

) was performed unconditionally for _all_ controllers.  (However, the
50-ms delay was used only for NVIDIA hardware.)  Following that commit,
the reset is performed for all controllers, but only if the HCFS
bitfield is nonzero.

> As it should not be needed for non-NVIDIA chipsets, only do the reset
> for Nvidia devices.

Therefore this reasoning is wrong.

> Tested on Asus F2A85-M PRO and ASRock E350M1. The USB keyboard works and
> the LUKS passphrase can be e
> ntered.

Unfortunately, there is a wide variety of OHCI controller hardware 
available.  Something that works on one or two controllers might not 
work on another.

Besides, doesn't it seem like a bad idea to reset the controller while 
leaving devices on the USB bus in whatever state they happened to be?

Alan Stern

> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  drivers/usb/host/pci-quirks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 3625a5c1a41b..f6b1a9bbe301 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -784,7 +784,7 @@ static void quirk_usb_handoff_ohci(struct pci_dev *pdev)
>  	writel((u32) ~0, base + OHCI_INTRDISABLE);
>  
>  	/* Reset the USB bus, if the controller isn't already in RESET */
> -	if (control & OHCI_HCFS) {
> +	if ((pdev->vendor == PCI_VENDOR_ID_NVIDIA) && (control & OHCI_HCFS)) {
>  		/* Go into RESET, preserving RWC (and possibly IR) */
>  		writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
>  		readl(base + OHCI_CONTROL);
> 

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Paul Menzel]

[Taking stable@ out of CC. I just started to use `git send-email`.]


Dear Alan,


Am 02.07.2018 um 17:45 schrieb Alan Stern:
> On Sun, 1 Jul 2018, Paul Menzel wrote:
> 
>> Currently, on the AMD board Asus F2A85-M Pro there is a 100 ms delay as
>> the USB bus of each of the two OHCI PCI devices is reset. As a 50 ms
>> delay is done per the USB specification.
>>
>> Commit c6187597 (OHCI: final fix for NVIDIA problems (I hope))
>> unconditionally does the bus reset for
>>   all chipsets, while it was only doen for NVIDIA chipsets before.
> 
> I don't follow this at all.  Prior to that commit, the bus reset (i.e.,
> 
> 	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
> 
> ) was performed unconditionally for _all_ controllers.  (However, the
> 50-ms delay was used only for NVIDIA hardware.)  Following that commit,
> the reset is performed for all controllers, but only if the HCFS
> bitfield is nonzero.
> 
>> As it should not be needed for non-NVIDIA chipsets, only do the reset
>> for Nvidia devices.
> 
> Therefore this reasoning is wrong.

True. Thank you for checking that.

>> Tested on Asus F2A85-M PRO and ASRock E350M1. The USB keyboard works and
>> the LUKS passphrase can be entered.
> 
> Unfortunately, there is a wide variety of OHCI controller hardware
> available.  Something that works on one or two controllers might not
> work on another.

The problem is, that currently 100 ms sleep is over 10 % of the overall
execution time of the Linux kernel here. So I really like to not sleep 
if it’s not needed.

> Besides, doesn't it seem like a bad idea to reset the controller while
> leaving devices on the USB bus in whatever state they happened to be?

Yes, it’s probably not optimal.

I wonder if the reset is needed, if the firmware has already initialized
the device.


Kind regards,

Paul

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Alan Stern]

On Mon, 2 Jul 2018, Paul Menzel wrote:

> [Taking stable@ out of CC. I just started to use `git send-email`.]
> 
> 
> Dear Alan,
> 
> 
> Am 02.07.2018 um 17:45 schrieb Alan Stern:
> > On Sun, 1 Jul 2018, Paul Menzel wrote:
> > 
> >> Currently, on the AMD board Asus F2A85-M Pro there is a 100 ms delay as
> >> the USB bus of each of the two OHCI PCI devices is reset. As a 50 ms
> >> delay is done per the USB specification.
> >>
> >> Commit c6187597 (OHCI: final fix for NVIDIA problems (I hope))
> >> unconditionally does the bus reset for
> >>   all chipsets, while it was only doen for NVIDIA chipsets before.
> > 
> > I don't follow this at all.  Prior to that commit, the bus reset (i.e.,
> > 
> > 	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
> > 
> > ) was performed unconditionally for _all_ controllers.  (However, the
> > 50-ms delay was used only for NVIDIA hardware.)  Following that commit,
> > the reset is performed for all controllers, but only if the HCFS
> > bitfield is nonzero.
> > 
> >> As it should not be needed for non-NVIDIA chipsets, only do the reset
> >> for Nvidia devices.
> > 
> > Therefore this reasoning is wrong.
> 
> True. Thank you for checking that.
> 
> >> Tested on Asus F2A85-M PRO and ASRock E350M1. The USB keyboard works and
> >> the LUKS passphrase can be entered.
> > 
> > Unfortunately, there is a wide variety of OHCI controller hardware
> > available.  Something that works on one or two controllers might not
> > work on another.
> 
> The problem is, that currently 100 ms sleep is over 10 % of the overall
> execution time of the Linux kernel here. So I really like to not sleep 
> if it’s not needed.

It would be nice to execute the probes in parallel; that would reduce 
the total delay to 50 ms.  However, that is the subject of a separate 
discussion.

> > Besides, doesn't it seem like a bad idea to reset the controller while
> > leaving devices on the USB bus in whatever state they happened to be?
> 
> Yes, it’s probably not optimal.
> 
> I wonder if the reset is needed, if the firmware has already initialized
> the device.

The devices on the bus need to be reset, because the OS has no idea of
what they are and what the firmware has them doing.

For example, the firmware may have assigned bus address 2 to a
keyboard.  But the OS can initialize the devices in a different order,
and it might want to assign bus address 2 to a USB drive.  Then you'd
have two devices trying to use the same address at the same time, which
would not be a good thing.

Alan Stern

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Paul Menzel]

Dear Alan,


Am 02.07.2018 um 18:03 schrieb Alan Stern:
> On Mon, 2 Jul 2018, Paul Menzel wrote:

>> Am 02.07.2018 um 17:45 schrieb Alan Stern:
>>> On Sun, 1 Jul 2018, Paul Menzel wrote:
>>>
>>>> Currently, on the AMD board Asus F2A85-M Pro there is a 100 ms delay as
>>>> the USB bus of each of the two OHCI PCI devices is reset. As a 50 ms
>>>> delay is done per the USB specification.
>>>>
>>>> Commit c6187597 (OHCI: final fix for NVIDIA problems (I hope))
>>>> unconditionally does the bus reset for
>>>>    all chipsets, while it was only doen for NVIDIA chipsets before.
>>>
>>> I don't follow this at all.  Prior to that commit, the bus reset (i.e.,
>>>
>>> 	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
>>>
>>> ) was performed unconditionally for _all_ controllers.  (However, the
>>> 50-ms delay was used only for NVIDIA hardware.)  Following that commit,
>>> the reset is performed for all controllers, but only if the HCFS
>>> bitfield is nonzero.
>>>
>>>> As it should not be needed for non-NVIDIA chipsets, only do the reset
>>>> for Nvidia devices.
>>>
>>> Therefore this reasoning is wrong.
>>
>> True. Thank you for checking that.
>>
>>>> Tested on Asus F2A85-M PRO and ASRock E350M1. The USB keyboard works and
>>>> the LUKS passphrase can be entered.
>>>
>>> Unfortunately, there is a wide variety of OHCI controller hardware
>>> available.  Something that works on one or two controllers might not
>>> work on another.
>>
>> The problem is, that currently 100 ms sleep is over 10 % of the overall
>> execution time of the Linux kernel here. So I really like to not sleep
>> if it’s not needed.
> 
> It would be nice to execute the probes in parallel; that would reduce
> the total delay to 50 ms.  However, that is the subject of a separate
> discussion.
> 
>>> Besides, doesn't it seem like a bad idea to reset the controller while
>>> leaving devices on the USB bus in whatever state they happened to be?
>>
>> Yes, it’s probably not optimal.
>>
>> I wonder if the reset is needed, if the firmware has already initialized
>> the device.
> 
> The devices on the bus need to be reset, because the OS has no idea of
> what they are and what the firmware has them doing.
> 
> For example, the firmware may have assigned bus address 2 to a
> keyboard.  But the OS can initialize the devices in a different order,
> and it might want to assign bus address 2 to a USB drive.  Then you'd
> have two devices trying to use the same address at the same time, which
> would not be a good thing.

Understood.

So, what would be a way forward? Add a whitelist for boards or chipsets
not needing the 50 ms delay?


Kind regards,

Paul

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Alan Stern]

On Mon, 2 Jul 2018, Paul Menzel wrote:

> Dear Alan,
> 
> 
> Am 02.07.2018 um 18:03 schrieb Alan Stern:
> > On Mon, 2 Jul 2018, Paul Menzel wrote:
> 
> >> Am 02.07.2018 um 17:45 schrieb Alan Stern:
> >>> On Sun, 1 Jul 2018, Paul Menzel wrote:
> >>>
> >>>> Currently, on the AMD board Asus F2A85-M Pro there is a 100 ms delay as
> >>>> the USB bus of each of the two OHCI PCI devices is reset. As a 50 ms
> >>>> delay is done per the USB specification.
> >>>>
> >>>> Commit c6187597 (OHCI: final fix for NVIDIA problems (I hope))
> >>>> unconditionally does the bus reset for
> >>>>    all chipsets, while it was only doen for NVIDIA chipsets before.
> >>>
> >>> I don't follow this at all.  Prior to that commit, the bus reset (i.e.,
> >>>
> >>> 	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
> >>>
> >>> ) was performed unconditionally for _all_ controllers.  (However, the
> >>> 50-ms delay was used only for NVIDIA hardware.)  Following that commit,
> >>> the reset is performed for all controllers, but only if the HCFS
> >>> bitfield is nonzero.
> >>>
> >>>> As it should not be needed for non-NVIDIA chipsets, only do the reset
> >>>> for Nvidia devices.
> >>>
> >>> Therefore this reasoning is wrong.
> >>
> >> True. Thank you for checking that.
> >>
> >>>> Tested on Asus F2A85-M PRO and ASRock E350M1. The USB keyboard works and
> >>>> the LUKS passphrase can be entered.
> >>>
> >>> Unfortunately, there is a wide variety of OHCI controller hardware
> >>> available.  Something that works on one or two controllers might not
> >>> work on another.
> >>
> >> The problem is, that currently 100 ms sleep is over 10 % of the overall
> >> execution time of the Linux kernel here. So I really like to not sleep
> >> if it’s not needed.
> > 
> > It would be nice to execute the probes in parallel; that would reduce
> > the total delay to 50 ms.  However, that is the subject of a separate
> > discussion.
> > 
> >>> Besides, doesn't it seem like a bad idea to reset the controller while
> >>> leaving devices on the USB bus in whatever state they happened to be?
> >>
> >> Yes, it’s probably not optimal.
> >>
> >> I wonder if the reset is needed, if the firmware has already initialized
> >> the device.
> > 
> > The devices on the bus need to be reset, because the OS has no idea of
> > what they are and what the firmware has them doing.
> > 
> > For example, the firmware may have assigned bus address 2 to a
> > keyboard.  But the OS can initialize the devices in a different order,
> > and it might want to assign bus address 2 to a USB drive.  Then you'd
> > have two devices trying to use the same address at the same time, which
> > would not be a good thing.
> 
> Understood.
> 
> So, what would be a way forward? Add a whitelist for boards or chipsets
> not needing the 50 ms delay?

The 50-ms delay isn't for the board or the chipset; it is for the USB 
devices that are plugged into the controller.  It is required by the 
USB spec, so we can't get rid of it.

Alan Stern

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Alan Stern]

On Mon, 2 Jul 2018, Alan Stern wrote:

> > >> The problem is, that currently 100 ms sleep is over 10 % of the overall
> > >> execution time of the Linux kernel here. So I really like to not sleep
> > >> if it’s not needed.
> > > 
> > > It would be nice to execute the probes in parallel; that would reduce
> > > the total delay to 50 ms.  However, that is the subject of a separate
> > > discussion.
> > > 
> > >>> Besides, doesn't it seem like a bad idea to reset the controller while
> > >>> leaving devices on the USB bus in whatever state they happened to be?
> > >>
> > >> Yes, it’s probably not optimal.
> > >>
> > >> I wonder if the reset is needed, if the firmware has already initialized
> > >> the device.
> > > 
> > > The devices on the bus need to be reset, because the OS has no idea of
> > > what they are and what the firmware has them doing.
> > > 
> > > For example, the firmware may have assigned bus address 2 to a
> > > keyboard.  But the OS can initialize the devices in a different order,
> > > and it might want to assign bus address 2 to a USB drive.  Then you'd
> > > have two devices trying to use the same address at the same time, which
> > > would not be a good thing.
> > 
> > Understood.
> > 
> > So, what would be a way forward? Add a whitelist for boards or chipsets
> > not needing the 50 ms delay?
> 
> The 50-ms delay isn't for the board or the chipset; it is for the USB 
> devices that are plugged into the controller.  It is required by the 
> USB spec, so we can't get rid of it.

I may have been too hasty.  The drivers for other types of USB host
controllers do not perform a USB bus reset during startup, so maybe
OHCI doesn't really need it either.

We still need to put the controller into the correct state, but what 
happens to the bus shouldn't matter.  This is because reinitializing 
the controller disables all its ports, and the only way to enable a 
port is by sending a reset signal through it.  So the attached devices 
will end up getting reset one way or another, even if we don't do it 
explicitly during the handoff.

Therefore in theory we should be okay without the 50-ms delay at all.  
And we might as well tell the controller to go into the USB_RESET even
if it already is in that state; testing the current state is more
effort than just doing setting it.  Something like the patch below
ought to work, although I wouldn't submit it for the -stable kernels
since it doesn't fix a real bug.

It's not clear why OHCI has both a USB_RESET state and a 
HostControllerReset command, especially since the command changes the 
state to USB_SUSPEND instead of USB_RESET.  This is just one of several 
odd things in the OHCI specification.

Alan Stern



Index: usb-4.x/drivers/usb/host/pci-quirks.c
===================================================================
--- usb-4.x.orig/drivers/usb/host/pci-quirks.c
+++ usb-4.x/drivers/usb/host/pci-quirks.c
@@ -783,15 +783,9 @@ static void quirk_usb_handoff_ohci(struc
 	/* disable interrupts */
 	writel((u32) ~0, base + OHCI_INTRDISABLE);
 
-	/* Reset the USB bus, if the controller isn't already in RESET */
-	if (control & OHCI_HCFS) {
-		/* Go into RESET, preserving RWC (and possibly IR) */
-		writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
-		readl(base + OHCI_CONTROL);
-
-		/* drive bus reset for at least 50 ms (7.1.7.5) */
-		msleep(50);
-	}
+	/* Go into the USB_RESET state, preserving RWC (and possibly IR) */
+	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
+	readl(base + OHCI_CONTROL);
 
 	/* software reset of the controller, preserving HcFmInterval */
 	if (!no_fminterval)

Re: [PATCH] usb/host/pci-quirks: Only reset USB bus on NVIDIA devices

[Paul Menzel]

Dear Alan,


Thank you very much for looking into this issue.

Am 02.07.2018 um 19:19 schrieb Alan Stern:
> On Mon, 2 Jul 2018, Alan Stern wrote:
> 
>>>>> The problem is, that currently 100 ms sleep is over 10 % of the overall
>>>>> execution time of the Linux kernel here. So I really like to not sleep
>>>>> if it’s not needed.
>>>>
>>>> It would be nice to execute the probes in parallel; that would reduce
>>>> the total delay to 50 ms.  However, that is the subject of a separate
>>>> discussion.
>>>>
>>>>>> Besides, doesn't it seem like a bad idea to reset the controller while
>>>>>> leaving devices on the USB bus in whatever state they happened to be?
>>>>>
>>>>> Yes, it’s probably not optimal.
>>>>>
>>>>> I wonder if the reset is needed, if the firmware has already initialized
>>>>> the device.
>>>>
>>>> The devices on the bus need to be reset, because the OS has no idea of
>>>> what they are and what the firmware has them doing.
>>>>
>>>> For example, the firmware may have assigned bus address 2 to a
>>>> keyboard.  But the OS can initialize the devices in a different order,
>>>> and it might want to assign bus address 2 to a USB drive.  Then you'd
>>>> have two devices trying to use the same address at the same time, which
>>>> would not be a good thing.
>>>
>>> Understood.
>>>
>>> So, what would be a way forward? Add a whitelist for boards or chipsets
>>> not needing the 50 ms delay?
>>
>> The 50-ms delay isn't for the board or the chipset; it is for the USB
>> devices that are plugged into the controller.  It is required by the
>> USB spec, so we can't get rid of it.
> 
> I may have been too hasty.  The drivers for other types of USB host
> controllers do not perform a USB bus reset during startup, so maybe
> OHCI doesn't really need it either.
> 
> We still need to put the controller into the correct state, but what
> happens to the bus shouldn't matter.  This is because reinitializing
> the controller disables all its ports, and the only way to enable a
> port is by sending a reset signal through it.  So the attached devices
> will end up getting reset one way or another, even if we don't do it
> explicitly during the handoff.
> 
> Therefore in theory we should be okay without the 50-ms delay at all.
> And we might as well tell the controller to go into the USB_RESET even
> if it already is in that state; testing the current state is more
> effort than just doing setting it.  Something like the patch below
> ought to work, although I wouldn't submit it for the -stable kernels
> since it doesn't fix a real bug.
> 
> It's not clear why OHCI has both a USB_RESET state and a
> HostControllerReset command, especially since the command changes the
> state to USB_SUSPEND instead of USB_RESET.  This is just one of several
> odd things in the OHCI specification.

> Index: usb-4.x/drivers/usb/host/pci-quirks.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/host/pci-quirks.c
> +++ usb-4.x/drivers/usb/host/pci-quirks.c
> @@ -783,15 +783,9 @@ static void quirk_usb_handoff_ohci(struc
>   	/* disable interrupts */
>   	writel((u32) ~0, base + OHCI_INTRDISABLE);
>   
> -	/* Reset the USB bus, if the controller isn't already in RESET */
> -	if (control & OHCI_HCFS) {
> -		/* Go into RESET, preserving RWC (and possibly IR) */
> -		writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
> -		readl(base + OHCI_CONTROL);
> -
> -		/* drive bus reset for at least 50 ms (7.1.7.5) */
> -		msleep(50);
> -	}
> +	/* Go into the USB_RESET state, preserving RWC (and possibly IR) */
> +	writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
> +	readl(base + OHCI_CONTROL);
>   
>   	/* software reset of the controller, preserving HcFmInterval */
>   	if (!no_fminterval)

Tested on ASRock E350M1 with GRUB as bootloader, and a mouse attached to 
the USB port. The mouse works under X.


Kind regards,

Paul