[PATCH] USB: set device dma_mask without reference to global data

[PATCH] USB: set device dma_mask without reference to global data

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Many USB host drivers contain code such as:

if (!pdev->dev.dma_mask)
        pdev->dev.dma_mask = &tegra_ehci_dma_mask;

... where tegra_ehci_dma_mask is a global. I suspect this code originated
in commit 4a53f4e "USB: ehci-tegra: add probing through device tree" and
was simply copied everywhere else.

This works fine when the code is built-in, but can cause a crash when the
code is in a module. The first module load sets up the dma_mask pointer,
but if the module is removed and re-inserted, the value is now non-NULL,
and hence is not updated to point at the new location, and hence points
at a stale location within the previous module load address, which in
turn causes a crash if the pointer is de-referenced.

The simplest way of solving this seems to be to copy the code from
ehci-platform.c, which uses the coherent_dma_mask as the target for the
dma_mask pointer.

Suggested-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   15 ++++-----------
 drivers/usb/dwc3/dwc3-exynos.c     |    6 +++---
 drivers/usb/host/ehci-atmel.c      |    6 +++---
 drivers/usb/host/ehci-omap.c       |    8 ++++----
 drivers/usb/host/ehci-orion.c      |    6 +++---
 drivers/usb/host/ehci-s5p.c        |    4 +---
 drivers/usb/host/ehci-spear.c      |    6 +++---
 drivers/usb/host/ehci-tegra.c      |    6 +++---
 drivers/usb/host/ohci-at91.c       |    6 +++---
 drivers/usb/host/ohci-exynos.c     |    4 +---
 drivers/usb/host/ohci-omap3.c      |    8 ++++----
 drivers/usb/host/ohci-pxa27x.c     |    6 +++---
 drivers/usb/host/ohci-spear.c      |    6 +++---
 drivers/usb/host/uhci-platform.c   |    6 +++---
 14 files changed, 41 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 8faec9d..73f9d5f 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -173,17 +173,10 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 
 	ci13xxx_imx_platdata.phy = data->phy;
 
-	if (!pdev->dev.dma_mask) {
-		pdev->dev.dma_mask = devm_kzalloc(&pdev->dev,
-				      sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
-		if (!pdev->dev.dma_mask) {
-			ret = -ENOMEM;
-			dev_err(&pdev->dev, "Failed to alloc dma_mask!\n");
-			goto err;
-		}
-		*pdev->dev.dma_mask = DMA_BIT_MASK(32);
-		dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
-	}
+	if (!pdev->dev.dma_mask)
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	if (usbmisc_ops && usbmisc_ops->init) {
 		ret = usbmisc_ops->init(&pdev->dev);
diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index a8afe6e..929e7dd 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -95,8 +95,6 @@ static int dwc3_exynos_remove_child(struct device *dev, void *unused)
 	return 0;
 }
 
-static u64 dwc3_exynos_dma_mask = DMA_BIT_MASK(32);
-
 static int dwc3_exynos_probe(struct platform_device *pdev)
 {
 	struct dwc3_exynos	*exynos;
@@ -118,7 +116,9 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 	 * Once we move to full device tree support this will vanish off.
 	 */
 	if (!dev->dma_mask)
-		dev->dma_mask = &dwc3_exynos_dma_mask;
+		dev->dma_mask = &dev->coherent_dma_mask;
+	if (!dev->coherent_dma_mask)
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
 
 	platform_set_drvdata(pdev, exynos);
 
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index 6642009..02f4611 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -63,8 +63,6 @@ static void atmel_stop_ehci(struct platform_device *pdev)
 
 /*-------------------------------------------------------------------------*/
 
-static u64 at91_ehci_dma_mask = DMA_BIT_MASK(32);
-
 static int ehci_atmel_drv_probe(struct platform_device *pdev)
 {
 	struct usb_hcd *hcd;
@@ -93,7 +91,9 @@ static int ehci_atmel_drv_probe(struct platform_device *pdev)
 	 * Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &at91_ehci_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd) {
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 3d1491b..16d7150 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -90,8 +90,6 @@ static const struct ehci_driver_overrides ehci_omap_overrides __initdata = {
 	.extra_priv_size = sizeof(struct omap_hcd),
 };
 
-static u64 omap_ehci_dma_mask = DMA_BIT_MASK(32);
-
 /**
  * ehci_hcd_omap_probe - initialize TI-based HCDs
  *
@@ -146,8 +144,10 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	 * Since shared usb code relies on it, set it here for now.
 	 * Once we have dma capability bindings this can go away.
 	 */
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &omap_ehci_dma_mask;
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+	if (!dev->coherent_dma_mask)
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
 
 	hcd = usb_create_hcd(&ehci_omap_hc_driver, dev,
 			dev_name(dev));
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 54c5794..efbc588 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -137,8 +137,6 @@ ehci_orion_conf_mbus_windows(struct usb_hcd *hcd,
 	}
 }
 
-static u64 ehci_orion_dma_mask = DMA_BIT_MASK(32);
-
 static int ehci_orion_drv_probe(struct platform_device *pdev)
 {
 	struct orion_ehci_data *pd = pdev->dev.platform_data;
@@ -183,7 +181,9 @@ static int ehci_orion_drv_probe(struct platform_device *pdev)
 	 * now. Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &ehci_orion_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	if (!request_mem_region(res->start, resource_size(res),
 				ehci_orion_hc_driver.description)) {
diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c
index 6357752..a81465e 100644
--- a/drivers/usb/host/ehci-s5p.c
+++ b/drivers/usb/host/ehci-s5p.c
@@ -71,8 +71,6 @@ static void s5p_setup_vbus_gpio(struct platform_device *pdev)
 		dev_err(dev, "can't request ehci vbus gpio %d", gpio);
 }
 
-static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32);
-
 static int s5p_ehci_probe(struct platform_device *pdev)
 {
 	struct s5p_ehci_platdata *pdata = pdev->dev.platform_data;
@@ -90,7 +88,7 @@ static int s5p_ehci_probe(struct platform_device *pdev)
 	 * Once we move to full device tree support this will vanish off.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &ehci_s5p_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
 	if (!pdev->dev.coherent_dma_mask)
 		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
index 61ecfb3..bd3e5cb 100644
--- a/drivers/usb/host/ehci-spear.c
+++ b/drivers/usb/host/ehci-spear.c
@@ -58,8 +58,6 @@ static int ehci_spear_drv_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(ehci_spear_pm_ops, ehci_spear_drv_suspend,
 		ehci_spear_drv_resume);
 
-static u64 spear_ehci_dma_mask = DMA_BIT_MASK(32);
-
 static int spear_ehci_hcd_drv_probe(struct platform_device *pdev)
 {
 	struct usb_hcd *hcd ;
@@ -84,7 +82,9 @@ static int spear_ehci_hcd_drv_probe(struct platform_device *pdev)
 	 * Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &spear_ehci_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	usbh_clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(usbh_clk)) {
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index e3eddc3..59d111b 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -637,8 +637,6 @@ static void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
 	writel(val, base + TEGRA_USB_PORTSC1);
 }
 
-static u64 tegra_ehci_dma_mask = DMA_BIT_MASK(32);
-
 static int tegra_ehci_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -661,7 +659,9 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	 * Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &tegra_ehci_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	setup_vbus_gpio(pdev, pdata);
 
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index a0cb44f..2ee1496 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -504,8 +504,6 @@ static const struct of_device_id at91_ohci_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, at91_ohci_dt_ids);
 
-static u64 at91_ohci_dma_mask = DMA_BIT_MASK(32);
-
 static int ohci_at91_of_init(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -522,7 +520,9 @@ static int ohci_at91_of_init(struct platform_device *pdev)
 	 * Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &at91_ohci_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 07592c0..b0b542c 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -98,8 +98,6 @@ static const struct hc_driver exynos_ohci_hc_driver = {
 	.start_port_reset	= ohci_start_port_reset,
 };
 
-static u64 ohci_exynos_dma_mask = DMA_BIT_MASK(32);
-
 static int exynos_ohci_probe(struct platform_device *pdev)
 {
 	struct exynos4_ohci_platdata *pdata = pdev->dev.platform_data;
@@ -117,7 +115,7 @@ static int exynos_ohci_probe(struct platform_device *pdev)
 	 * Once we move to full device tree support this will vanish off.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &ohci_exynos_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
 	if (!pdev->dev.coherent_dma_mask)
 		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
diff --git a/drivers/usb/host/ohci-omap3.c b/drivers/usb/host/ohci-omap3.c
index ddfc314..8663851 100644
--- a/drivers/usb/host/ohci-omap3.c
+++ b/drivers/usb/host/ohci-omap3.c
@@ -114,8 +114,6 @@ static const struct hc_driver ohci_omap3_hc_driver = {
 
 /*-------------------------------------------------------------------------*/
 
-static u64 omap_ohci_dma_mask = DMA_BIT_MASK(32);
-
 /*
  * configure so an HC device and id are always provided
  * always called with process context; sleeping is OK
@@ -168,8 +166,10 @@ static int ohci_hcd_omap3_probe(struct platform_device *pdev)
 	 * Since shared usb code relies on it, set it here for now.
 	 * Once we have dma capability bindings this can go away.
 	 */
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &omap_ohci_dma_mask;
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+	if (!dev->coherent_dma_mask)
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
 
 	hcd = usb_create_hcd(&ohci_omap3_hc_driver, dev,
 			dev_name(dev));
diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
index efe71f3..279b2ef 100644
--- a/drivers/usb/host/ohci-pxa27x.c
+++ b/drivers/usb/host/ohci-pxa27x.c
@@ -282,8 +282,6 @@ static const struct of_device_id pxa_ohci_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, pxa_ohci_dt_ids);
 
-static u64 pxa_ohci_dma_mask = DMA_BIT_MASK(32);
-
 static int ohci_pxa_of_init(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -298,7 +296,9 @@ static int ohci_pxa_of_init(struct platform_device *pdev)
 	 * Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &pxa_ohci_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
diff --git a/drivers/usb/host/ohci-spear.c b/drivers/usb/host/ohci-spear.c
index 9020bf0..3e19e01 100644
--- a/drivers/usb/host/ohci-spear.c
+++ b/drivers/usb/host/ohci-spear.c
@@ -91,8 +91,6 @@ static const struct hc_driver ohci_spear_hc_driver = {
 	.start_port_reset	= ohci_start_port_reset,
 };
 
-static u64 spear_ohci_dma_mask = DMA_BIT_MASK(32);
-
 static int spear_ohci_hcd_drv_probe(struct platform_device *pdev)
 {
 	const struct hc_driver *driver = &ohci_spear_hc_driver;
@@ -114,7 +112,9 @@ static int spear_ohci_hcd_drv_probe(struct platform_device *pdev)
 	 * Once we have dma capability bindings this can go away.
 	 */
 	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &spear_ohci_dma_mask;
+		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	if (!pdev->dev.coherent_dma_mask)
+		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	usbh_clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(usbh_clk)) {
diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
index 8c4dace..f1db61a 100644
--- a/drivers/usb/host/uhci-platform.c
+++ b/drivers/usb/host/uhci-platform.c
@@ -60,8 +60,6 @@ static const struct hc_driver uhci_platform_hc_driver = {
 	.hub_control =		uhci_hub_control,
 };
 
-static u64 platform_uhci_dma_mask = DMA_BIT_MASK(32);

Re: [PATCH] USB: set device dma_mask without reference to global data

[Peter Chen]

On Wed, May 8, 2013 at 6:53 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Many USB host drivers contain code such as:
>
> if (!pdev->dev.dma_mask)
>         pdev->dev.dma_mask = &tegra_ehci_dma_mask;
>
> ... where tegra_ehci_dma_mask is a global. I suspect this code originated
> in commit 4a53f4e "USB: ehci-tegra: add probing through device tree" and
> was simply copied everywhere else.
>

One question: why device tree can't do this when create device?

Re: [PATCH] USB: set device dma_mask without reference to global data

[Stephen Warren]

On 05/07/2013 07:13 PM, Peter Chen wrote:
> On Wed, May 8, 2013 at 6:53 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Many USB host drivers contain code such as:
>>
>> if (!pdev->dev.dma_mask)
>>         pdev->dev.dma_mask = &tegra_ehci_dma_mask;
>>
>> ... where tegra_ehci_dma_mask is a global. I suspect this code originated
>> in commit 4a53f4e "USB: ehci-tegra: add probing through device tree" and
>> was simply copied everywhere else.
> 
> One question: why device tree can't do this when create device?

This probably could be initialized from some DT property. However,
there's no such property defined right now, and considering that DT is
supposed to be an ABI, we'd always need the code in this patch as a
fallback for DTs that were created before any such property was defined.

Equally, since the data is SoC-specific rather than board-specific, and
is even fairly unlikely to vary between SoC versions since these values
are all 0xffffffff anyway, I don't really see much point in putting it
into DT, rather than just putting the static data into the driver.

Re: [PATCH] USB: set device dma_mask without reference to global data

[Peter Chen]

>
> This probably could be initialized from some DT property. However,
> there's no such property defined right now, and considering that DT is
> supposed to be an ABI, we'd always need the code in this patch as a
> fallback for DTs that were created before any such property was defined.
>
> Equally, since the data is SoC-specific rather than board-specific, and
> is even fairly unlikely to vary between SoC versions since these values
> are all 0xffffffff anyway, I don't really see much point in putting it
> into DT, rather than just putting the static data into the driver.

I mean there is already dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
at function of_platform_device_create, why can't add
dev->dev.dma_mask = &dev->dev.coherent_dma_mask after that?

If DT core can do above things, can we delete dma_mask assignment
at every driver?


--
BR,
Peter Chen

Re: [PATCH] USB: set device dma_mask without reference to global data

[Matthijs Kooijman]

Hi folks,

I also bumped into the question of how to set the dma_mask when enabling
the dwc2 driver on the ramips target and found there didn't seem to be
any clear way to get a dma_mask.

It seems to me that in the pre-DT era, a platform_device would get a
dma_mask when it was defined in the board / soc code, which makes sense
since that code knows if a dma_mask is required and what its value
should be (it seems to me that a driver can only know it needs a
dma_mask, but not what value it should have?).

> > This probably could be initialized from some DT property. However,
> > there's no such property defined right now, and considering that DT is
> > supposed to be an ABI, we'd always need the code in this patch as a
> > fallback for DTs that were created before any such property was defined.
It seems there has already been a patch to implement this. For
reference, this seems to be the most recent version:

https://lkml.org/lkml/2012/12/4/54

And here's the previous attempt, to which Rob Herring refers in a reply.

https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-March/013180.html

> > Equally, since the data is SoC-specific rather than board-specific, and
> > is even fairly unlikely to vary between SoC versions since these values
> > are all 0xffffffff anyway, I don't really see much point in putting it
> > into DT, rather than just putting the static data into the driver.
>
> I mean there is already dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> at function of_platform_device_create, why can't add
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask after that?
Perhaps it would sense to set the 32-bit mask as a default, but allow to
override this mask from the devicetree for boards that need another
value? Or perhaps override it from the soc code instead?

For the ramips target, the MIPS folks suggested another approach: The
soc code finds the platform_device generated by DT and adds the
dma_mask:

http://www.linux-mips.org/archives/linux-mips/2013-04/msg00162.html

> If DT core can do above things, can we delete dma_mask assignment
> at every driver?
That would seem like a likeably goal to me :-)


Gr.

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

Re: [PATCH] USB: set device dma_mask without reference to global data

[Matthijs Kooijman]

Hi,

> For the ramips target, the MIPS folks suggested another approach: The
> soc code finds the platform_device generated by DT and adds the
> dma_mask:
> 
> http://www.linux-mips.org/archives/linux-mips/2013-04/msg00162.html

For completeness: It seems this approach is not going to be used after
all. The approach (not this particular patch) was conceived to get ehci
support on ramips, but now that echi also sets a default mask and it
turns out that the other setup needed is best done through a phy device,
this approach is probably not needed anymore for ehci. For dwc2, I guess
it should also set up a dma_mask like ehci does (at least until we sort
out this thread) :-)

Gr.

Matthijs

Re: [PATCH] USB: set device dma_mask without reference to global data

[Rob Herring]

On 05/08/2013 02:11 AM, Matthijs Kooijman wrote:
> Hi folks,
> 
> I also bumped into the question of how to set the dma_mask when enabling
> the dwc2 driver on the ramips target and found there didn't seem to be
> any clear way to get a dma_mask.
> 
> It seems to me that in the pre-DT era, a platform_device would get a
> dma_mask when it was defined in the board / soc code, which makes sense
> since that code knows if a dma_mask is required and what its value
> should be (it seems to me that a driver can only know it needs a
> dma_mask, but not what value it should have?).
> 
>>> This probably could be initialized from some DT property. However,
>>> there's no such property defined right now, and considering that DT is
>>> supposed to be an ABI, we'd always need the code in this patch as a
>>> fallback for DTs that were created before any such property was defined.
> It seems there has already been a patch to implement this. For
> reference, this seems to be the most recent version:
> 
> https://lkml.org/lkml/2012/12/4/54
> 
> And here's the previous attempt, to which Rob Herring refers in a reply.
> 
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-March/013180.html

I believe most of the issues have been around supporting ARM LPAE
systems. There is a much more simple approach to address this by using
the dma_addr_t size to set the coherent_dma_mask which I have queued for
3.11:

https://patchwork-mail1.kernel.org/patch/2495861/

This does not set dma_mask though. There's always been some mystery
around why there are separate masks. I think for most systems dma_mask
can be set to coherent_dma_mask based on what Arnd found:

http://pastebin.com/E7fFVJyq

This can always be overridden by a platform with a bus notifier or by a
driver if needed.

Rob

> 
>>> Equally, since the data is SoC-specific rather than board-specific, and
>>> is even fairly unlikely to vary between SoC versions since these values
>>> are all 0xffffffff anyway, I don't really see much point in putting it
>>> into DT, rather than just putting the static data into the driver.
>>
>> I mean there is already dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>> at function of_platform_device_create, why can't add
>> dev->dev.dma_mask = &dev->dev.coherent_dma_mask after that?
> Perhaps it would sense to set the 32-bit mask as a default, but allow to
> override this mask from the devicetree for boards that need another
> value? Or perhaps override it from the soc code instead?
> 
> For the ramips target, the MIPS folks suggested another approach: The
> soc code finds the platform_device generated by DT and adds the
> dma_mask:
> 
> http://www.linux-mips.org/archives/linux-mips/2013-04/msg00162.html
> 
>> If DT core can do above things, can we delete dma_mask assignment
>> at every driver?
> That would seem like a likeably goal to me :-)
> 
> 
> Gr.
> 
> Matthijs
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH] USB: set device dma_mask without reference to global data

[Arnd Bergmann]

On Wednesday 08 May 2013, Rob Herring wrote:
> On 05/08/2013 02:11 AM, Matthijs Kooijman wrote:

> > https://lkml.org/lkml/2012/12/4/54
> > 
> > And here's the previous attempt, to which Rob Herring refers in a reply.
> > 
> > https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-March/013180.html
> 
> I believe most of the issues have been around supporting ARM LPAE
> systems. There is a much more simple approach to address this by using
> the dma_addr_t size to set the coherent_dma_mask which I have queued for
> 3.11:
> 
> https://patchwork-mail1.kernel.org/patch/2495861/

Hmm, this approach seems wrong -- a lot of devices I expect cannot
actually do DMA to addresses beyond 4GB. A better default would
be to use a 32 bit mask for all devices and override the ones that
actually matter for performance and are known to be 64-bit DMA
capable.

Laura, you obviously tried this code on an LPAE-enabled system. Have
you had a look in the hardware manual which DMA masters in the
system are actually 64-bit capable?

	Arnd

Re: [PATCH] USB: set device dma_mask without reference to global data

[Arnd Bergmann]

On Wednesday 08 May 2013, Peter Chen wrote:
> >
> > This probably could be initialized from some DT property. However,
> > there's no such property defined right now, and considering that DT is
> > supposed to be an ABI, we'd always need the code in this patch as a
> > fallback for DTs that were created before any such property was defined.
> >
> > Equally, since the data is SoC-specific rather than board-specific, and
> > is even fairly unlikely to vary between SoC versions since these values
> > are all 0xffffffff anyway, I don't really see much point in putting it
> > into DT, rather than just putting the static data into the driver.
> 
> I mean there is already dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> at function of_platform_device_create, why can't add
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask after that?
> 
> If DT core can do above things, can we delete dma_mask assignment
> at every driver?

It probably should. The main thing is that the dma_mask setting in
of_platform (and elsewhere) is a mess and that nobody so far had the
guts to try to get it right for good.

Setting a 32 bit DMA mask is /probably/ the right default on all
ARM systems, but there are caveats:

- Once you get to systems with larger than 32 bit addressing (powerpc64,
  arm64, arm32 with LPAE), it's not so obvious: you may have some devices
  that need a 32 bit mask and some that need a 64 bit mask.

- Some (very rare these days, thankfully) devices require a mask that is
  less than 32 bits. Since that knowledge is device specific, not platform
  specific, it should probably stay in the driver.

- There are cases (I know them only on powerpc, but they probably exist
  on ARM and other places too) where the mapping from bus addresses to
  physical addresses is not linear. There is a device-tree binding for
  a "dma-ranges" property that can accurately describe the specific
  mapping. Actually using this on architecture independent code requires
  not only setting the dma_mask but also supporting the remapping
  in the dma_map_ops.

- Things get more interesting in combination with an IOMMU. If we have
  an IOMMU, I think we should set the dma_mask pointer to the mask of
  the IOMMU and set the map_ops accordingly.

- Whether we actually need coherent_dma_mask these days is another hard
  question to answer. I suspect that the only thing really needing it
  was some version of the Itanium based Altix machine for its PCI
  devices and we'd be better off finding a simpler solution for platform
  devices. For all practical purposes I think coherent_dma_mask must be
  the same as dma_mask.

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

Re: [PATCH] USB: set device dma_mask without reference to global data

[Russell King - ARM Linux]

On Wed, May 08, 2013 at 09:24:44AM +0200, Arnd Bergmann wrote:
> It probably should. The main thing is that the dma_mask setting in
> of_platform (and elsewhere) is a mess and that nobody so far had the
> guts to try to get it right for good.
> 
> Setting a 32 bit DMA mask is /probably/ the right default on all
> ARM systems, but there are caveats:
> 
> - Once you get to systems with larger than 32 bit addressing (powerpc64,
>   arm64, arm32 with LPAE), it's not so obvious: you may have some devices
>   that need a 32 bit mask and some that need a 64 bit mask.

This is precisely why drivers should be using dma_set_mask() and the
coherent version to provide the mask for which the driver knows about
- iow, if the device itself is only capable of 32-bit DMA, then the
device must use dma_set_mask(dev, DMA_BIT_MASK(32)).
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: set device dma_mask without reference to global data

[Stephen Warren]

On 05/07/2013 08:54 PM, Peter Chen wrote:
>>
>> This probably could be initialized from some DT property. However,
>> there's no such property defined right now, and considering that DT is
>> supposed to be an ABI, we'd always need the code in this patch as a
>> fallback for DTs that were created before any such property was defined.
>>
>> Equally, since the data is SoC-specific rather than board-specific, and
>> is even fairly unlikely to vary between SoC versions since these values
>> are all 0xffffffff anyway, I don't really see much point in putting it
>> into DT, rather than just putting the static data into the driver.
> 
> I mean there is already dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> at function of_platform_device_create, why can't add
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask after that?
> 
> If DT core can do above things, can we delete dma_mask assignment
> at every driver?

Perhaps. However, such a change has a much larger potential for regressions.

I would suggest going with the current patch for 3.10 and any later
backports in order to reduce risk. We can revisit better cleanup for
later kernels.

Re: [PATCH] USB: set device dma_mask without reference to global data

[Russell King - ARM Linux]

On Wed, May 08, 2013 at 10:54:48AM +0800, Peter Chen wrote:
> >
> > This probably could be initialized from some DT property. However,
> > there's no such property defined right now, and considering that DT is
> > supposed to be an ABI, we'd always need the code in this patch as a
> > fallback for DTs that were created before any such property was defined.
> >
> > Equally, since the data is SoC-specific rather than board-specific, and
> > is even fairly unlikely to vary between SoC versions since these values
> > are all 0xffffffff anyway, I don't really see much point in putting it
> > into DT, rather than just putting the static data into the driver.
> 
> I mean there is already dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> at function of_platform_device_create, why can't add
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask after that?

Because technically they're different things, and if we have a driver
somewhere which uses the DMA API correctly by making use of
dma_set_mask() and dma_set_coherent_mask(), the two will interfere with
each other.

These two masks have always been separate.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: set device dma_mask without reference to global data

[Greg Kroah-Hartman]

On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Many USB host drivers contain code such as:
> 
> if (!pdev->dev.dma_mask)
>         pdev->dev.dma_mask = &tegra_ehci_dma_mask;
> 
> ... where tegra_ehci_dma_mask is a global. I suspect this code originated
> in commit 4a53f4e "USB: ehci-tegra: add probing through device tree" and
> was simply copied everywhere else.
> 
> This works fine when the code is built-in, but can cause a crash when the
> code is in a module. The first module load sets up the dma_mask pointer,
> but if the module is removed and re-inserted, the value is now non-NULL,
> and hence is not updated to point at the new location, and hence points
> at a stale location within the previous module load address, which in
> turn causes a crash if the pointer is de-referenced.
> 
> The simplest way of solving this seems to be to copy the code from
> ehci-platform.c, which uses the coherent_dma_mask as the target for the
> dma_mask pointer.
> 
> Suggested-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

So this needs to go in for 3.10, right?  Any older kernels as well?  If
so, which ones?

thanks,

greg k-h

Re: [PATCH] USB: set device dma_mask without reference to global data

[Arnd Bergmann]

On Wednesday 08 May 2013, Greg Kroah-Hartman wrote:
> On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote:
> > From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

> > Suggested-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> So this needs to go in for 3.10, right?  Any older kernels as well?  If
> so, which ones?

The fix should definitely go into 3.10, but I'd suggest waiting with
the backport for a couple of -rc releases to avoid possible regressions.
We know that the current code is broken, but few people fully understand
what is going on with coherent_dma_mask, so it might cause new problems
in combination with some other unknown bug, and I don't see this as
urgent: none of the ARM defconfigs build this driver as a loadable
module and there is no bug in the built-in case. For some reason, only
the ARM back-end drivers are broken.

The first occurence was apparently in 3.3, but only in ehci-tegra.c,
while the other drivers subsequently copied the bug.

	Arnd

Re: [PATCH] USB: set device dma_mask without reference to global data

[Alan Stern]

On Wed, 8 May 2013, Arnd Bergmann wrote:

> On Wednesday 08 May 2013, Greg Kroah-Hartman wrote:
> > On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote:
> > > From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> > > Suggested-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > So this needs to go in for 3.10, right?  Any older kernels as well?  If
> > so, which ones?
> 
> The fix should definitely go into 3.10, but I'd suggest waiting with
> the backport for a couple of -rc releases to avoid possible regressions.
> We know that the current code is broken, but few people fully understand
> what is going on with coherent_dma_mask, so it might cause new problems
> in combination with some other unknown bug, and I don't see this as
> urgent: none of the ARM defconfigs build this driver as a loadable
> module and there is no bug in the built-in case. For some reason, only
> the ARM back-end drivers are broken.
> 
> The first occurence was apparently in 3.3, but only in ehci-tegra.c,
> while the other drivers subsequently copied the bug.

An alternative solution -- perhaps not better but also not relying on
coherent_dma_mask -- is to clear pdev->dev.dma_mask in the remove
routine if it points to the static mask value.

Alan Stern

Re: [PATCH] USB: set device dma_mask without reference to global data

[Arnd Bergmann]

On Wednesday 08 May 2013, Alan Stern wrote:
> > The first occurence was apparently in 3.3, but only in ehci-tegra.c,
> > while the other drivers subsequently copied the bug.
> 
> An alternative solution -- perhaps not better but also not relying on
> coherent_dma_mask -- is to clear pdev->dev.dma_mask in the remove
> routine if it points to the static mask value.
> 

Yes, I thought about that, but couldn't see any advantage one way
or the other and this one seemed simpler.

	Arnd

Re: [PATCH] USB: set device dma_mask without reference to global data

[Russell King - ARM Linux]

On Wed, May 08, 2013 at 01:42:11AM +0200, Arnd Bergmann wrote:
> On Wednesday 08 May 2013, Greg Kroah-Hartman wrote:
> > On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote:
> > > From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> > > Suggested-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > So this needs to go in for 3.10, right?  Any older kernels as well?  If
> > so, which ones?
> 
> The fix should definitely go into 3.10, but I'd suggest waiting with
> the backport for a couple of -rc releases to avoid possible regressions.
> We know that the current code is broken, but few people fully understand
> what is going on with coherent_dma_mask, so it might cause new problems
> in combination with some other unknown bug, and I don't see this as
> urgent: none of the ARM defconfigs build this driver as a loadable
> module and there is no bug in the built-in case. For some reason, only
> the ARM back-end drivers are broken.
> 
> The first occurence was apparently in 3.3, but only in ehci-tegra.c,
> while the other drivers subsequently copied the bug.

I've already suggested this approach:

diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index dc662fc..51bb740 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 struct omap_device;
 
 struct pdev_archdata {
+	u64 dma_mask;
 #ifdef CONFIG_ARCH_OMAP
 	struct omap_device *od;
 #endif

And then we can have dev->dma_mask pointed at that instead, which fully
eliminates any possible problems of things like dma_set_mask() interfering
with dma_set_coherent_mask().

Even better - because this is a common problem - would be to make 'dma_mask'
be a member of struct platform_device so that all arches can sort this
out once and for all (correction: generic code/drivers can in an arch-
independent way.)  IOW:

diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 9abf1db..121c74c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -26,6 +26,7 @@ struct platform_device {
 	struct device	dev;
 	u32		num_resources;
 	struct resource	*resource;
+	u64		dma_mask;
 
 	const struct platform_device_id	*id_entry;
 

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

Re: [PATCH] USB: set device dma_mask without reference to global data

[Tony Prisk]

On 08/05/13 10:53, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Many USB host drivers contain code such as:
>
> if (!pdev->dev.dma_mask)
>          pdev->dev.dma_mask = &tegra_ehci_dma_mask;
>
> ... where tegra_ehci_dma_mask is a global. I suspect this code originated
> in commit 4a53f4e "USB: ehci-tegra: add probing through device tree" and
> was simply copied everywhere else.
>
> This works fine when the code is built-in, but can cause a crash when the
> code is in a module. The first module load sets up the dma_mask pointer,
> but if the module is removed and re-inserted, the value is now non-NULL,
> and hence is not updated to point at the new location, and hence points
> at a stale location within the previous module load address, which in
> turn causes a crash if the pointer is de-referenced.
>
> The simplest way of solving this seems to be to copy the code from
> ehci-platform.c, which uses the coherent_dma_mask as the target for the
> dma_mask pointer.
>
> Suggested-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
In the case of uhci-platform you would be absolutely correct. I copied the
example from tegra when we first had the problem on arch-vt8500.Because we
have no NAND support yet, I have always booted myrootfs from USB so it's
always been builtin and the problem wasnever a problem. The same problem 
would
have existed on ehci-vt8500 but Arnd replaced it with ehci-platform due to
the multiplatform issues.

for uhci-platform.c
Acked-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>

Regards
Tony P