RE: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-26 Thread Ivan T. Ivanov 
Hi,

On Fri, 2013-07-26 at 02:06 +, Paul Zimmerman wrote:
  From: Felipe Balbi [mailto:ba...@ti.com]
  Sent: Thursday, July 25, 2013 1:52 PM
  
  On Thu, Jul 25, 2013 at 07:46:58PM +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-26 Thread Felipe Balbi 
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


signature.asc
Description: Digital signature

RE: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-26 Thread Paul Zimmerman 
 From: Felipe Balbi [mailto:ba...@ti.com]
 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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-26 Thread Felipe Balbi 
Hi,

On Fri, Jul 26, 2013 at 06:44:23PM +, Paul Zimmerman wrote:
  From: Felipe Balbi [mailto:ba...@ti.com]
  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


signature.asc
Description: Digital signature

RE: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-26 Thread Paul Zimmerman 
 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Friday, July 26, 2013 1:32 PM
 
 On Fri, Jul 26, 2013 at 06:44:23PM +, Paul Zimmerman wrote:
   From: Felipe Balbi [mailto:ba...@ti.com]
   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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-25 Thread Sergei Shtylyov 

On 07/25/2013 08:26 PM, Ivan T. Ivanov wrote:


From: Ivan T. Ivanov iiva...@mm-sol.com



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 iiva...@mm-sol.com
---
  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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-25 Thread Ivan T. Ivanov 
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 iiva...@mm-sol.com
 
  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 iiva...@mm-sol.com
  ---
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
 


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-25 Thread Paul Zimmerman 
 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 iiva...@mm-sol.com
 ---
  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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-25 Thread Felipe Balbi 
Hi,

On Thu, Jul 25, 2013 at 07:46:58PM +, 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


signature.asc
Description: Digital signature

RE: [PATCH] usb: dwc3: core: modify IO memory resource after deferred probe completes

2013-07-25 Thread Paul Zimmerman 
 From: Felipe Balbi [mailto:ba...@ti.com]
 Sent: Thursday, July 25, 2013 1:52 PM
 
 On Thu, Jul 25, 2013 at 07:46:58PM +, 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

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html