All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] gpu: host1x: Call of_dma_configure after setting bus
@ 2017-09-24  9:04 ` Mikko Perttunen
  0 siblings, 0 replies; 16+ messages in thread
From: Mikko Perttunen @ 2017-09-24  9:04 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA, hch-jcswGhMUV9g,
	robin.murphy-5wv7dgnIgG8, robh-DgEjT+Ai2ygdnm+yROfE0A
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Mikko Perttunen

of_dma_configure now checks the device's bus before configuring it, so
we need to set the device's bus before calling.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/host1x/bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index f9cde03030fd..66ea5acee820 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -403,12 +403,13 @@ static int host1x_device_add(struct host1x *host1x,
 	device->dev.coherent_dma_mask = host1x->dev->coherent_dma_mask;
 	device->dev.dma_mask = &device->dev.coherent_dma_mask;
 	dev_set_name(&device->dev, "%s", driver->driver.name);
-	of_dma_configure(&device->dev, host1x->dev->of_node);
 	device->dev.release = host1x_device_release;
 	device->dev.of_node = host1x->dev->of_node;
 	device->dev.bus = &host1x_bus_type;
 	device->dev.parent = host1x->dev;
 
+	of_dma_configure(&device->dev, host1x->dev->of_node);
+
 	err = host1x_device_parse_dt(device, driver);
 	if (err < 0) {
 		kfree(device);
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 1/2] gpu: host1x: Call of_dma_configure after setting bus
@ 2017-09-24  9:04 ` Mikko Perttunen
  0 siblings, 0 replies; 16+ messages in thread
From: Mikko Perttunen @ 2017-09-24  9:04 UTC (permalink / raw)
  To: linux-arm-kernel

of_dma_configure now checks the device's bus before configuring it, so
we need to set the device's bus before calling.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/gpu/host1x/bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index f9cde03030fd..66ea5acee820 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -403,12 +403,13 @@ static int host1x_device_add(struct host1x *host1x,
 	device->dev.coherent_dma_mask = host1x->dev->coherent_dma_mask;
 	device->dev.dma_mask = &device->dev.coherent_dma_mask;
 	dev_set_name(&device->dev, "%s", driver->driver.name);
-	of_dma_configure(&device->dev, host1x->dev->of_node);
 	device->dev.release = host1x_device_release;
 	device->dev.of_node = host1x->dev->of_node;
 	device->dev.bus = &host1x_bus_type;
 	device->dev.parent = host1x->dev;
 
+	of_dma_configure(&device->dev, host1x->dev->of_node);
+
 	err = host1x_device_parse_dt(device, driver);
 	if (err < 0) {
 		kfree(device);
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-24  9:04 ` Mikko Perttunen
@ 2017-09-24  9:04     ` Mikko Perttunen
  -1 siblings, 0 replies; 16+ messages in thread
From: Mikko Perttunen @ 2017-09-24  9:04 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA, hch-jcswGhMUV9g,
	robin.murphy-5wv7dgnIgG8, robh-DgEjT+Ai2ygdnm+yROfE0A
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Mikko Perttunen

Devices in the host1x bus rely on the old behavior of of_dma_configure
to set up DMA ops. Add a check for them into of_dma_configure.

We must do the check using a string comparison instead of using
pointers since the host1x bus can be compiled into a module.

Fixes: 723288836628 ("of: restrict DMA configuration")
Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 64b710265d39..12368418cd33 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
 		if (!dev_is_pci(dev) &&
 #ifdef CONFIG_ARM_AMBA
 		    dev->bus != &amba_bustype &&
+#endif
+#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
+		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
 #endif
 		    dev->bus != &platform_bus_type)
 			return ret == -ENODEV ? 0 : ret;
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-09-24  9:04     ` Mikko Perttunen
  0 siblings, 0 replies; 16+ messages in thread
From: Mikko Perttunen @ 2017-09-24  9:04 UTC (permalink / raw)
  To: linux-arm-kernel

Devices in the host1x bus rely on the old behavior of of_dma_configure
to set up DMA ops. Add a check for them into of_dma_configure.

We must do the check using a string comparison instead of using
pointers since the host1x bus can be compiled into a module.

Fixes: 723288836628 ("of: restrict DMA configuration")
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/of/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 64b710265d39..12368418cd33 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
 		if (!dev_is_pci(dev) &&
 #ifdef CONFIG_ARM_AMBA
 		    dev->bus != &amba_bustype &&
+#endif
+#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
+		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
 #endif
 		    dev->bus != &platform_bus_type)
 			return ret == -ENODEV ? 0 : ret;
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-24  9:04     ` Mikko Perttunen
@ 2017-09-25  7:44         ` Thierry Reding
  -1 siblings, 0 replies; 16+ messages in thread
From: Thierry Reding @ 2017-09-25  7:44 UTC (permalink / raw)
  To: Mikko Perttunen
  Cc: jonathanh-DDmLM1+adcrQT0dZR+AlfA, hch-jcswGhMUV9g,
	robin.murphy-5wv7dgnIgG8, robh-DgEjT+Ai2ygdnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
> Devices in the host1x bus rely on the old behavior of of_dma_configure
> to set up DMA ops. Add a check for them into of_dma_configure.
> 
> We must do the check using a string comparison instead of using
> pointers since the host1x bus can be compiled into a module.
> 
> Fixes: 723288836628 ("of: restrict DMA configuration")
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/of/device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..12368418cd33 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>  		if (!dev_is_pci(dev) &&
>  #ifdef CONFIG_ARM_AMBA
>  		    dev->bus != &amba_bustype &&
> +#endif
> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>  #endif
>  		    dev->bus != &platform_bus_type)
>  			return ret == -ENODEV ? 0 : ret;

Maybe a slightly better solution would be to add a dev_is_host1x()
function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
enabled and returns false otherwise. That way we can have a "proper"
check for the bus type and avoid the #if in this file.

It's slightly more complicated because of the dependencies between the
DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
in the DT tree.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-09-25  7:44         ` Thierry Reding
  0 siblings, 0 replies; 16+ messages in thread
From: Thierry Reding @ 2017-09-25  7:44 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
> Devices in the host1x bus rely on the old behavior of of_dma_configure
> to set up DMA ops. Add a check for them into of_dma_configure.
> 
> We must do the check using a string comparison instead of using
> pointers since the host1x bus can be compiled into a module.
> 
> Fixes: 723288836628 ("of: restrict DMA configuration")
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/of/device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..12368418cd33 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>  		if (!dev_is_pci(dev) &&
>  #ifdef CONFIG_ARM_AMBA
>  		    dev->bus != &amba_bustype &&
> +#endif
> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>  #endif
>  		    dev->bus != &platform_bus_type)
>  			return ret == -ENODEV ? 0 : ret;

Maybe a slightly better solution would be to add a dev_is_host1x()
function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
enabled and returns false otherwise. That way we can have a "proper"
check for the bus type and avoid the #if in this file.

It's slightly more complicated because of the dependencies between the
DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
in the DT tree.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170925/9c58ca02/attachment.sig>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-25  7:44         ` Thierry Reding
@ 2017-09-25 12:26             ` Robin Murphy
  -1 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2017-09-25 12:26 UTC (permalink / raw)
  To: Thierry Reding, Mikko Perttunen
  Cc: jonathanh-DDmLM1+adcrQT0dZR+AlfA, hch-jcswGhMUV9g,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r

On 25/09/17 08:44, Thierry Reding wrote:
> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>> to set up DMA ops. Add a check for them into of_dma_configure.
>>
>> We must do the check using a string comparison instead of using
>> pointers since the host1x bus can be compiled into a module.
>>
>> Fixes: 723288836628 ("of: restrict DMA configuration")
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/of/device.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 64b710265d39..12368418cd33 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>  		if (!dev_is_pci(dev) &&
>>  #ifdef CONFIG_ARM_AMBA
>>  		    dev->bus != &amba_bustype &&
>> +#endif
>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>  #endif
>>  		    dev->bus != &platform_bus_type)
>>  			return ret == -ENODEV ? 0 : ret;
> 
> Maybe a slightly better solution would be to add a dev_is_host1x()
> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
> enabled and returns false otherwise. That way we can have a "proper"
> check for the bus type and avoid the #if in this file.
> 
> It's slightly more complicated because of the dependencies between the
> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
> in the DT tree.

Alternatively (and modulo Greg's opinion, obviously) it would arguably be
neatest to handle this at a slightly lower level as below - I only really
considered this idea after writing the existing fix, but now that I have
actually written it up I do rather like it.

Robin.

----->8-----
From: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
Date: Mon, 25 Sep 2017 12:45:58 +0100
Subject: [PATCH] drivers: Flag buses which demand DMA configuration

We do not want the common dma_configure() pathway to apply
indiscriminately to all devices, since there are plenty of buses which
do not have DMA capability, and if their child devices were used for
DMA API calls it would only be indicative of a driver bug. However,
there are a number of buses for which DMA is implicitly expected even
when not described by firmware, so we currently whitelist those to
automatically opt in to dma_configure(), assuming a 1:1 mapping between
the DMA address space and the physical address space.

Commit 723288836628 ("of: restrict DMA configuration") introduced a
short-term fix by comparing explicit bus types, but this approach is far
from pretty, doesn't scale well, and fails to cope at all with bus
drivers which may be built as modules. Let's refine things by making
that opt-in a property of the bus type, which neatly addresses those
problems and lets the decision of whether firmware description of DMA
capability should be optional or mandatory stay internal to the bus
drivers themselves.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/amba/bus.c       | 1 +
 drivers/base/platform.c  | 1 +
 drivers/gpu/host1x/bus.c | 1 +
 drivers/of/device.c      | 8 +-------
 drivers/pci/pci-driver.c | 1 +
 include/linux/device.h   | 4 ++++
 6 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index e0f74ddc22b7..594c228d2f02 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
 	.match		= amba_match,
 	.uevent		= amba_uevent,
 	.pm		= &amba_pm,
+	.force_dma	= true,
 };
 
 static int __init amba_init(void)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index d1bd99271066..9c95c8db00f8 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
 	.match		= platform_match,
 	.uevent		= platform_uevent,
 	.pm		= &platform_dev_pm_ops,
+	.force_dma	= true,
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index f9cde03030fd..ed03b3243195 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
 	.name = "host1x",
 	.match = host1x_device_match,
 	.pm = &host1x_device_pm_ops,
+	.force_dma = true,
 };
 
 static void __host1x_device_del(struct host1x_device *device)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 64b710265d39..25bddf9c9fe1 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -9,9 +9,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
-#include <linux/pci.h>
 #include <linux/platform_device.h>
-#include <linux/amba/bus.h>
 
 #include <asm/errno.h>
 #include "of_private.h"
@@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
 		 * DMA configuration regardless of whether "dma-ranges" is
 		 * correctly specified or not.
 		 */
-		if (!dev_is_pci(dev) &&
-#ifdef CONFIG_ARM_AMBA
-		    dev->bus != &amba_bustype &&
-#endif
-		    dev->bus != &platform_bus_type)
+		if (!dev->bus->force_dma)
 			return ret == -ENODEV ? 0 : ret;
 
 		dma_addr = offset = 0;
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 11bd267fc137..38bdb97b6dc6 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
 	.drv_groups	= pci_drv_groups,
 	.pm		= PCI_PM_OPS_PTR,
 	.num_vf		= pci_bus_num_vf,
+	.force_dma	= true,
 };
 EXPORT_SYMBOL(pci_bus_type);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index c6f27207dbe8..3a1efa56e7c4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @p:		The private data of the driver core, only the driver core can
  *		touch this.
  * @lock_key:	Lock class key for use by the lock validator
+ * @force_dma:	Assume devices on this bus should be set up by dma_configure()
+ * 		even if DMA capability is not explicitly described by firmware.
  *
  * A bus is a channel between the processor and one or more devices. For the
  * purposes of the device model, all devices are connected via a bus, even if
@@ -135,6 +137,8 @@ struct bus_type {
 
 	struct subsys_private *p;
 	struct lock_class_key lock_key;
+
+	bool force_dma;
 };
 
 extern int __must_check bus_register(struct bus_type *bus);
-- 
2.13.4.dirty

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-09-25 12:26             ` Robin Murphy
  0 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2017-09-25 12:26 UTC (permalink / raw)
  To: linux-arm-kernel

On 25/09/17 08:44, Thierry Reding wrote:
> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>> to set up DMA ops. Add a check for them into of_dma_configure.
>>
>> We must do the check using a string comparison instead of using
>> pointers since the host1x bus can be compiled into a module.
>>
>> Fixes: 723288836628 ("of: restrict DMA configuration")
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>  drivers/of/device.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 64b710265d39..12368418cd33 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>  		if (!dev_is_pci(dev) &&
>>  #ifdef CONFIG_ARM_AMBA
>>  		    dev->bus != &amba_bustype &&
>> +#endif
>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>  #endif
>>  		    dev->bus != &platform_bus_type)
>>  			return ret == -ENODEV ? 0 : ret;
> 
> Maybe a slightly better solution would be to add a dev_is_host1x()
> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
> enabled and returns false otherwise. That way we can have a "proper"
> check for the bus type and avoid the #if in this file.
> 
> It's slightly more complicated because of the dependencies between the
> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
> in the DT tree.

Alternatively (and modulo Greg's opinion, obviously) it would arguably be
neatest to handle this at a slightly lower level as below - I only really
considered this idea after writing the existing fix, but now that I have
actually written it up I do rather like it.

Robin.

----->8-----
From: Robin Murphy <robin.murphy@arm.com>
Date: Mon, 25 Sep 2017 12:45:58 +0100
Subject: [PATCH] drivers: Flag buses which demand DMA configuration

We do not want the common dma_configure() pathway to apply
indiscriminately to all devices, since there are plenty of buses which
do not have DMA capability, and if their child devices were used for
DMA API calls it would only be indicative of a driver bug. However,
there are a number of buses for which DMA is implicitly expected even
when not described by firmware, so we currently whitelist those to
automatically opt in to dma_configure(), assuming a 1:1 mapping between
the DMA address space and the physical address space.

Commit 723288836628 ("of: restrict DMA configuration") introduced a
short-term fix by comparing explicit bus types, but this approach is far
from pretty, doesn't scale well, and fails to cope at all with bus
drivers which may be built as modules. Let's refine things by making
that opt-in a property of the bus type, which neatly addresses those
problems and lets the decision of whether firmware description of DMA
capability should be optional or mandatory stay internal to the bus
drivers themselves.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/amba/bus.c       | 1 +
 drivers/base/platform.c  | 1 +
 drivers/gpu/host1x/bus.c | 1 +
 drivers/of/device.c      | 8 +-------
 drivers/pci/pci-driver.c | 1 +
 include/linux/device.h   | 4 ++++
 6 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index e0f74ddc22b7..594c228d2f02 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
 	.match		= amba_match,
 	.uevent		= amba_uevent,
 	.pm		= &amba_pm,
+	.force_dma	= true,
 };
 
 static int __init amba_init(void)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index d1bd99271066..9c95c8db00f8 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
 	.match		= platform_match,
 	.uevent		= platform_uevent,
 	.pm		= &platform_dev_pm_ops,
+	.force_dma	= true,
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index f9cde03030fd..ed03b3243195 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
 	.name = "host1x",
 	.match = host1x_device_match,
 	.pm = &host1x_device_pm_ops,
+	.force_dma = true,
 };
 
 static void __host1x_device_del(struct host1x_device *device)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 64b710265d39..25bddf9c9fe1 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -9,9 +9,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
-#include <linux/pci.h>
 #include <linux/platform_device.h>
-#include <linux/amba/bus.h>
 
 #include <asm/errno.h>
 #include "of_private.h"
@@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
 		 * DMA configuration regardless of whether "dma-ranges" is
 		 * correctly specified or not.
 		 */
-		if (!dev_is_pci(dev) &&
-#ifdef CONFIG_ARM_AMBA
-		    dev->bus != &amba_bustype &&
-#endif
-		    dev->bus != &platform_bus_type)
+		if (!dev->bus->force_dma)
 			return ret == -ENODEV ? 0 : ret;
 
 		dma_addr = offset = 0;
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 11bd267fc137..38bdb97b6dc6 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
 	.drv_groups	= pci_drv_groups,
 	.pm		= PCI_PM_OPS_PTR,
 	.num_vf		= pci_bus_num_vf,
+	.force_dma	= true,
 };
 EXPORT_SYMBOL(pci_bus_type);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index c6f27207dbe8..3a1efa56e7c4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @p:		The private data of the driver core, only the driver core can
  *		touch this.
  * @lock_key:	Lock class key for use by the lock validator
+ * @force_dma:	Assume devices on this bus should be set up by dma_configure()
+ * 		even if DMA capability is not explicitly described by firmware.
  *
  * A bus is a channel between the processor and one or more devices. For the
  * purposes of the device model, all devices are connected via a bus, even if
@@ -135,6 +137,8 @@ struct bus_type {
 
 	struct subsys_private *p;
 	struct lock_class_key lock_key;
+
+	bool force_dma;
 };
 
 extern int __must_check bus_register(struct bus_type *bus);
-- 
2.13.4.dirty

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-25 12:26             ` Robin Murphy
@ 2017-09-25 12:47                 ` Christoph Hellwig
  -1 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2017-09-25 12:47 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Thierry Reding, Mikko Perttunen,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA, hch-jcswGhMUV9g,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r

On Mon, Sep 25, 2017 at 01:26:46PM +0100, Robin Murphy wrote:
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.

I also like this much better than hardcoding specific busses.  The only
real downside is that it makes it too easy to add exceptions :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-09-25 12:47                 ` Christoph Hellwig
  0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2017-09-25 12:47 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 25, 2017 at 01:26:46PM +0100, Robin Murphy wrote:
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.

I also like this much better than hardcoding specific busses.  The only
real downside is that it makes it too easy to add exceptions :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-25 12:26             ` Robin Murphy
@ 2017-09-25 18:26                 ` Mikko Perttunen
  -1 siblings, 0 replies; 16+ messages in thread
From: Mikko Perttunen @ 2017-09-25 18:26 UTC (permalink / raw)
  To: Robin Murphy, Thierry Reding, Mikko Perttunen
  Cc: jonathanh-DDmLM1+adcrQT0dZR+AlfA, hch-jcswGhMUV9g,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r

On 09/25/2017 03:26 PM, Robin Murphy wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
>> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>>> to set up DMA ops. Add a check for them into of_dma_configure.
>>>
>>> We must do the check using a string comparison instead of using
>>> pointers since the host1x bus can be compiled into a module.
>>>
>>> Fixes: 723288836628 ("of: restrict DMA configuration")
>>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   drivers/of/device.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index 64b710265d39..12368418cd33 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>>   		if (!dev_is_pci(dev) &&
>>>   #ifdef CONFIG_ARM_AMBA
>>>   		    dev->bus != &amba_bustype &&
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>>> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>>   #endif
>>>   		    dev->bus != &platform_bus_type)
>>>   			return ret == -ENODEV ? 0 : ret;
>>
>> Maybe a slightly better solution would be to add a dev_is_host1x()
>> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
>> enabled and returns false otherwise. That way we can have a "proper"
>> check for the bus type and avoid the #if in this file.
>>
>> It's slightly more complicated because of the dependencies between the
>> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
>> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
>> in the DT tree.
> 
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
> 
> Robin.

Yep, this solution seems the cleanest to me as well.

Mikko

> 
> ----->8-----
> From: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
> 
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
> 
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>   drivers/amba/bus.c       | 1 +
>   drivers/base/platform.c  | 1 +
>   drivers/gpu/host1x/bus.c | 1 +
>   drivers/of/device.c      | 8 +-------
>   drivers/pci/pci-driver.c | 1 +
>   include/linux/device.h   | 4 ++++
>   6 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>   	.match		= amba_match,
>   	.uevent		= amba_uevent,
>   	.pm		= &amba_pm,
> +	.force_dma	= true,
>   };
>   
>   static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>   	.match		= platform_match,
>   	.uevent		= platform_uevent,
>   	.pm		= &platform_dev_pm_ops,
> +	.force_dma	= true,
>   };
>   EXPORT_SYMBOL_GPL(platform_bus_type);
>   
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>   	.name = "host1x",
>   	.match = host1x_device_match,
>   	.pm = &host1x_device_pm_ops,
> +	.force_dma = true,
>   };
>   
>   static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>   #include <linux/module.h>
>   #include <linux/mod_devicetable.h>
>   #include <linux/slab.h>
> -#include <linux/pci.h>
>   #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>   
>   #include <asm/errno.h>
>   #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>   		 * DMA configuration regardless of whether "dma-ranges" is
>   		 * correctly specified or not.
>   		 */
> -		if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -		    dev->bus != &amba_bustype &&
> -#endif
> -		    dev->bus != &platform_bus_type)
> +		if (!dev->bus->force_dma)
>   			return ret == -ENODEV ? 0 : ret;
>   
>   		dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>   	.drv_groups	= pci_drv_groups,
>   	.pm		= PCI_PM_OPS_PTR,
>   	.num_vf		= pci_bus_num_vf,
> +	.force_dma	= true,
>   };
>   EXPORT_SYMBOL(pci_bus_type);
>   
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>    * @p:		The private data of the driver core, only the driver core can
>    *		touch this.
>    * @lock_key:	Lock class key for use by the lock validator
> + * @force_dma:	Assume devices on this bus should be set up by dma_configure()
> + * 		even if DMA capability is not explicitly described by firmware.
>    *
>    * A bus is a channel between the processor and one or more devices. For the
>    * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>   
>   	struct subsys_private *p;
>   	struct lock_class_key lock_key;
> +
> +	bool force_dma;
>   };
>   
>   extern int __must_check bus_register(struct bus_type *bus);
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-09-25 18:26                 ` Mikko Perttunen
  0 siblings, 0 replies; 16+ messages in thread
From: Mikko Perttunen @ 2017-09-25 18:26 UTC (permalink / raw)
  To: linux-arm-kernel

On 09/25/2017 03:26 PM, Robin Murphy wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
>> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>>> to set up DMA ops. Add a check for them into of_dma_configure.
>>>
>>> We must do the check using a string comparison instead of using
>>> pointers since the host1x bus can be compiled into a module.
>>>
>>> Fixes: 723288836628 ("of: restrict DMA configuration")
>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>> ---
>>>   drivers/of/device.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index 64b710265d39..12368418cd33 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>>   		if (!dev_is_pci(dev) &&
>>>   #ifdef CONFIG_ARM_AMBA
>>>   		    dev->bus != &amba_bustype &&
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>>> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>>   #endif
>>>   		    dev->bus != &platform_bus_type)
>>>   			return ret == -ENODEV ? 0 : ret;
>>
>> Maybe a slightly better solution would be to add a dev_is_host1x()
>> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
>> enabled and returns false otherwise. That way we can have a "proper"
>> check for the bus type and avoid the #if in this file.
>>
>> It's slightly more complicated because of the dependencies between the
>> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
>> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
>> in the DT tree.
> 
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
> 
> Robin.

Yep, this solution seems the cleanest to me as well.

Mikko

> 
> ----->8-----
> From: Robin Murphy <robin.murphy@arm.com>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
> 
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
> 
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/amba/bus.c       | 1 +
>   drivers/base/platform.c  | 1 +
>   drivers/gpu/host1x/bus.c | 1 +
>   drivers/of/device.c      | 8 +-------
>   drivers/pci/pci-driver.c | 1 +
>   include/linux/device.h   | 4 ++++
>   6 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>   	.match		= amba_match,
>   	.uevent		= amba_uevent,
>   	.pm		= &amba_pm,
> +	.force_dma	= true,
>   };
>   
>   static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>   	.match		= platform_match,
>   	.uevent		= platform_uevent,
>   	.pm		= &platform_dev_pm_ops,
> +	.force_dma	= true,
>   };
>   EXPORT_SYMBOL_GPL(platform_bus_type);
>   
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>   	.name = "host1x",
>   	.match = host1x_device_match,
>   	.pm = &host1x_device_pm_ops,
> +	.force_dma = true,
>   };
>   
>   static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>   #include <linux/module.h>
>   #include <linux/mod_devicetable.h>
>   #include <linux/slab.h>
> -#include <linux/pci.h>
>   #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>   
>   #include <asm/errno.h>
>   #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>   		 * DMA configuration regardless of whether "dma-ranges" is
>   		 * correctly specified or not.
>   		 */
> -		if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -		    dev->bus != &amba_bustype &&
> -#endif
> -		    dev->bus != &platform_bus_type)
> +		if (!dev->bus->force_dma)
>   			return ret == -ENODEV ? 0 : ret;
>   
>   		dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>   	.drv_groups	= pci_drv_groups,
>   	.pm		= PCI_PM_OPS_PTR,
>   	.num_vf		= pci_bus_num_vf,
> +	.force_dma	= true,
>   };
>   EXPORT_SYMBOL(pci_bus_type);
>   
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>    * @p:		The private data of the driver core, only the driver core can
>    *		touch this.
>    * @lock_key:	Lock class key for use by the lock validator
> + * @force_dma:	Assume devices on this bus should be set up by dma_configure()
> + * 		even if DMA capability is not explicitly described by firmware.
>    *
>    * A bus is a channel between the processor and one or more devices. For the
>    * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>   
>   	struct subsys_private *p;
>   	struct lock_class_key lock_key;
> +
> +	bool force_dma;
>   };
>   
>   extern int __must_check bus_register(struct bus_type *bus);
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-25 12:26             ` Robin Murphy
@ 2017-09-26 11:27                 ` Thierry Reding
  -1 siblings, 0 replies; 16+ messages in thread
From: Thierry Reding @ 2017-09-26 11:27 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Mikko Perttunen, jonathanh-DDmLM1+adcrQT0dZR+AlfA,
	hch-jcswGhMUV9g, robh-DgEjT+Ai2ygdnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r

[-- Attachment #1: Type: text/plain, Size: 7385 bytes --]

On Mon, Sep 25, 2017 at 01:26:46PM +0100, Robin Murphy wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
> > On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
> >> Devices in the host1x bus rely on the old behavior of of_dma_configure
> >> to set up DMA ops. Add a check for them into of_dma_configure.
> >>
> >> We must do the check using a string comparison instead of using
> >> pointers since the host1x bus can be compiled into a module.
> >>
> >> Fixes: 723288836628 ("of: restrict DMA configuration")
> >> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  drivers/of/device.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index 64b710265d39..12368418cd33 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
> >>  		if (!dev_is_pci(dev) &&
> >>  #ifdef CONFIG_ARM_AMBA
> >>  		    dev->bus != &amba_bustype &&
> >> +#endif
> >> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
> >> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
> >>  #endif
> >>  		    dev->bus != &platform_bus_type)
> >>  			return ret == -ENODEV ? 0 : ret;
> > 
> > Maybe a slightly better solution would be to add a dev_is_host1x()
> > function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
> > enabled and returns false otherwise. That way we can have a "proper"
> > check for the bus type and avoid the #if in this file.
> > 
> > It's slightly more complicated because of the dependencies between the
> > DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
> > Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
> > in the DT tree.
> 
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
> 
> Robin.
> 
> ----->8-----
> From: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
> 
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
> 
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/amba/bus.c       | 1 +
>  drivers/base/platform.c  | 1 +
>  drivers/gpu/host1x/bus.c | 1 +
>  drivers/of/device.c      | 8 +-------
>  drivers/pci/pci-driver.c | 1 +
>  include/linux/device.h   | 4 ++++
>  6 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>  	.match		= amba_match,
>  	.uevent		= amba_uevent,
>  	.pm		= &amba_pm,
> +	.force_dma	= true,
>  };
>  
>  static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>  	.match		= platform_match,
>  	.uevent		= platform_uevent,
>  	.pm		= &platform_dev_pm_ops,
> +	.force_dma	= true,
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>  
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>  	.name = "host1x",
>  	.match = host1x_device_match,
>  	.pm = &host1x_device_pm_ops,
> +	.force_dma = true,
>  };
>  
>  static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
> -#include <linux/pci.h>
>  #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>  
>  #include <asm/errno.h>
>  #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>  		 * DMA configuration regardless of whether "dma-ranges" is
>  		 * correctly specified or not.
>  		 */
> -		if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -		    dev->bus != &amba_bustype &&
> -#endif
> -		    dev->bus != &platform_bus_type)
> +		if (!dev->bus->force_dma)
>  			return ret == -ENODEV ? 0 : ret;
>  
>  		dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>  	.drv_groups	= pci_drv_groups,
>  	.pm		= PCI_PM_OPS_PTR,
>  	.num_vf		= pci_bus_num_vf,
> +	.force_dma	= true,
>  };
>  EXPORT_SYMBOL(pci_bus_type);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @p:		The private data of the driver core, only the driver core can
>   *		touch this.
>   * @lock_key:	Lock class key for use by the lock validator
> + * @force_dma:	Assume devices on this bus should be set up by dma_configure()
> + * 		even if DMA capability is not explicitly described by firmware.
>   *
>   * A bus is a channel between the processor and one or more devices. For the
>   * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>  
>  	struct subsys_private *p;
>  	struct lock_class_key lock_key;
> +
> +	bool force_dma;

Maybe force_dma_setup to clarify that we're not trying to force DMA
usage, but rather setup of DMA properties.

Either way:

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-09-26 11:27                 ` Thierry Reding
  0 siblings, 0 replies; 16+ messages in thread
From: Thierry Reding @ 2017-09-26 11:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 25, 2017 at 01:26:46PM +0100, Robin Murphy wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
> > On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
> >> Devices in the host1x bus rely on the old behavior of of_dma_configure
> >> to set up DMA ops. Add a check for them into of_dma_configure.
> >>
> >> We must do the check using a string comparison instead of using
> >> pointers since the host1x bus can be compiled into a module.
> >>
> >> Fixes: 723288836628 ("of: restrict DMA configuration")
> >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> >> ---
> >>  drivers/of/device.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index 64b710265d39..12368418cd33 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
> >>  		if (!dev_is_pci(dev) &&
> >>  #ifdef CONFIG_ARM_AMBA
> >>  		    dev->bus != &amba_bustype &&
> >> +#endif
> >> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
> >> +		    !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
> >>  #endif
> >>  		    dev->bus != &platform_bus_type)
> >>  			return ret == -ENODEV ? 0 : ret;
> > 
> > Maybe a slightly better solution would be to add a dev_is_host1x()
> > function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
> > enabled and returns false otherwise. That way we can have a "proper"
> > check for the bus type and avoid the #if in this file.
> > 
> > It's slightly more complicated because of the dependencies between the
> > DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
> > Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
> > in the DT tree.
> 
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
> 
> Robin.
> 
> ----->8-----
> From: Robin Murphy <robin.murphy@arm.com>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
> 
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
> 
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/amba/bus.c       | 1 +
>  drivers/base/platform.c  | 1 +
>  drivers/gpu/host1x/bus.c | 1 +
>  drivers/of/device.c      | 8 +-------
>  drivers/pci/pci-driver.c | 1 +
>  include/linux/device.h   | 4 ++++
>  6 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>  	.match		= amba_match,
>  	.uevent		= amba_uevent,
>  	.pm		= &amba_pm,
> +	.force_dma	= true,
>  };
>  
>  static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>  	.match		= platform_match,
>  	.uevent		= platform_uevent,
>  	.pm		= &platform_dev_pm_ops,
> +	.force_dma	= true,
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>  
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>  	.name = "host1x",
>  	.match = host1x_device_match,
>  	.pm = &host1x_device_pm_ops,
> +	.force_dma = true,
>  };
>  
>  static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
> -#include <linux/pci.h>
>  #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>  
>  #include <asm/errno.h>
>  #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>  		 * DMA configuration regardless of whether "dma-ranges" is
>  		 * correctly specified or not.
>  		 */
> -		if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -		    dev->bus != &amba_bustype &&
> -#endif
> -		    dev->bus != &platform_bus_type)
> +		if (!dev->bus->force_dma)
>  			return ret == -ENODEV ? 0 : ret;
>  
>  		dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>  	.drv_groups	= pci_drv_groups,
>  	.pm		= PCI_PM_OPS_PTR,
>  	.num_vf		= pci_bus_num_vf,
> +	.force_dma	= true,
>  };
>  EXPORT_SYMBOL(pci_bus_type);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @p:		The private data of the driver core, only the driver core can
>   *		touch this.
>   * @lock_key:	Lock class key for use by the lock validator
> + * @force_dma:	Assume devices on this bus should be set up by dma_configure()
> + * 		even if DMA capability is not explicitly described by firmware.
>   *
>   * A bus is a channel between the processor and one or more devices. For the
>   * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>  
>  	struct subsys_private *p;
>  	struct lock_class_key lock_key;
> +
> +	bool force_dma;

Maybe force_dma_setup to clarify that we're not trying to force DMA
usage, but rather setup of DMA properties.

Either way:

Acked-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170926/ac787ff8/attachment.sig>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] of: configure DMA for host1x devices
  2017-09-25 12:26             ` Robin Murphy
@ 2017-10-11 13:06                 ` Rob Herring
  -1 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2017-10-11 13:06 UTC (permalink / raw)
  To: Robin Murphy, Kishon Vijay Abraham I
  Cc: Thierry Reding, Mikko Perttunen, Jon Hunter, Christoph Hellwig,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r

On Mon, Sep 25, 2017 at 7:26 AM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
>> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>>> to set up DMA ops. Add a check for them into of_dma_configure.
>>>
>>> We must do the check using a string comparison instead of using
>>> pointers since the host1x bus can be compiled into a module.
>>>
>>> Fixes: 723288836628 ("of: restrict DMA configuration")
>>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/of/device.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index 64b710265d39..12368418cd33 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>>              if (!dev_is_pci(dev) &&
>>>  #ifdef CONFIG_ARM_AMBA
>>>                  dev->bus != &amba_bustype &&
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>>> +                !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>>  #endif
>>>                  dev->bus != &platform_bus_type)
>>>                      return ret == -ENODEV ? 0 : ret;
>>
>> Maybe a slightly better solution would be to add a dev_is_host1x()
>> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
>> enabled and returns false otherwise. That way we can have a "proper"
>> check for the bus type and avoid the #if in this file.
>>
>> It's slightly more complicated because of the dependencies between the
>> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
>> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
>> in the DT tree.
>
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
>
> Robin.
>
> ----->8-----
> From: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
>
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
>
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Are you going to send out as a proper patch? We've now got another
case with pci_epf_bus_type...

Rob

> ---
>  drivers/amba/bus.c       | 1 +
>  drivers/base/platform.c  | 1 +
>  drivers/gpu/host1x/bus.c | 1 +
>  drivers/of/device.c      | 8 +-------
>  drivers/pci/pci-driver.c | 1 +
>  include/linux/device.h   | 4 ++++
>  6 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>         .match          = amba_match,
>         .uevent         = amba_uevent,
>         .pm             = &amba_pm,
> +       .force_dma      = true,
>  };
>
>  static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>         .match          = platform_match,
>         .uevent         = platform_uevent,
>         .pm             = &platform_dev_pm_ops,
> +       .force_dma      = true,
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>         .name = "host1x",
>         .match = host1x_device_match,
>         .pm = &host1x_device_pm_ops,
> +       .force_dma = true,
>  };
>
>  static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
> -#include <linux/pci.h>
>  #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>
>  #include <asm/errno.h>
>  #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>                  * DMA configuration regardless of whether "dma-ranges" is
>                  * correctly specified or not.
>                  */
> -               if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -                   dev->bus != &amba_bustype &&
> -#endif
> -                   dev->bus != &platform_bus_type)
> +               if (!dev->bus->force_dma)
>                         return ret == -ENODEV ? 0 : ret;
>
>                 dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>         .drv_groups     = pci_drv_groups,
>         .pm             = PCI_PM_OPS_PTR,
>         .num_vf         = pci_bus_num_vf,
> +       .force_dma      = true,
>  };
>  EXPORT_SYMBOL(pci_bus_type);
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @p:         The private data of the driver core, only the driver core can
>   *             touch this.
>   * @lock_key:  Lock class key for use by the lock validator
> + * @force_dma: Assume devices on this bus should be set up by dma_configure()
> + *             even if DMA capability is not explicitly described by firmware.
>   *
>   * A bus is a channel between the processor and one or more devices. For the
>   * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>
>         struct subsys_private *p;
>         struct lock_class_key lock_key;
> +
> +       bool force_dma;
>  };
>
>  extern int __must_check bus_register(struct bus_type *bus);
> --
> 2.13.4.dirty
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/2] of: configure DMA for host1x devices
@ 2017-10-11 13:06                 ` Rob Herring
  0 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2017-10-11 13:06 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 25, 2017 at 7:26 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
>> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>>> to set up DMA ops. Add a check for them into of_dma_configure.
>>>
>>> We must do the check using a string comparison instead of using
>>> pointers since the host1x bus can be compiled into a module.
>>>
>>> Fixes: 723288836628 ("of: restrict DMA configuration")
>>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>>> ---
>>>  drivers/of/device.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index 64b710265d39..12368418cd33 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>>              if (!dev_is_pci(dev) &&
>>>  #ifdef CONFIG_ARM_AMBA
>>>                  dev->bus != &amba_bustype &&
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>>> +                !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>>  #endif
>>>                  dev->bus != &platform_bus_type)
>>>                      return ret == -ENODEV ? 0 : ret;
>>
>> Maybe a slightly better solution would be to add a dev_is_host1x()
>> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
>> enabled and returns false otherwise. That way we can have a "proper"
>> check for the bus type and avoid the #if in this file.
>>
>> It's slightly more complicated because of the dependencies between the
>> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
>> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
>> in the DT tree.
>
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
>
> Robin.
>
> ----->8-----
> From: Robin Murphy <robin.murphy@arm.com>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
>
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
>
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Rob Herring <robh@kernel.org>

Are you going to send out as a proper patch? We've now got another
case with pci_epf_bus_type...

Rob

> ---
>  drivers/amba/bus.c       | 1 +
>  drivers/base/platform.c  | 1 +
>  drivers/gpu/host1x/bus.c | 1 +
>  drivers/of/device.c      | 8 +-------
>  drivers/pci/pci-driver.c | 1 +
>  include/linux/device.h   | 4 ++++
>  6 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>         .match          = amba_match,
>         .uevent         = amba_uevent,
>         .pm             = &amba_pm,
> +       .force_dma      = true,
>  };
>
>  static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>         .match          = platform_match,
>         .uevent         = platform_uevent,
>         .pm             = &platform_dev_pm_ops,
> +       .force_dma      = true,
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>         .name = "host1x",
>         .match = host1x_device_match,
>         .pm = &host1x_device_pm_ops,
> +       .force_dma = true,
>  };
>
>  static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
> -#include <linux/pci.h>
>  #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>
>  #include <asm/errno.h>
>  #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>                  * DMA configuration regardless of whether "dma-ranges" is
>                  * correctly specified or not.
>                  */
> -               if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -                   dev->bus != &amba_bustype &&
> -#endif
> -                   dev->bus != &platform_bus_type)
> +               if (!dev->bus->force_dma)
>                         return ret == -ENODEV ? 0 : ret;
>
>                 dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>         .drv_groups     = pci_drv_groups,
>         .pm             = PCI_PM_OPS_PTR,
>         .num_vf         = pci_bus_num_vf,
> +       .force_dma      = true,
>  };
>  EXPORT_SYMBOL(pci_bus_type);
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @p:         The private data of the driver core, only the driver core can
>   *             touch this.
>   * @lock_key:  Lock class key for use by the lock validator
> + * @force_dma: Assume devices on this bus should be set up by dma_configure()
> + *             even if DMA capability is not explicitly described by firmware.
>   *
>   * A bus is a channel between the processor and one or more devices. For the
>   * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>
>         struct subsys_private *p;
>         struct lock_class_key lock_key;
> +
> +       bool force_dma;
>  };
>
>  extern int __must_check bus_register(struct bus_type *bus);
> --
> 2.13.4.dirty

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-10-11 13:06 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-24  9:04 [PATCH 1/2] gpu: host1x: Call of_dma_configure after setting bus Mikko Perttunen
2017-09-24  9:04 ` Mikko Perttunen
     [not found] ` <20170924090454.28116-1-mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2017-09-24  9:04   ` [PATCH 2/2] of: configure DMA for host1x devices Mikko Perttunen
2017-09-24  9:04     ` Mikko Perttunen
     [not found]     ` <20170924090454.28116-2-mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2017-09-25  7:44       ` Thierry Reding
2017-09-25  7:44         ` Thierry Reding
     [not found]         ` <20170925074448.GB12494-m5CkvRiFyV9wFLYp8hBm2A@public.gmane.org>
2017-09-25 12:26           ` Robin Murphy
2017-09-25 12:26             ` Robin Murphy
     [not found]             ` <dd8e9524-1d91-2c8b-a753-086d3ad9f9c9-5wv7dgnIgG8@public.gmane.org>
2017-09-25 12:47               ` Christoph Hellwig
2017-09-25 12:47                 ` Christoph Hellwig
2017-09-25 18:26               ` Mikko Perttunen
2017-09-25 18:26                 ` Mikko Perttunen
2017-09-26 11:27               ` Thierry Reding
2017-09-26 11:27                 ` Thierry Reding
2017-10-11 13:06               ` Rob Herring
2017-10-11 13:06                 ` Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.