Re: [PATCH] USB: set device dma_mask without reference to global data
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 swar...@nvidia.com
Suggested-by: Arnd Bergmann a...@arndb.de
Signed-off-by: Stephen Warren swar...@nvidia.com
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-omap 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: set device dma_mask without reference to global data
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 0x 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-omap 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: set device dma_mask without reference to global data
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-omap 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: set device dma_mask without reference to global data
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 0x 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-omap 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: set device dma_mask without reference to global data
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 0x 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-omap 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: set device dma_mask without reference to global data
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 0x 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-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 swar...@nvidia.com Suggested-by: Arnd Bergmann a...@arndb.de Signed-off-by: Stephen Warren swar...@nvidia.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 0x 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. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: set device dma_mask without reference to global data
From: Stephen Warren swar...@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.
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 a...@arndb.de
Signed-off-by: Stephen Warren swar...@nvidia.com
---
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
Re: [PATCH] USB: set device dma_mask without reference to global data
On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote: From: Stephen Warren swar...@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. 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 a...@arndb.de Signed-off-by: Stephen Warren swar...@nvidia.com So this needs to go in for 3.10, right? Any older kernels as well? If so, which ones? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 swar...@nvidia.com Suggested-by: Arnd Bergmann a...@arndb.de Signed-off-by: Stephen Warren swar...@nvidia.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
On Wed, May 8, 2013 at 6:53 AM, Stephen Warren swar...@wwwdotorg.org wrote: From: Stephen Warren swar...@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? -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
On 05/07/2013 07:13 PM, Peter Chen wrote: On Wed, May 8, 2013 at 6:53 AM, Stephen Warren swar...@wwwdotorg.org wrote: From: Stephen Warren swar...@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 0x anyway, I don't really see much point in putting it into DT, rather than just putting the static data into the driver. -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
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 0x 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
On 08/05/13 10:53, Stephen Warren wrote: From: Stephen Warren swar...@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. 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 a...@arndb.de Signed-off-by: Stephen Warren swar...@nvidia.com --- 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 li...@prisktech.co.nz Regards Tony P -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
