Felipe, Gregory,
On Tue, 6 May 2014 02:14:00 +0200, Gregory CLEMENT wrote:
> +struct xhci_plat_priv {
> + struct clk *clk;
> +};
> +
> static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> {
> /*
> @@ -38,7 +43,8 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
> static const struct hc_driver xhci_plat_xhci_driver = {
> .description = "xhci-hcd",
> .product_desc = "xHCI Host Controller",
> - .hcd_priv_size = sizeof(struct xhci_hcd *),
> + .hcd_priv_size = sizeof(struct xhci_hcd *) +
> + sizeof(struct xhci_plat_priv),
>
> /*
> * generic hardware linkage
> @@ -85,6 +91,40 @@ static const struct hc_driver xhci_plat_xhci_driver = {
> .bus_resume = xhci_bus_resume,
> };
>
> +static int xhci_plat_enable_clk(struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd = platform_get_drvdata(pdev);
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + struct xhci_plat_priv *priv = (struct xhci_plat_priv *) xhci->priv;
Unless I misread the USB code, I believe the way this patch proposes to
handle private data for the XHCI HCD is wrong and leads to memory
corruption.
By growing the size .hcd_priv_size, it increases the memory size
pointed by usb_hcd->hcd_priv. However, this pointer has nothing to do
with xhci->priv, which points to the end of the xhci_hcd structure.
I believe the confusion comes from the fact that OHCI and EHCI do
allocate the entire ohci_hcd and ehci_hcd structure as part of the
usb_hcd private data:
.hcd_priv_size = sizeof(struct ohci_hcd),
or
.hcd_priv_size = sizeof(struct ehci_hcd),
In this case, enlarging hcd_priv_size, and having a ehci->priv or
ohci->priv pointing to the end of {ohci,ehci}_hcd structures works fine.
However, in the XHCI case, the usb_hcd private data is not used to hold
the entire xhci_hcd structure, but only a *pointer* to it:
.hcd_priv_size = sizeof(struct xhci_hcd *),
Therefore, adding more size to .hcd_priv_size isn't going to give extra
room at the end of the xhci_hcd structure. And therefore the whole
strategy of using xhci->priv pointing at the end of xhci_hcd is broken.
In v4, what I will do is simply to add a 'struct clk *' member to
xhci_hcd. A clock is, like a register area or an interrupt, a very
typical resource for any device, so it makes sense to have a pointer to
it from xhci_hcd. If someone complains that the clock would only be
used by xhci_plat, then I could point him to the fact that xhci_hcd
already contains members such as msix_count and msix_entries, that are
only used in xhci_pci :-)
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
