Hi, On Fri, 2013-07-26 at 02:06 +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Thursday, July 25, 2013 1:52 PM > > > > On Thu, Jul 25, 2013 at 07:46:58PM +0000, Paul Zimmerman wrote: > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 607bef8..50c833f 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -384,21 +384,6 @@ static int dwc3_probe(struct platform_device *pdev) > > > > dev_err(dev, "missing memory resource\n"); > > > > return -ENODEV; > > > > } > > > > - dwc->xhci_resources[0].start = res->start; > > > > - dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > > > > - DWC3_XHCI_REGS_END; > > > > - dwc->xhci_resources[0].flags = res->flags; > > > > - dwc->xhci_resources[0].name = res->name; > > > > - > > > > - res->start += DWC3_GLOBALS_REGS_START; > > > > - > > > > - /* > > > > - * Request memory region but exclude xHCI regs, > > > > - * since it will be requested by the xhci-plat driver. > > > > - */ > > > > - regs = devm_ioremap_resource(dev, res); > > > > - if (IS_ERR(regs)) > > > > - return PTR_ERR(regs); > > > > > > > > if (node) { > > > > dwc->maximum_speed = of_usb_get_maximum_speed(node); > > > > @@ -452,6 +437,22 @@ static int dwc3_probe(struct platform_device *pdev) > > > > return -EPROBE_DEFER; > > > > } > > > > > > > > + dwc->xhci_resources[0].start = res->start; > > > > + dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > > > > + DWC3_XHCI_REGS_END; > > > > + dwc->xhci_resources[0].flags = res->flags; > > > > + dwc->xhci_resources[0].name = res->name; > > > > + > > > > + res->start += DWC3_GLOBALS_REGS_START; > > > > > > Ick. The driver is modifying the struct resource passed to it by the > > > > heh... > > > > > platform code? That seems like a layering violation, and is fragile as > > > hell. In addition to this bug, what would happen if the struct resource > > > was declared 'const'? > > > > nothing would happen if it was declared const since platform_add_device > > makes a copy of what was declared, and that's always non-const. > > OK. > > > Also, this is not *modifying* what was passed, just skipping the xHCI > > address space so we don't request_mem_region() an area we won't really > > handle and prevent xhci-hcd.ko from probing. > > Hmm? platform_get_resource() returns a pointer to an entry in the > platform_device's resource[] array. And "res->start +=" modifies the > entry pointed at. If it didn't, the bug fixed by this patch wouldn't > have happened. > > Are you sure this code will work OK if you build the driver as a module, > modprobe it, rmmod it, and then modprobe it again? Seems like it won't, > unless the dev->resource[] array gets reinitialized in between somehow. In addition, I think driver is wasting memory, because on every probe it will reallocate driver state variable. This also happens in several other drivers which are using deferred probe. Regards, Ivan > > All this assumes I'm reading the code correctly, of course. If I'm not, > then never mind :) > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
