| 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
| ||
|
Message-ID: <be81ccf6-d66b-13e7-bbf8-f21c8f4d5dbd@linux.intel.com>
Date: Thu, 18 Jan 2018 17:02:30 +0200
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Tung Vuong Nguyen <tunguyen@....com>
Cc: "Thang Q. Nguyen" <tqnguyen@....com>,
Mathias Nyman <mathias.nyman@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
Phong Vo <pvo@....com>, Loc Ho <lho@....com>,
patches <patches@....com>
Subject: Re: [PATCH v1 1/1] usb: xhci: do not create and register shared_hcd
when USB3.0 is disabled
On 18.01.2018 09:27, Tung Vuong Nguyen wrote:
> On Tue, Jan 16, 2018 at 9:50 PM, Mathias Nyman
> <mathias.nyman@...ux.intel.com> wrote:
>>
>> Hi, Sorry about the delay
>>
>>
>> On 04.01.2018 07:17, Thang Q. Nguyen wrote:
>>>
>>> Hi,
>>>
>>> On Sat, Dec 16, 2017 at 10:45 AM, Thang Q. Nguyen <tqnguyen@....com> wrote:
>>>>
>>>> From: Tung Nguyen <tunguyen@....com>
>>>>
>>>> Currently, hcd->shared_hcd always creates and registers to the usb-core.
>>>> If, for some reasons, USB3 downstream port is disabled, no roothub port for
>>>> USB3.0 is found. This causes kernel to display an error:
>>>> hub 2-0:1.0: config failed, hub doesn't have any ports! (err -19)
>>>> This patch checks, creates and registers shared_hcd if USB3.0 downstream
>>>> port is available.
>>>>
>>>> Signed-off-by: Tung Nguyen <tunguyen@....com>
>>>> Signed-off-by: Thang Q. Nguyen <tqnguyen@....com>
>>>> ---
>>>> drivers/usb/host/xhci-mem.c | 2 +-
>>>> drivers/usb/host/xhci-plat.c | 26 +++++++++++----------
>>>> drivers/usb/host/xhci.c | 54 ++++++++++++++++++++++++++++++++------------
>>>> 3 files changed, 54 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>>> index 554a8a5..157d1e7 100644
>>>> --- a/drivers/usb/host/xhci-mem.c
>>>> +++ b/drivers/usb/host/xhci-mem.c
>>>> @@ -1067,7 +1067,7 @@ static u32 xhci_find_real_port_number(struct xhci_hcd *xhci,
>>>> struct usb_device *top_dev;
>>>> struct usb_hcd *hcd;
>>>>
>>>> - if (udev->speed >= USB_SPEED_SUPER)
>>>> + if (udev->speed >= USB_SPEED_SUPER && xhci->shared_hcd)
>>>> hcd = xhci->shared_hcd;
>>>> else
>>>> hcd = xhci->main_hcd;
>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>>> index 6f03830..e812e3d 100644
>>>> --- a/drivers/usb/host/xhci-plat.c
>>>> +++ b/drivers/usb/host/xhci-plat.c
>>>> @@ -253,12 +253,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>>
>>>> xhci->clk = clk;
>>>> xhci->main_hcd = hcd;
>>>> - xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>>>> - dev_name(&pdev->dev), hcd);
>>>> - if (!xhci->shared_hcd) {
>>>> - ret = -ENOMEM;
>>>> - goto disable_clk;
>>>> - }
>>>>
>>>> if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
>>>> xhci->quirks |= XHCI_HW_LPM_DISABLE;
>>>> @@ -290,12 +284,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>> if (ret)
>>>> goto disable_usb_phy;
>>>>
>>>> - if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>>>> - xhci->shared_hcd->can_do_streams = 1;
>>>> -
>>>> - ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
>>>> - if (ret)
>>>> - goto dealloc_usb2_hcd;
>>>> + if (xhci->num_usb3_ports > 0) {
>>>> + xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>>>> + dev_name(&pdev->dev), hcd);
>>>> + if (!xhci->shared_hcd) {
>>>> + ret = -ENOMEM;
>>>> + goto disable_clk;
>>>> + }
>>>> + if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>>>> + xhci->shared_hcd->can_do_streams = 1;
>>>> +
>>>> + ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
>>>> + if (ret)
>>>> + goto dealloc_usb2_hcd;
>>>> + }
>>>>
>>>> device_enable_async_suspend(&pdev->dev);
>>>> pm_runtime_put_noidle(&pdev->dev);
>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>> index 05104bd..4824bf6 100644
>>>> --- a/drivers/usb/host/xhci.c
>>>> +++ b/drivers/usb/host/xhci.c
>>>> @@ -417,12 +417,14 @@ static void compliance_mode_recovery(struct timer_list *t)
>>>> i + 1);
>>>> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
>>>> "Attempting compliance mode recovery");
>>>> - hcd = xhci->shared_hcd;
>>>> + if (xhci->shared_hcd) {
>>>> + hcd = xhci->shared_hcd;
>>>>
>>>> - if (hcd->state == HC_STATE_SUSPENDED)
>>>> - usb_hcd_resume_root_hub(hcd);
>>>> + if (hcd->state == HC_STATE_SUSPENDED)
>>>> + usb_hcd_resume_root_hub(hcd);
>>>>
>>>> - usb_hcd_poll_rh_status(hcd);
>>>> + usb_hcd_poll_rh_status(hcd);
>>>> + }
>>>> }
>>>> }
>>>>
>>>> @@ -611,6 +613,18 @@ int xhci_run(struct usb_hcd *hcd)
>>>> if (ret)
>>>> xhci_free_command(xhci, command);
>>>> }
>>>> + /*
>>>> + * Execute xhci_start() in case xhci->shared_hcd is not registered.
>>>> + * If the xhci->shared_hcd doesn't exist, no one triggers to start
>>>> + * the xhci which should be done before exitting run function
>>>> + */
>>>> + if (!xhci->shared_hcd) {
>>>> + if (xhci_start(xhci)) {
>>
>>
>> This probably won't work as primary hcd was added before shared_hcd was created.
>> usb_add_hcd(hcd) calls xhci_run() before xhci->shared_hcd exists, so this will
>> cause the xHC to start before the shared_hcd is created or setup.
>>
>> -Mathias
>
> Hi Mathias
> I ran the test and saw the patch works fine in 2 cases:
> xhci->num_usb3_ports = 0 and xhci->num_usb3_ports > 0.
> For the case that we don't have the usb 3.0, the standalone
> primary_hcd should work without the share_hcd. This case the root hubs
> run at speed 480M.
> If we have xhci->num_usb3_ports > 0 then we have 2 times to trigger to
> start xHC in xhci_run. Once by the primary_hcd and once by share_hcd.
> We will have 5000M root hubs and following by 480M root hubs when
> triggering by the share_hcd and as properly.
>
> So the xHC should works when we only have primary_hcd or we have both
> primary hcd and share_hcd
>
> Here is the brief of the scenario for this patch:
> + create the primary hcd in xhci-plat
> + execute usb_add_hcd(hcd), execute xhci_run and start the xHC (for primary hcd)
> + if the xhci->num_usb3_ports > 0 then:
> + create the share_hcd structure
> + execute usb_add_hcd(share_hcd), execute xhci_run and start
> the xHC again (for share_hcd)
> + The root hub run at 5000M
> else if xhci->num_usb3_ports == 0 then:
> + don't create the share_hcd structure
> + don't usb_add_hcd(share_hcd)
> + the xHC can run with root hub 480M
>
> Please share your ideas for something should be changed.
>
original xhci_plat probe:
-------------------------
usb_create_hcd(hcd) // USB2
usb_create_hcd(shared) // USB3 xhci->shared_hcd exists now, and hcd->primary is set for the USB3 hcd
usb_add_hcd(hcd) // USB2
hcd->driver->start(hcd) // = .xhci_run()
if (!usb_hcd_is_primary_hcd(hcd)) // FALSE, this is the primary USB2
else
do other setup stuff
usb_add_hcd(shared) // USB3
hcd->driver->start(shared) // = .xhci_run()
if (!usb_hcd_is_primary_hcd(hcd)) // TRUE, this is the secondary USB3
xhci_run_finished()
xhci_start()
actually write bit to xHC that starts conrtoller.
so xHC HW is not really processing anyting before everything is ready
xhci_plat probe after this patch:
---------------------------------
usb_create_hcd(hcd) // USB2
usb_add_hcd(hcd) // USB2
hcd->driver->start(hcd) // = .xhci_run()
if (!xhci->shared_hcd) // TRUE, shared HCD is not yet created
if (xhci_start(xhci))
actually write bit to xHC that starts controller. // <------------ THIS IS THE PROBLEM
usb_create_hcd(shared) // USB3 xhci->shared_hcd exists now
usb_add_hcd(shared) // USB3
hcd->driver->start(shared) // = .xhci_run()
...
So there is a time when xHC HW is processing but the USB3 side in driver is not properly set up yet.
-Mathias
Powered by blists - more mailing lists