Re: [PATCH 1/5] usb: xhci: cleanup xhci_hcd allocation

[Mathias Nyman]

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@xxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/