linux-kernel - Re: [PATCH 1/5] usb: xhci: cleanup xhci

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: <552BBB2B.6010902@linux.intel.com>
Date:	Mon, 13 Apr 2015 15:48:43 +0300
From:	Mathias Nyman <mathias.nyman@...ux.intel.com>
To:	Roger Quadros <rogerq@...com>, mathias.nyman@...el.com,
	Alan Stern <stern@...land.harvard.edu>
CC:	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

Hi

On 09.04.2015 12:22, Roger Quadros wrote:
> Hi,
> 
> On 07/04/15 17:23, Mathias Nyman wrote:
>> Hi
>>
>> On 02.04.2015 15:23, Roger Quadros wrote:
>>> HCD core allocates memory for HCD private data in
>>> usb_create_[shared_]hcd() so make use of that
>>> mechanism to allocate the struct xhci_hcd.
>>>
>>> Introduce struct xhci_driver_overrides to provide
>>> the size of HCD private data and hc_driver operation
>>> overrides. As of now we only need to override the
>>> reset and start methods.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@...com>
>>
>> I'm not sure I fully understand the what's going on, or what the
>> intention of this patch is.
> 
> The main intention is to have both primary and shared HCDs allocated
> before calling usb_add_hcd() for the primary hcd.
> This is so that at the first usb_add_hcd() the OTG core knows the HCD topology
> (i.e. whether it uses a shared HCD or not).
> 
> From the OTG perspective we have to prevent the actual usb_add_hcd() till the
> OTG state machine says so.
> This means that xhci_gen_setup() won't be necessarily called immediately and
> so we need to allocate for xhci somewhere else.

Ok, thanks for explaining. I now understand the reason behind this.

> 
>>
>> So currently xhci driver manages the allocation and freeing of
>> the xhci_hcd structure. We store a pointer to the xhci_hcd structure in
>> the content of both the primary and shared usb_hcds structures hcd_priv
>> field.
>>
>> With this patch xhci would be part of the usb_hcd structure,
>> starting at hcd_priv[0]. (Like EHCI I think) It allocates enough space to include
>> the xhci_hcd in both the primary and shared usb_hcd, but always only use the one
>> in the primary hcd.
> 
> precisely.
> 
>> I'm not sure what to do with the space allocated for the shared hcd's
>> hcd_priv field.
> 
> we just ignore the space allocated for the shared hcd.

Ok, not a big loss.

> 
>>
>> This also means that xhci goes away together with the primary hcd. It's possible
>> this has some impact as the xhci driver expects xhci to always exists.
> 
> Can you please point out where this impact is.
> 
> I've been testing add/remove HCD extensively and didn't observe any issues after applying
> these 5 patches. Well there is one issue that comes up but it has nothing to do with xhci
> not being allocated. It has more to do with command being queued after the HCD has gone away
> and so getting stuck forever without timing out.

I went through the codepaths and you're right, should work fine. My concern wasn't valid. 
This patchset doesn't even touch the order how primary and shared HCDs are created and added
in the PCI case, only for the platform device case.   

I'll try it out and send forward once rc1 is out.

-Mathias

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