linux-kernel - Re: [PATCH] Intel xhci: Only switch the switchable ports

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADXHx7ZbWPb1qLQcdquE9QTkGhOwR0ka1gX2m+o_Y68c9jqtog@mail.gmail.com>
Date:	Fri, 10 Aug 2012 00:13:19 +0800
From:	Keng-Yu Lin <kengyu@...onical.com>
To:	Sarah Sharp <sarah.a.sharp@...ux.intel.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Intel xhci: Only switch the switchable ports

On Thu, Aug 9, 2012 at 10:24 PM, Sarah Sharp
<sarah.a.sharp@...ux.intel.com> wrote:
> On Thu, Aug 09, 2012 at 05:31:51PM +0800, Keng-Yu Lin wrote:
>> With a previous patch to enable the EHCI/XHCI port switching, it switches
>> all the available ports.
>>
>> The assumption is not correct because the BIOS may expect some ports
>> not switchable by the OS.
>
> Why would the BIOS expect some ports to not be switchable?  I know that
> we internally at Intel had discussed some theoretical reasons why it
> might not be good to switch some ports, but when I presented the
> original patch with this same code in it to Linux USB mailing list, both
> Alan and Greg said, "Why not unconditionally switch ports?"  I had no
> good examples at the time.
>
> Is this causing issues with some particular BIOS?
>

Yes, this is causing the internal webcam missing on the USB bus as I
observed on some HM70-based laptops.

The internal webcam is attached to one port that is controlled by the
xhci host.
But the other ports with the outer plugs work well after booting. I
cannot test the USB port of the internal webcam easily (without
tearing down the laptop :-/).

I also tried some similar HM77-based models. HM77 has no this issue.
This could be some chipset mystery I am not aware now.

Some bisecting led to your original patch.

>> There are two more registers that contains the information of the switchable
>> and non-switchable ports.
>>
>> This patch adds the checking code for the two register so that only the
>> switchable ports are altered.
>>
>> Signed-off-by: Keng-Yu Lin <kengyu@...onical.com>
>> ---
>>  drivers/usb/host/pci-quirks.c |   27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index 833b3c6..89f62f2 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -75,7 +75,9 @@
>>  #define      NB_PIF0_PWRDOWN_1       0x01100013
>>
>>  #define USB_INTEL_XUSB2PR      0xD0
>> +#define USB_INTEL_USB2PRM      0xD4
>>  #define USB_INTEL_USB3_PSSEN   0xD8
>> +#define USB_INTEL_USB3PRM      0xDC
>>
>>  static struct amd_chipset_info {
>>       struct pci_dev  *nb_dev;
>> @@ -772,10 +774,18 @@ void usb_enable_xhci_ports(struct pci_dev *xhci_pdev)
>>               return;
>>       }
>>
>> -     ports_available = 0xffffffff;
>> +     /* Read USB3PRM, the USB 3.0 Port Routing Mask Register
>> +      * Indicate the ports that can be changed from OS.
>> +      */
>> +     pci_read_config_dword(xhci_pdev, USB_INTEL_USB3PRM,
>> +                     &ports_available);
>> +
>> +     dev_dbg(&xhci_pdev->dev, "Configurable ports to enable SuperSpeed: 0x%x\n",
>> +                     ports_available);
>> +
>>       /* Write USB3_PSSEN, the USB 3.0 Port SuperSpeed Enable
>> -      * Register, to turn on SuperSpeed terminations for all
>> -      * available ports.
>> +      * Register, to turn on SuperSpeed terminations for the
>> +      * switchable ports.
>>        */
>>       pci_write_config_dword(xhci_pdev, USB_INTEL_USB3_PSSEN,
>>                       cpu_to_le32(ports_available));
>> @@ -785,7 +795,16 @@ void usb_enable_xhci_ports(struct pci_dev *xhci_pdev)
>>       dev_dbg(&xhci_pdev->dev, "USB 3.0 ports that are now enabled "
>>                       "under xHCI: 0x%x\n", ports_available);
>>
>> -     ports_available = 0xffffffff;
>> +     /* Read XUSB2PRM, xHCI USB 2.0 Port Routing Mask Register
>> +      * Indicate the port to be controlled by the EHCI host.
>
> Your code is correct, but your comment is wrong.  XUSB2PRM is the USB
> 2.0 ports that should be controlled by the xHCI host.
>

Thanks for the explanation.

>> +      */
>> +
>> +     pci_read_config_dword(xhci_pdev, USB_INTEL_USB2PRM,
>> +                     &ports_available);
>> +
>> +     dev_dbg(&xhci_pdev->dev, "Configurable ports to hand over the ECHI host:
>> +                     0x%x\n", ports_available);
>
> Again, this should be "Configurable USB 2.0 ports to hand over to xHCI:"
> Also, don't split strings, it makes it hard to grep for them later.
>

Thanks. I will re-sent a patch with the correct comment.

>> +
>>       /* Write XUSB2PR, the xHC USB 2.0 Port Routing Register, to
>>        * switch the USB 2.0 power and data lines over to the xHCI
>>        * host.
>
> Sarah Sharp

-kengyu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/