From: "Ivan T. Ivanov" <[email protected]>
When deferred probe happens driver will try to ioremap multiple times
and will fail. Memory resource.start variable is a global variable,
modifications in this field will be accumulated on every probe.
Fix this by moving the above operations after driver hold all
required PHY's.
Signed-off-by: Ivan T. Ivanov <[email protected]>
---
drivers/usb/dwc3/core.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
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;
+
+ /*
+ * 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);
+
usb_phy_set_suspend(dwc->usb2_phy, 0);
usb_phy_set_suspend(dwc->usb3_phy, 0);
--
1.7.9.5
On 07/25/2013 08:26 PM, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <[email protected]>
> When deferred probe happens driver will try to ioremap multiple times
> and will fail. Memory resource.start variable is a global variable,
> modifications in this field will be accumulated on every probe.
> Fix this by moving the above operations after driver hold all
> required PHY's.
> Signed-off-by: Ivan T. Ivanov <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
> 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
[...]
> @@ -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;
> +
> + /*
> + * Request memory region but exclude xHCI regs,
> + * since it will be requested by the xhci-plat driver.
> + */
Please remove an extra space after a tab on each comment line.
It seems like a good time to do it, while you're moving this code.
> + regs = devm_ioremap_resource(dev, res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
WBR, Sergei
On Thu, 2013-07-25 at 21:21 +0400, Sergei Shtylyov wrote:
> On 07/25/2013 08:26 PM, Ivan T. Ivanov wrote:
>
> > From: "Ivan T. Ivanov" <[email protected]>
>
> > When deferred probe happens driver will try to ioremap multiple times
> > and will fail. Memory resource.start variable is a global variable,
> > modifications in this field will be accumulated on every probe.
> > Fix this by moving the above operations after driver hold all
> > required PHY's.
>
Forgot to mention, generated on top of Felipe's 'testing' branch.
> > Signed-off-by: Ivan T. Ivanov <[email protected]>
> > ---
> > drivers/usb/dwc3/core.c | 31 ++++++++++++++++---------------
> > 1 file changed, 16 insertions(+), 15 deletions(-)
>
> > 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
> [...]
> > @@ -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;
> > +
> > + /*
> > + * Request memory region but exclude xHCI regs,
> > + * since it will be requested by the xhci-plat driver.
> > + */
>
> Please remove an extra space after a tab on each comment line.
> It seems like a good time to do it, while you're moving this code.
>
Ok.
Regards,
Ivan
> > + regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(regs))
> > + return PTR_ERR(regs);
> > +
>
> WBR, Sergei
>
> From: Ivan T. Ivanov
> Sent: Thursday, July 25, 2013 9:27 AM
>
> When deferred probe happens driver will try to ioremap multiple times
> and will fail. Memory resource.start variable is a global variable,
> modifications in this field will be accumulated on every probe.
> Fix this by moving the above operations after driver hold all
> required PHY's.
>
> Signed-off-by: Ivan T. Ivanov <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> 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
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'?
--
Paul
Hi,
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.
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.
--
balbi
> From: Felipe Balbi [mailto:[email protected]]
> 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.
All this assumes I'm reading the code correctly, of course. If I'm not,
then never mind :)
--
Paul
Hi,
On Fri, 2013-07-26 at 02:06 +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:[email protected]]
> > 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 :)
>
Hi,
On Fri, Jul 26, 2013 at 09:48:26AM +0300, Ivan T. Ivanov wrote:
> > > 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.
gotta try that one... Perhaps the correct way would be to copy the
resource to a private struct resource and modify that one, leaving
pdev->resources untouched.
> 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.
We can't do much about this since we're using devm_* API. Perhaps
deferred probe should make sure to destroy the device and add it back
later ? Otherwise what's the benefit of using devm_* ?
--
balbi
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Friday, July 26, 2013 2:54 AM
> > > > 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.
>
> gotta try that one... Perhaps the correct way would be to copy the
> resource to a private struct resource and modify that one, leaving
> pdev->resources untouched.
Maybe this is a dumb question, but why can't the driver that is going
to use the resource after this just "know" that it has to add
DWC3_GLOBALS_REGS_START to the start address? Are there some versions
of the core where that is not the case?
Or, maybe there should be two sets of resources?
--
Paul
Hi,
On Fri, Jul 26, 2013 at 06:44:23PM +0000, Paul Zimmerman wrote:
> > From: Felipe Balbi [mailto:[email protected]]
> > Sent: Friday, July 26, 2013 2:54 AM
> > > > > 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.
> >
> > gotta try that one... Perhaps the correct way would be to copy the
> > resource to a private struct resource and modify that one, leaving
> > pdev->resources untouched.
>
> Maybe this is a dumb question, but why can't the driver that is going
> to use the resource after this just "know" that it has to add
> DWC3_GLOBALS_REGS_START to the start address? Are there some versions
> of the core where that is not the case?
that won't work, because dwc3.ko will already have request_mem_region()
the entire region and a subsequent request_mem_region() for xHCI space
only would fail.
> Or, maybe there should be two sets of resources?
maybe we should require two sets of resources, yes... but then there's
no point in having any host initialization whatsoever in dwc3.ko.
--
balbi
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Friday, July 26, 2013 1:32 PM
>
> On Fri, Jul 26, 2013 at 06:44:23PM +0000, Paul Zimmerman wrote:
> > > From: Felipe Balbi [mailto:[email protected]]
> > > Sent: Friday, July 26, 2013 2:54 AM
> > > > > > 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.
> > >
> > > gotta try that one... Perhaps the correct way would be to copy the
> > > resource to a private struct resource and modify that one, leaving
> > > pdev->resources untouched.
> >
> > Maybe this is a dumb question, but why can't the driver that is going
> > to use the resource after this just "know" that it has to add
> > DWC3_GLOBALS_REGS_START to the start address? Are there some versions
> > of the core where that is not the case?
>
> that won't work, because dwc3.ko will already have request_mem_region()
> the entire region and a subsequent request_mem_region() for xHCI space
> only would fail.
>
> > Or, maybe there should be two sets of resources?
>
> maybe we should require two sets of resources, yes... but then there's
> no point in having any host initialization whatsoever in dwc3.ko.
Right. For a platform with a DWC3 controller, have DT or whatever set up
a second memory resource in the platform device, and have xhci_plat_probe()
look for that one first. If it finds it, it uses that resource instead
of the first one. If it doesn't find the second resource (not a DWC3
platform) then it uses the first one like it does today.
Then you don't need to have dwc3 set up the xhci resource like it does
today. Seems cleaner to me. 'Course it's incompatible with what you have
today, I guess that's the major drawback.
--
Paul