Introduce automatic DMA configuration for IOMMU masters

IOMMU drivers must be initialised before any of their upstream devices, otherwise the relevant iommu_ops won't be configured for the bus in question. To solve this, a number of IOMMU drivers use initcalls to initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing the IOMMU completely separate from the iommu_ops to configure the IOMMU, which are called on a per-bus basis and require the driver to figure out exactly which instance of the IOMMU is being requested. In particular, the add_device callback simply passes a struct device to the driver, which then has to parse firmware tables or probe buses to identify the relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an early initialisation pass for IOMMU drivers, giving them the ability to store some per-instance data in their iommu_ops structure and store that in their of_node. This can later be used when parsing OF masters to identify the IOMMU instance in question.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Joerg Roedel <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
 include/asm-generic/vmlinux.lds.h |  2 ++
 include/linux/iommu.h             |  2 ++
 include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
 4 files changed, 46 insertions(+)

drivers/iommu/of_iommu.c +17 -0

index e550ccb7634e..89b903406968 100644
@@ -22,6 +22,9 @@
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 
+static const struct of_device_id __iommu_of_table_sentinel
+       __used __section(__iommu_of_table_end);
+
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
  *

@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
        return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+       struct device_node *np;
+       const struct of_device_id *match, *matches = &__iommu_of_table;
+
+       for_each_matching_node_and_match(np, matches, &match) {
+               const of_iommu_init_fn init_fn = match->data;
+
+               if (init_fn(np))
+                       pr_err("Failed to initialise IOMMU %s\n",
+                               of_node_full_name(np));
+       }
+}

include/asm-generic/vmlinux.lds.h +2 -0

index aa70cbda327c..bee5d683074d 100644
@@ -164,6 +164,7 @@
 #define CLKSRC_OF_TABLES()     OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
 #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
 #define CLK_OF_TABLES()                OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()      OF_TABLE(CONFIG_OF_IOMMU, iommu)
 #define RESERVEDMEM_OF_TABLES()        OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
 #define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
 #define EARLYCON_OF_TABLES()   OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)

@@ -497,6 +498,7 @@
        CLK_OF_TABLES()                                                 \
        RESERVEDMEM_OF_TABLES()                                         \
        CLKSRC_OF_TABLES()                                              \
+       IOMMU_OF_TABLES()                                               \
        CPU_METHOD_OF_TABLES()                                          \
        KERNEL_DTB()                                                    \
        IRQCHIP_OF_MATCH_TABLE()                                        \

include/linux/iommu.h +2 -0

index e6a7c9ff72f2..7b83f9f8e11d 100644
@@ -103,6 +103,7 @@ enum iommu_attr {
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
  */
 struct iommu_ops {
        bool (*capable)(enum iommu_cap);

@@ -133,6 +134,7 @@ struct iommu_ops {
        u32 (*domain_get_windows)(struct iommu_domain *domain);
 
        unsigned long pgsize_bitmap;
+       void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE          1 /* Device added */

include/linux/of_iommu.h +7 -0

index 51a560f34bca..5762cdc8effe 100644
@@ -1,12 +1,17 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
                             int index, unsigned long *busno, dma_addr_t *addr,
                             size_t *size);
 
+extern void of_iommu_init(void);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -16,6 +21,26 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
        return -EINVAL;
 }
 
+static inline void of_iommu_init(void) { }
+
 #endif /* CONFIG_OF_IOMMU */
 

Adding Grant and Pantelis...

Quoted 133 lines · Will Deacon

IOMMU drivers must be initialised before any of their upstream devices, otherwise the relevant iommu_ops won't be configured for the bus in question. To solve this, a number of IOMMU drivers use initcalls to initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing the IOMMU completely separate from the iommu_ops to configure the IOMMU, which are called on a per-bus basis and require the driver to figure out exactly which instance of the IOMMU is being requested. In particular, the add_device callback simply passes a struct device to the driver, which then has to parse firmware tables or probe buses to identify the relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an early initialisation pass for IOMMU drivers, giving them the ability to store some per-instance data in their iommu_ops structure and store that in their of_node. This can later be used when parsing OF masters to identify the IOMMU instance in question.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Joerg Roedel <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
  drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
  include/asm-generic/vmlinux.lds.h |  2 ++
  include/linux/iommu.h             |  2 ++
  include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
  4 files changed, 46 insertions(+)

drivers/iommu/of_iommu.c +17 -0

index e550ccb7634e..89b903406968 100644
@@ -22,6 +22,9 @@
  #include <linux/of.h>
  #include <linux/of_iommu.h>
 
+static const struct of_device_id __iommu_of_table_sentinel
+       __used __section(__iommu_of_table_end);
+
  /**
   * of_get_dma_window - Parse *dma-window property and returns 0 if found.
   *

@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
         return 0;
  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+       struct device_node *np;
+       const struct of_device_id *match, *matches = &__iommu_of_table;
+
+       for_each_matching_node_and_match(np, matches, &match) {
+               const of_iommu_init_fn init_fn = match->data;
+
+               if (init_fn(np))
+                       pr_err("Failed to initialise IOMMU %s\n",
+                               of_node_full_name(np));
+       }
+}

include/asm-generic/vmlinux.lds.h +2 -0

index aa70cbda327c..bee5d683074d 100644
@@ -164,6 +164,7 @@
  #define CLKSRC_OF_TABLES()     OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
  #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
  #define CLK_OF_TABLES()                OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()      OF_TABLE(CONFIG_OF_IOMMU, iommu)
  #define RESERVEDMEM_OF_TABLES()        OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
  #define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
  #define EARLYCON_OF_TABLES()   OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)

@@ -497,6 +498,7 @@
         CLK_OF_TABLES()                                                 \
         RESERVEDMEM_OF_TABLES()                                         \
         CLKSRC_OF_TABLES()                                              \
+       IOMMU_OF_TABLES()                                               \
         CPU_METHOD_OF_TABLES()                                          \
         KERNEL_DTB()                                                    \
         IRQCHIP_OF_MATCH_TABLE()                                        \

include/linux/iommu.h +2 -0

index e6a7c9ff72f2..7b83f9f8e11d 100644
@@ -103,6 +103,7 @@ enum iommu_attr {
   * @domain_get_attr: Query domain attributes
   * @domain_set_attr: Change domain attributes
   * @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
   */
  struct iommu_ops {
         bool (*capable)(enum iommu_cap);

@@ -133,6 +134,7 @@ struct iommu_ops {
         u32 (*domain_get_windows)(struct iommu_domain *domain);
 
         unsigned long pgsize_bitmap;
+       void *priv;
  };
 
  #define IOMMU_GROUP_NOTIFY_ADD_DEVICE          1 /* Device added */

include/linux/of_iommu.h +7 -0

index 51a560f34bca..5762cdc8effe 100644
@@ -1,12 +1,17 @@
  #ifndef __OF_IOMMU_H
  #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
  #ifdef CONFIG_OF_IOMMU
 
  extern int of_get_dma_window(struct device_node *dn, const char *prefix,
                              int index, unsigned long *busno, dma_addr_t *addr,
                              size_t *size);
 
+extern void of_iommu_init(void);
+
  #else
 
  static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -16,6 +21,26 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
         return -EINVAL;
  }
 
+static inline void of_iommu_init(void) { }
+
  #endif /* CONFIG_OF_IOMMU */
 

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 10 lines

include/linux/of_iommu.h +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Quoted 8 lines

Quoted 8 lines

include/linux/of_iommu.h +8 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{

Quoted 8 lines

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 2 lines

include/linux/of_iommu.h +2 -0

+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

I think that for the initial version it is ok to use np->data. When per-iommu
controller structure is introduced later, it can be reused also for performing
of_node to iommu_ops lookup, because IOMMU framework will need to keep track on all such iommu controllers anyway.

Quoted 17 lines · On Tuesday 02 December 2014 10:23:00 Marek Szyprowski wrote:

Quoted 8 lines

Quoted 8 lines

include/linux/of_iommu.h +8 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{

Quoted 8 lines

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 2 lines

include/linux/of_iommu.h +2 -0

+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 6 lines

I think that for the initial version it is ok to use np->data. When per-iommu
controller structure is introduced later, it can be reused also for performing
of_node to iommu_ops lookup, because IOMMU framework will need to keep track on all such iommu controllers anyway.

Agreed. I think in the long run, we will have a 'struct iommu' that is added into a global linked list and that contains (among other things) an iommu_ops pointer and a device_node pointer. The of_iommu_get_ops function then walks the list to find the right iommu instance.

Quoted 17 lines · On Tuesday 02 December 2014 10:23:00 Marek Szyprowski wrote:

Quoted 8 lines

Quoted 8 lines

include/linux/of_iommu.h +8 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{

Quoted 8 lines

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 2 lines

include/linux/of_iommu.h +2 -0

+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 6 lines

I think that for the initial version it is ok to use np->data. When per-iommu
controller structure is introduced later, it can be reused also for performing
of_node to iommu_ops lookup, because IOMMU framework will need to keep track on all such iommu controllers anyway.

Agreed. I think in the long run, we will have a 'struct iommu' that is added into a global linked list and that contains (among other things) an iommu_ops pointer and a device_node pointer. The of_iommu_get_ops function then walks the list to find the right iommu instance.

Adding Grant and Pantelis...

Quoted 142 lines · Will Deacon

IOMMU drivers must be initialised before any of their upstream devices, otherwise the relevant iommu_ops won't be configured for the bus in question. To solve this, a number of IOMMU drivers use initcalls to initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing the IOMMU completely separate from the iommu_ops to configure the IOMMU, which are called on a per-bus basis and require the driver to figure out exactly which instance of the IOMMU is being requested. In particular, the add_device callback simply passes a struct device to the driver, which then has to parse firmware tables or probe buses to identify the relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an early initialisation pass for IOMMU drivers, giving them the ability to store some per-instance data in their iommu_ops structure and store that in their of_node. This can later be used when parsing OF masters to identify the IOMMU instance in question.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Joerg Roedel <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
include/asm-generic/vmlinux.lds.h |  2 ++
include/linux/iommu.h             |  2 ++
include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
4 files changed, 46 insertions(+)

drivers/iommu/of_iommu.c +17 -0

index e550ccb7634e..89b903406968 100644
@@ -22,6 +22,9 @@
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 
+static const struct of_device_id __iommu_of_table_sentinel
+       __used __section(__iommu_of_table_end);
+
 /**
 * of_get_dma_window - Parse *dma-window property and returns 0 if found.
 *

@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
       return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+       struct device_node *np;
+       const struct of_device_id *match, *matches = &__iommu_of_table;
+
+       for_each_matching_node_and_match(np, matches, &match) {
+               const of_iommu_init_fn init_fn = match->data;
+
+               if (init_fn(np))
+                       pr_err("Failed to initialise IOMMU %s\n",
+                               of_node_full_name(np));
+       }
+}

include/asm-generic/vmlinux.lds.h +2 -0

index aa70cbda327c..bee5d683074d 100644
@@ -164,6 +164,7 @@
 #define CLKSRC_OF_TABLES()     OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
 #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
 #define CLK_OF_TABLES()                OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()      OF_TABLE(CONFIG_OF_IOMMU, iommu)
 #define RESERVEDMEM_OF_TABLES()        OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
 #define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
 #define EARLYCON_OF_TABLES()   OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)

@@ -497,6 +498,7 @@
       CLK_OF_TABLES()                                                 \
       RESERVEDMEM_OF_TABLES()                                         \
       CLKSRC_OF_TABLES()                                              \
+       IOMMU_OF_TABLES()                                               \
       CPU_METHOD_OF_TABLES()                                          \
       KERNEL_DTB()                                                    \
       IRQCHIP_OF_MATCH_TABLE()                                        \

include/linux/iommu.h +2 -0

index e6a7c9ff72f2..7b83f9f8e11d 100644
@@ -103,6 +103,7 @@ enum iommu_attr {
 * @domain_get_attr: Query domain attributes
 * @domain_set_attr: Change domain attributes
 * @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
 */
 struct iommu_ops {
       bool (*capable)(enum iommu_cap);

@@ -133,6 +134,7 @@ struct iommu_ops {
       u32 (*domain_get_windows)(struct iommu_domain *domain);
 
       unsigned long pgsize_bitmap;
+       void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE          1 /* Device added */

include/linux/of_iommu.h +16 -0

index 51a560f34bca..5762cdc8effe 100644
@@ -1,12 +1,17 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
                            int index, unsigned long *busno, dma_addr_t *addr,
                            size_t *size);
 
+extern void of_iommu_init(void);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -16,6 +21,26 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
       return -EINVAL;
 }
 
+static inline void of_iommu_init(void) { }
+
 #endif /* CONFIG_OF_IOMMU */
 
+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 1 line

include/linux/of_iommu.h +1 -0

+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Rob

Adding Grant and Pantelis...

Quoted 133 lines · Will Deacon

IOMMU drivers must be initialised before any of their upstream devices, otherwise the relevant iommu_ops won't be configured for the bus in question. To solve this, a number of IOMMU drivers use initcalls to initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing the IOMMU completely separate from the iommu_ops to configure the IOMMU, which are called on a per-bus basis and require the driver to figure out exactly which instance of the IOMMU is being requested. In particular, the add_device callback simply passes a struct device to the driver, which then has to parse firmware tables or probe buses to identify the relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an early initialisation pass for IOMMU drivers, giving them the ability to store some per-instance data in their iommu_ops structure and store that in their of_node. This can later be used when parsing OF masters to identify the IOMMU instance in question.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Joerg Roedel <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
 include/asm-generic/vmlinux.lds.h |  2 ++
 include/linux/iommu.h             |  2 ++
 include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
 4 files changed, 46 insertions(+)

drivers/iommu/of_iommu.c +17 -0

index e550ccb7634e..89b903406968 100644
@@ -22,6 +22,9 @@
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 
+static const struct of_device_id __iommu_of_table_sentinel
+       __used __section(__iommu_of_table_end);
+
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
  *

@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
        return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+       struct device_node *np;
+       const struct of_device_id *match, *matches = &__iommu_of_table;
+
+       for_each_matching_node_and_match(np, matches, &match) {
+               const of_iommu_init_fn init_fn = match->data;
+
+               if (init_fn(np))
+                       pr_err("Failed to initialise IOMMU %s\n",
+                               of_node_full_name(np));
+       }
+}

include/asm-generic/vmlinux.lds.h +2 -0

index aa70cbda327c..bee5d683074d 100644
@@ -164,6 +164,7 @@
 #define CLKSRC_OF_TABLES()     OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
 #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
 #define CLK_OF_TABLES()                OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()      OF_TABLE(CONFIG_OF_IOMMU, iommu)
 #define RESERVEDMEM_OF_TABLES()        OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
 #define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method)
 #define EARLYCON_OF_TABLES()   OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)

@@ -497,6 +498,7 @@
        CLK_OF_TABLES()                                                 \
        RESERVEDMEM_OF_TABLES()                                         \
        CLKSRC_OF_TABLES()                                              \
+       IOMMU_OF_TABLES()                                               \
        CPU_METHOD_OF_TABLES()                                          \
        KERNEL_DTB()                                                    \
        IRQCHIP_OF_MATCH_TABLE()                                        \

include/linux/iommu.h +2 -0

index e6a7c9ff72f2..7b83f9f8e11d 100644
@@ -103,6 +103,7 @@ enum iommu_attr {
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
  */
 struct iommu_ops {
        bool (*capable)(enum iommu_cap);

@@ -133,6 +134,7 @@ struct iommu_ops {
        u32 (*domain_get_windows)(struct iommu_domain *domain);
 
        unsigned long pgsize_bitmap;
+       void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE          1 /* Device added */

include/linux/of_iommu.h +7 -0

index 51a560f34bca..5762cdc8effe 100644
@@ -1,12 +1,17 @@
 #ifndef __OF_IOMMU_H
 #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
 #ifdef CONFIG_OF_IOMMU
 
 extern int of_get_dma_window(struct device_node *dn, const char *prefix,
                             int index, unsigned long *busno, dma_addr_t *addr,
                             size_t *size);
 
+extern void of_iommu_init(void);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -16,6 +21,26 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
        return -EINVAL;
 }
 
+static inline void of_iommu_init(void) { }
+
 #endif /* CONFIG_OF_IOMMU */
 

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 10 lines

include/linux/of_iommu.h +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Quoted 20 lines · Rob Herring

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 11 lines · Will Deacon

include/linux/of_iommu.h +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 1 line

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 21 lines · On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:

Quoted 20 lines · Rob Herring

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 11 lines · Will Deacon

include/linux/of_iommu.h +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 8 lines

Quoted 1 line

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

Quoted 31 lines · Arnd Bergmann

Quoted 29 lines · On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:

Quoted 20 lines · Rob Herring

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 11 lines · Will Deacon

include/linux/of_iommu.h +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 1 line

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

Sure, I'll add this to my list of stuff to do for 3.20.

Quoted 32 lines · On Thursday 04 December 2014 09:49:53 Will Deacon wrote:

Quoted 31 lines · Arnd Bergmann

Quoted 29 lines · On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:

Quoted 20 lines · Rob Herring

include/linux/of_iommu.h static inline int of_get_dma_window(struct device_node *dn, const char *prefix, · line 37

Quoted 11 lines · Will Deacon

include/linux/of_iommu.h +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 1 line

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 5 lines

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

Sure, I'll add this to my list of stuff to do for 3.20.

Does that mean the we don't get any of the patches for 3.19 despite the Acks?

Quoted 33 lines · Arnd Bergmann

Quoted 32 lines · On Thursday 04 December 2014 09:49:53 Will Deacon wrote:

Quoted 31 lines · Arnd Bergmann

Quoted 29 lines · On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:

Quoted 20 lines · Rob Herring

Review

Quoted 11 lines · Will Deacon

(quoted diff) +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 1 line

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

Sure, I'll add this to my list of stuff to do for 3.20.

Does that mean the we don't get any of the patches for 3.19 despite the Acks?

Hmm, I don't know how useful they are without the get/set ops and I don't think I can get those ready for 3.19 given where we currently are.

Grant's suggestion of adding an iommu_ops pointer to device_node would work as a temporary hack, but anything more advanced is going to need proper review.

Quoted 46 lines · On Thursday 04 December 2014 10:21:27 Will Deacon wrote:

Quoted 33 lines · Arnd Bergmann

Quoted 32 lines · On Thursday 04 December 2014 09:49:53 Will Deacon wrote:

Quoted 31 lines · Arnd Bergmann

Quoted 29 lines · On Tuesday 02 December 2014 14:16:57 Grant Likely wrote:

Quoted 20 lines · Rob Herring

Review

Quoted 11 lines · Will Deacon

(quoted diff) +10 -0

+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->data = (struct iommu_ops *)ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->data;
+}

This may collide with other users. While use of it is rare, PPC uses it in its PCI code. The OF_DYNAMIC code frees it but never actually sets it. There may be some coming usage with the DT overlay code or that's just a bug. Pantelis or Grant can comment. If not, I think we really should try to get rid of this pointer rather than expand it's usage.

Quoted 1 line

I didn't see a user of this. I'm guessing that is coming in a SMMU patch?

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

Sure, I'll add this to my list of stuff to do for 3.20.

Does that mean the we don't get any of the patches for 3.19 despite the Acks?

Hmm, I don't know how useful they are without the get/set ops and I don't think I can get those ready for 3.19 given where we currently are.

Quoted 3 lines

Grant's suggestion of adding an iommu_ops pointer to device_node would work as a temporary hack, but anything more advanced is going to need proper review.

Right. I guess it doesn't hurt much if we put the new pointer inside #ifdef CONFIG_OF_IOMMU, then at least there is no significant size increase in most DT based platforms.

Quoted 9 lines · Arnd Bergmann

Quoted 8 lines · On Thursday 04 December 2014 10:21:27 Will Deacon wrote:

Quoted 4 lines

Quoted 1 line

Sure, I'll add this to my list of stuff to do for 3.20.

Does that mean the we don't get any of the patches for 3.19 despite the Acks?

Hmm, I don't know how useful they are without the get/set ops and I don't think I can get those ready for 3.19 given where we currently are.

Quoted 7 lines

Quoted 3 lines

Grant's suggestion of adding an iommu_ops pointer to device_node would work as a temporary hack, but anything more advanced is going to need proper review.

Right. I guess it doesn't hurt much if we put the new pointer inside #ifdef CONFIG_OF_IOMMU, then at least there is no significant size increase in most DT based platforms.

Yes, I can live with that hack on the proviso that it will be removed by v3.20

Oh, and please put an ugly /* */ comment block in the #ifdef CONFIG_OF_IOMMU section that makes it really clear that it is an ugly hack and will be removed in the next release. I don't want anyone getting ideas that adding pointers to struct device_node is a good idea.

Quoted 20 lines · Grant Likely

Quoted 9 lines · Arnd Bergmann

Quoted 8 lines · On Thursday 04 December 2014 10:21:27 Will Deacon wrote:

Quoted 4 lines

Quoted 1 line

Sure, I'll add this to my list of stuff to do for 3.20.

Does that mean the we don't get any of the patches for 3.19 despite the Acks?

Hmm, I don't know how useful they are without the get/set ops and I don't think I can get those ready for 3.19 given where we currently are.

Quoted 7 lines

Quoted 3 lines

Grant's suggestion of adding an iommu_ops pointer to device_node would work as a temporary hack, but anything more advanced is going to need proper review.

Right. I guess it doesn't hurt much if we put the new pointer inside #ifdef CONFIG_OF_IOMMU, then at least there is no significant size increase in most DT based platforms.

Yes, I can live with that hack on the proviso that it will be removed by v3.20

Quoted 5 lines

Oh, and please put an ugly /* */ comment block in the #ifdef CONFIG_OF_IOMMU section that makes it really clear that it is an ugly hack and will be removed in the next release. I don't want anyone getting ideas that adding pointers to struct device_node is a good idea.

Something like the mess below?

Will

--->8

include/linux/of.h +16 -0

index 29f0adc5f3e4..6f85c02bc1a6 100644
@@ -43,6 +43,9 @@ struct property {
 #if defined(CONFIG_SPARC)
 struct of_irq_controller;
 #endif
+#ifdef CONFIG_OF_IOMMU
+struct iommu_ops;
+#endif
 
 struct device_node {
        const char *name;

@@ -65,6 +68,19 @@ struct device_node {
        unsigned int unique_id;
        struct of_irq_controller *irq_trans;
 #endif
+#ifdef CONFIG_OF_IOMMU
+/*
+ * HACK! HACK! HACK!
+ *
+ * This is a temporary hack to associate a device_node for an
+ * IOMMU with its set of iommu_ops so that we can probe its upstream DMA
+ * masters on the platform bus by parsing the "iommus" property directly.
+ *
+ * This is going away in 3.20. Please use the of_iommu_{get,set}_ops
+ * functions to get hold of this data.
+ */
+       struct iommu_ops *__iommu_ops_use_accessors;
+#endif
 };
 
 #define MAX_PHANDLE_ARGS 16

include/linux/of_iommu.h +15 -8

index d03abbb11c34..392ec5f212db 100644
@@ -14,6 +14,17 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 extern void of_iommu_init(void);
 extern struct iommu_ops *of_iommu_configure(struct device *dev);
 
+static inline void of_iommu_set_ops(struct device_node *np,
+                                   const struct iommu_ops *ops)
+{
+       np->__iommu_ops_use_accessors = ops;
+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       return np->__iommu_ops_use_accessors;
+}
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -29,19 +40,15 @@ static inline struct iommu_ops *of_iommu_configure(struct device *dev)
        return NULL;
 }
 
-#endif /* CONFIG_OF_IOMMU */
-
 static inline void of_iommu_set_ops(struct device_node *np,
-                                   const struct iommu_ops *ops)
-{
-       np->data = (struct iommu_ops *)ops;
-}
-
+                                   const struct iommu_ops *ops) { }
 static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 {
-       return np->data;
+       return NULL;
 }
 
+#endif /* CONFIG_OF_IOMMU */
+
 extern struct of_device_id __iommu_of_table;
 
 typedef int (*of_iommu_init_fn)(struct device_node *);

Quoted 6 lines

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

Hi Arnd,

On 03/12/14 19:57, Arnd Bergmann wrote:
[...]

Quoted 9 lines

Quoted 7 lines

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

I'm quite keen to see this series go in, since I'm depending on it to make arm64 IOMMU DMA ops "just work". Will and I came to the conclusion the other day that we pretty much need to build up some kind of bus abstraction based on the probe data in order to be able to assign IOMMU groups correctly, which can also subsume this particular problem in the long run. Since I've made a start on that already, I've hacked the following short-term fix out of it. Tested on my Juno - admittedly with only two SMMUs and one master (EHCI) being probed, but it didn't blow up or regress anything.

Regards,
Robin.

--->8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001 Message-Id:
<1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH] iommu: store DT-probed IOMMU data privately

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/of_iommu.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 12 ++----------
 2 files changed, 40 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +0 -0

index 73236d3..5cd451c 100644

drivers/iommu/of_iommu.c +12 -0

+
+       INIT_HLIST_NODE(&iommu->list);
+       iommu->np = np;
+       iommu->ops = ops;
+       spin_lock(&of_iommu_lock);
+       hlist_add_head(&iommu->list, &of_iommu_list);
+       spin_unlock(&of_iommu_lock);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       struct of_iommu_node *node;

drivers/iommu/of_iommu.c +2 -0

+}
+
 struct iommu_ops *of_iommu_configure(struct device *dev)
 {

        struct of_phandle_args iommu_spec;

include/linux/of_iommu.h +2 -11

index d03abbb..e27c53a 100644
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
 device *dev)
 
 #endif /* CONFIG_OF_IOMMU */
 
-static inline void of_iommu_set_ops(struct device_node *np,
-                                   const struct iommu_ops *ops)
-{
-       np->data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
-       return np->data;
-}
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
 
 extern struct of_device_id __iommu_of_table;
 
--

1.9.1

Quoted 38 lines · Robin Murphy

Hi Arnd,

On 03/12/14 19:57, Arnd Bergmann wrote:
[...]

Quoted 9 lines

Quoted 7 lines

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

I'm quite keen to see this series go in, since I'm depending on it to make arm64 IOMMU DMA ops "just work". Will and I came to the conclusion the other day that we pretty much need to build up some kind of bus abstraction based on the probe data in order to be able to assign IOMMU groups correctly, which can also subsume this particular problem in the long run. Since I've made a start on that already, I've hacked the following short-term fix out of it. Tested on my Juno - admittedly with only two SMMUs and one master (EHCI) being probed, but it didn't blow up or regress anything.

Regards,
Robin.

--->8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
Message-Id:
<1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH] iommu: store DT-probed IOMMU data privately

Quoted 5 lines

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

Looks reasonable to me. Comments below...

Quoted 10 lines

---
  drivers/iommu/of_iommu.c | 38 ++++++++++++++++++++++++++++++++++++++
  include/linux/of_iommu.h | 12 ++----------
  2 files changed, 40 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +0 -0

index 73236d3..5cd451c 100644

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char · line 103

Quoted 10 lines

drivers/iommu/of_iommu.c +6 -0

@@ -94,6 +94,44 @@ int of_get_dma_window(struct device_node *dn, const char
 *prefix, int index,
  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+struct of_iommu_node {
+       struct hlist_node list;
+       struct device_node *np;
+       const struct iommu_ops *ops;
+};
+static HLIST_HEAD(of_iommu_list);

Just use a list_head. hlist_head merely saves one pointer in this case.

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char · line 108

Quoted 5 lines

drivers/iommu/of_iommu.c +5 -0

+static DEFINE_SPINLOCK(of_iommu_lock);
+
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
+{
+       struct of_iommu_node *iommu = kmalloc(sizeof(*iommu), GFP_KERNEL);

kzalloc()

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char · line 111

Quoted 3 lines

drivers/iommu/of_iommu.c +3 -0

+
+       if (!iommu)
+               return;

Shouldn't there be a WARN() here on failure? I don't think failing silently is desired.

Quoted 12 lines

drivers/iommu/of_iommu.c +12 -0

+
+       INIT_HLIST_NODE(&iommu->list);
+       iommu->np = np;
+       iommu->ops = ops;
+       spin_lock(&of_iommu_lock);
+       hlist_add_head(&iommu->list, &of_iommu_list);
+       spin_unlock(&of_iommu_lock);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       struct of_iommu_node *node;

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char · line 133

Quoted 10 lines

drivers/iommu/of_iommu.c +10 -0

+       const struct iommu_ops *ops = NULL;
+
+       spin_lock(&of_iommu_lock);
+       hlist_for_each_entry(node, &of_iommu_list, list)
+               if (node->np == np) {
+                       ops = node->ops;
+                       break;
+               }
+       spin_unlock(&of_iommu_lock);
+       return (struct iommu_ops *)ops;

The cast looks fishy. If you need to use a cast, then the data types are probably wrong. If you drop the const from *ops here and in the structure then it should probably work fine. Due to the way it is being used, there isn't any advantage to using const (unless you changes of_iommu_get_ops() to return a const pointer, then const would make sense).

Quoted 33 lines

drivers/iommu/of_iommu.c +2 -0

+}
+
  struct iommu_ops *of_iommu_configure(struct device *dev)
  {

         struct of_phandle_args iommu_spec;

include/linux/of_iommu.h +2 -11

index d03abbb..e27c53a 100644
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
 device *dev)
 
  #endif /* CONFIG_OF_IOMMU */
 
-static inline void of_iommu_set_ops(struct device_node *np,
-                                   const struct iommu_ops *ops)
-{
-       np->data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
-       return np->data;
-}
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
 
  extern struct of_device_id __iommu_of_table;
 
--

1.9.1

Hi Grant, thanks for the advice - silly micro-optimisations removed, and I'll make a note to do so from my in-development code, too ;)

I didn't much like the casting either, so rather than push it elsewhere or out to the caller I've just changed the prototype to obviate it completely. Since we're also expecting to churn this again to use something more suitable than iommu_ops as the private data, I think keeping things simple wins over const-correctness for now.

Thanks,
Robin

--->8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001 Message-Id:
<b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/of_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 12 ++----------
 2 files changed, 42 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +40 -0

index 73236d3..c7078f6 100644
@@ -94,6 +94,46 @@ int of_get_dma_window(struct device_node *dn, const char
 *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
+struct of_iommu_node {
+       struct list_head list;
+       struct device_node *np;
+       struct iommu_ops *ops;
+};
+static LIST_HEAD(of_iommu_list);
+static DEFINE_SPINLOCK(of_iommu_lock);
+
+void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops)
+{
+       struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
+
+       if (!iommu) {
+               __WARN();
+               return;
+       }
+
+       INIT_LIST_HEAD(&iommu->list);
+       iommu->np = np;
+       iommu->ops = ops;
+       spin_lock(&of_iommu_lock);
+       list_add_tail(&iommu->list, &of_iommu_list);
+       spin_unlock(&of_iommu_lock);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       struct of_iommu_node *node;
+       struct iommu_ops *ops = NULL;
+
+       spin_lock(&of_iommu_lock);
+       list_for_each_entry(node, &of_iommu_list, list)
+               if (node->np == np) {
+                       ops = node->ops;
+                       break;
+               }
+       spin_unlock(&of_iommu_lock);
+       return ops;
+}
+
 struct iommu_ops *of_iommu_configure(struct device *dev)
 {

        struct of_phandle_args iommu_spec;

include/linux/of_iommu.h +2 -11

index d03abbb..16c7554 100644
@@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct
 device *dev)
 
 #endif /* CONFIG_OF_IOMMU */
 
-static inline void of_iommu_set_ops(struct device_node *np,
-                                   const struct iommu_ops *ops)
-{
-       np->data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
-       return np->data;
-}
+void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);
 
 extern struct of_device_id __iommu_of_table;
 
--

1.9.1

Quoted 171 lines · Grant Likely

Quoted 48 lines · Robin Murphy

Hi Arnd,

On 03/12/14 19:57, Arnd Bergmann wrote:
[...]

Quoted 11 lines

Quoted 8 lines

Good catch. This is not good. The data pointer should be avoided since there are no controls over its use. Until a better solution can be implemented, probably the safest thing to do is add a struct iommu_ops pointer to struct device_node. However, assuming that only a small portion of nodes will actually have iommu_ops set, I'd rather see a separate registry that matches device_nodes to iommu_ops.

Quoted 3 lines

Fair enough. Will, can you take a copy of drivers/dma/of-dma.c and adapt it as needed? It should be exactly what we need to start out and can be extended and generalized later.

I'm quite keen to see this series go in, since I'm depending on it to make
arm64 IOMMU DMA ops "just work". Will and I came to the conclusion the other
day that we pretty much need to build up some kind of bus abstraction based
on the probe data in order to be able to assign IOMMU groups correctly, which can also subsume this particular problem in the long run. Since I've
made a start on that already, I've hacked the following short-term fix out
of it. Tested on my Juno - admittedly with only two SMMUs and one master (EHCI) being probed, but it didn't blow up or regress anything.

Regards,
Robin.

--->8---
From 1f3d2612682c239e53f2c20e2ac5d19ef3f5387c Mon Sep 17 00:00:00 2001
Message-Id:

<1f3d2612682c239e53f2c20e2ac5d19ef3f5387c.1417695078.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH] iommu: store DT-probed IOMMU data privately

Quoted 5 lines

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

Looks reasonable to me. Comments below...

Quoted 15 lines

---
  drivers/iommu/of_iommu.c | 38 ++++++++++++++++++++++++++++++++++++++
  include/linux/of_iommu.h | 12 ++----------
  2 files changed, 40 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +0 -0

index 73236d3..5cd451c 100644
@@ -94,6 +94,44 @@ int of_get_dma_window(struct device_node *dn, const

char
*prefix, int index,
}
EXPORT_SYMBOL_GPL(of_get_dma_window);

drivers/iommu/of_iommu.c

Quoted 6 lines

drivers/iommu/of_iommu.c +6 -0

+struct of_iommu_node {
+       struct hlist_node list;
+       struct device_node *np;
+       const struct iommu_ops *ops;
+};
+static HLIST_HEAD(of_iommu_list);

Just use a list_head. hlist_head merely saves one pointer in this case.

Quoted 7 lines

drivers/iommu/of_iommu.c +3 -0

+static DEFINE_SPINLOCK(of_iommu_lock);
+
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops

*ops)

drivers/iommu/of_iommu.c +2 -0

+{
+       struct of_iommu_node *iommu = kmalloc(sizeof(*iommu),

GFP_KERNEL);

kzalloc()

drivers/iommu/of_iommu.c

Quoted 3 lines

drivers/iommu/of_iommu.c +3 -0

+
+       if (!iommu)
+               return;

Shouldn't there be a WARN() here on failure? I don't think failing silently is desired.

Quoted 12 lines

drivers/iommu/of_iommu.c +12 -0

+
+       INIT_HLIST_NODE(&iommu->list);
+       iommu->np = np;
+       iommu->ops = ops;
+       spin_lock(&of_iommu_lock);
+       hlist_add_head(&iommu->list, &of_iommu_list);
+       spin_unlock(&of_iommu_lock);
+}
+
+struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+       struct of_iommu_node *node;

drivers/iommu/of_iommu.c

Quoted 10 lines

drivers/iommu/of_iommu.c +10 -0

+       const struct iommu_ops *ops = NULL;
+
+       spin_lock(&of_iommu_lock);
+       hlist_for_each_entry(node, &of_iommu_list, list)
+               if (node->np == np) {
+                       ops = node->ops;
+                       break;
+               }
+       spin_unlock(&of_iommu_lock);
+       return (struct iommu_ops *)ops;

The cast looks fishy. If you need to use a cast, then the data types are probably wrong. If you drop the const from *ops here and in the structure then it should probably work fine. Due to the way it is being used, there isn't any advantage to using const (unless you changes of_iommu_get_ops() to return a const pointer, then const would make sense).

Quoted 35 lines

drivers/iommu/of_iommu.c +2 -0

+}
+
  struct iommu_ops *of_iommu_configure(struct device *dev)
  {
         struct of_phandle_args iommu_spec;

include/linux/of_iommu.h +2 -10

index d03abbb..e27c53a 100644
@@ -31,16 +31,8 @@ static inline struct iommu_ops
 *of_iommu_configure(struct
 device *dev)
 
  #endif /* CONFIG_OF_IOMMU */
 
-static inline void of_iommu_set_ops(struct device_node *np,
-                                   const struct iommu_ops *ops)
-{
-       np->data = (struct iommu_ops *)ops;
-}
-
-static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
-       return np->data;
-}
+void of_iommu_set_ops(struct device_node *np, const struct iommu_ops
 *ops);
+struct iommu_ops *of_iommu_get_ops(struct device_node *np);

extern struct of_device_id __iommu_of_table;

--
1.9.1

Hi Grant, thanks for the advice - silly micro-optimisations removed, and I'll make a note to do so from my in-development code, too ;)

I didn't much like the casting either, so rather than push it elsewhere or out to the caller I've just changed the prototype to obviate it completely. Since we're also expecting to churn this again to use something more suitable than iommu_ops as the private data, I think keeping things simple wins over const-correctness for now.

Thanks,
Robin

--->8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
Message-Id: <b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/of_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 12 ++----------
 2 files changed, 42 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +0 -0

index 73236d3..c7078f6 100644

Quoted 38 lines · Robin Murphy

Hi Grant, thanks for the advice - silly micro-optimisations removed, and I'll make a note to do so from my in-development code, too ;)

I didn't much like the casting either, so rather than push it elsewhere or out to the caller I've just changed the prototype to obviate it completely. Since we're also expecting to churn this again to use something more suitable than iommu_ops as the private data, I think keeping things simple wins over const-correctness for now.

Thanks,
Robin

--->8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
Message-Id: <b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/of_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
  include/linux/of_iommu.h | 12 ++----------
  2 files changed, 42 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +0 -0

index 73236d3..c7078f6 100644
@@ -94,6 +94,46 @@ int of_get_dma_window(struct device_node *dn, const char
 *prefix, int index,
  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
 

drivers/iommu/of_iommu.c int of_get_dma_window(struct device_node *dn, const char · line 101

Quoted 4 lines

drivers/iommu/of_iommu.c +4 -0

+struct of_iommu_node {
+	struct list_head list;
+	struct device_node *np;
+	struct iommu_ops *ops;

Why can't this be const? Why would anyone ever need to modify it? Also drivers do define their iommu_ops structures const these days, so you'll either still have to cast away at some point or the compiler will complain once you start calling this from drivers.

Hi Thierry,

Quoted 46 lines · Thierry Reding

Quoted 44 lines · Robin Murphy

Hi Grant, thanks for the advice - silly micro-optimisations removed, and I'll make a note to do so from my in-development code, too ;)

I didn't much like the casting either, so rather than push it elsewhere or
out to the caller I've just changed the prototype to obviate it completely.
Since we're also expecting to churn this again to use something more suitable than iommu_ops as the private data, I think keeping things simple
wins over const-correctness for now.

Thanks,
Robin

--->8---
From b2e8c91ac49bef4008661e4628cd6b7249d84af5 Mon Sep 17 00:00:00 2001
Message-Id:
<b2e8c91ac49bef4008661e4628cd6b7249d84af5.1417698001.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Thu, 4 Dec 2014 11:53:13 +0000
Subject: [PATCH v2] iommu: store DT-probed IOMMU data privately

Since the data pointer in the DT node is public and may be overwritten by conflicting code, move the DT-probed IOMMU ops to a private list where they will be safe.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/of_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
  include/linux/of_iommu.h | 12 ++----------
  2 files changed, 42 insertions(+), 10 deletions(-)

drivers/iommu/of_iommu.c +0 -0

index 73236d3..c7078f6 100644
@@ -94,6 +94,46 @@ int of_get_dma_window(struct device_node *dn, const

char
*prefix, int index,
}
EXPORT_SYMBOL_GPL(of_get_dma_window);

Quoted 11 lines

drivers/iommu/of_iommu.c

Quoted 4 lines

drivers/iommu/of_iommu.c +4 -0

+struct of_iommu_node {
+       struct list_head list;
+       struct device_node *np;
+       struct iommu_ops *ops;

Why can't this be const? Why would anyone ever need to modify it? Also drivers do define their iommu_ops structures const these days, so you'll either still have to cast away at some point or the compiler will complain once you start calling this from drivers.

...whereas if we make everything const then the drivers that _do_ want to use the private data introduced by this series and thus pass mutable per-instance iommu_ops will be the ones having to cast. We have no users either way until this series is merged, and the need to stash the per-instance data somewhere other than np->data is in the way of getting it merged - this is just a quick hack to address that. I think we've already agreed that mutable per-instance iommu_ops holding private data aren't particularly pleasant and will (hopefully) go away in the next iteration[1], at which point this all changes anyway.

Quoted 12 lines

Quoted 11 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct of_iommu_node {
+       struct list_head list;
+       struct device_node *np;
+       struct iommu_ops *ops;

Why can't this be const? Why would anyone ever need to modify it? Also drivers do define their iommu_ops structures const these days, so you'll either still have to cast away at some point or the compiler will complain once you start calling this from drivers.

Quoted 9 lines

...whereas if we make everything const then the drivers that _do_ want to use the private data introduced by this series and thus pass mutable per-instance iommu_ops will be the ones having to cast. We have no users either way until this series is merged, and the need to stash the per-instance data somewhere other than np->data is in the way of getting it merged - this is just a quick hack to address that. I think we've already agreed that mutable per-instance iommu_ops holding private data aren't particularly pleasant and will (hopefully) go away in the next iteration[1], at which point this all changes anyway.

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

On 04/12/14 17:58, Grant Likely wrote:
[...]

Quoted 22 lines

Quoted 12 lines

Quoted 11 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct of_iommu_node {
+       struct list_head list;
+       struct device_node *np;
+       struct iommu_ops *ops;

Why can't this be const? Why would anyone ever need to modify it? Also drivers do define their iommu_ops structures const these days, so you'll either still have to cast away at some point or the compiler will complain once you start calling this from drivers.

Quoted 9 lines

...whereas if we make everything const then the drivers that _do_ want to use the private data introduced by this series and thus pass mutable per-instance iommu_ops will be the ones having to cast. We have no users either way until this series is merged, and the need to stash the per-instance data somewhere other than np->data is in the way of getting it merged - this is just a quick hack to address that. I think we've already agreed that mutable per-instance iommu_ops holding private data aren't particularly pleasant and will (hopefully) go away in the next iteration[1], at which point this all changes anyway.

Quoted 5 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

Quoted 30 lines · Robin Murphy

On 04/12/14 17:58, Grant Likely wrote:
[...]

Quoted 22 lines

Quoted 12 lines

Quoted 11 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct of_iommu_node {
+       struct list_head list;
+       struct device_node *np;
+       struct iommu_ops *ops;

Why can't this be const? Why would anyone ever need to modify it? Also drivers do define their iommu_ops structures const these days, so you'll either still have to cast away at some point or the compiler will complain once you start calling this from drivers.

Quoted 9 lines

...whereas if we make everything const then the drivers that _do_ want to use the private data introduced by this series and thus pass mutable per-instance iommu_ops will be the ones having to cast. We have no users either way until this series is merged, and the need to stash the per-instance data somewhere other than np->data is in the way of getting it merged - this is just a quick hack to address that. I think we've already agreed that mutable per-instance iommu_ops holding private data aren't particularly pleasant and will (hopefully) go away in the next iteration[1], at which point this all changes anyway.

Quoted 5 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Quoted 6 lines

Quoted 5 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Quoted 17 lines

Quoted 8 lines

Quoted 6 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

Quoted 5 lines

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive.

Quoted 3 lines

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D

Quoted 39 lines · Robin Murphy

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Quoted 17 lines

Quoted 8 lines

Quoted 6 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

Quoted 5 lines

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive.

Quoted 3 lines

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Quoted 2 lines

Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D

I've already acked it. Why are we still talking about it? :-D

Quoted 40 lines · Grant Likely

Quoted 39 lines · Robin Murphy

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Quoted 17 lines

Quoted 8 lines

Quoted 6 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

Quoted 5 lines

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive.

Quoted 3 lines

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Quoted 4 lines

Quoted 2 lines

Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D

I've already acked it. Why are we still talking about it? :-D

Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle.

If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway?

Quoted 51 lines · Thierry Reding

Quoted 40 lines · Grant Likely

Quoted 39 lines · Robin Murphy

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Quoted 17 lines

Quoted 8 lines

Quoted 6 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

Quoted 5 lines

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive.

Quoted 3 lines

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Quoted 4 lines

Quoted 2 lines

Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D

I've already acked it. Why are we still talking about it? :-D

Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle.

Quoted 2 lines

If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway?

I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging.

Quoted 37 lines · Grant Likely

Quoted 36 lines · Robin Murphy

Hi Will,

On 05/12/14 12:10, Will Deacon wrote:
[...]

Quoted 14 lines

Quoted 6 lines

Quoted 5 lines

Do you expect drivers to modify that *priv pointer after the ops structure is registered? I'd be very surprised if that was the use case. It's fine for the driver to register a non-const version, but once it is registered, the infrastructure can treat it as const from then on.

Quoted 7 lines

Possibly not - certainly my current port of the ARM SMMU which makes use of *priv is only ever reading it - although we did also wave around reasons for mutable ops like dynamically changing the pgsize_bitmap and possibly even swizzling individual ops for runtime reconfiguration. On consideration though, I'd agree that things like that are mad enough to stay well within individual drivers if they did ever happen, and certainly shouldn't apply to this bit of the infrastructure at any rate.

Quoted 5 lines

I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help.

We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive.

Quoted 3 lines

Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon.

Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D

Quoted 1 line

I've already acked it. Why are we still talking about it? :-D

Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle.

If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway?

set_arch_dma_coherent_ops is called from of_dma_configure in order to swizzle the architectural dma-mapping functions over to a cache-coherent implementation. This is currently implemented only for ARM.

In anticipation of re-using this mechanism for IOMMU-backed dma-mapping ops too, this patch replaces the function with a broader arch_setup_dma_ops callback which will be extended in future.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/include/asm/dma-mapping.h |  8 ++++----
 drivers/of/platform.c              | 31 +++++++++++++------------------
 include/linux/dma-mapping.h        |  7 ++-----
 3 files changed, 19 insertions(+), 27 deletions(-)

arch/arm/include/asm/dma-mapping.h +4 -4

index 85738b200023..dc3420e77758 100644
@@ -121,12 +121,12 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline int set_arch_dma_coherent_ops(struct device *dev)
+static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
-       set_dma_ops(dev, &arm_coherent_dma_ops);
-       return 0;
+       if (coherent)
+               set_dma_ops(dev, &arm_coherent_dma_ops);
 }
-#define set_arch_dma_coherent_ops(dev) set_arch_dma_coherent_ops(dev)
+#define arch_setup_dma_ops arch_setup_dma_ops
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {

drivers/of/platform.c +2 -9

index 3b64d0bf5bba..ff1f4e9afccb 100644
@@ -164,6 +164,8 @@ static void of_dma_configure(struct device *dev)
 {
        u64 dma_addr, paddr, size;
        int ret;
+       bool coherent;
+       unsigned long offset;
 
        /*
         * Set default dma-mask to 32 bit. Drivers are expected to setup

@@ -178,28 +180,21 @@ static void of_dma_configure(struct device *dev)
        if (!dev->dma_mask)
                dev->dma_mask = &dev->coherent_dma_mask;
 
-       /*
-        * if dma-coherent property exist, call arch hook to setup
-        * dma coherent operations.
-        */
-       if (of_dma_is_coherent(dev->of_node)) {
-               set_arch_dma_coherent_ops(dev);
-               dev_dbg(dev, "device is dma coherent\n");
-       }
-

drivers/of/platform.c +9 -3

+       } else {
+               offset = PFN_DOWN(paddr - dma_addr);
+               dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
        }
+       dev->dma_pfn_offset = offset;
+
+       coherent = of_dma_is_coherent(dev->of_node);
+       dev_dbg(dev, "device is%sdma coherent\n",
+               coherent ? " " : " not ");
 
-       /* DMA ranges found. Calculate and set dma_pfn_offset */
-       dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
-       dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+       arch_setup_dma_ops(dev, coherent);
 }
 
 /**

include/linux/dma-mapping.h +2 -6

index d5d388160f42..8a1560f95d4a 100644
@@ -129,11 +129,8 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 
 extern u64 dma_get_required_mask(struct device *dev);
 
-#ifndef set_arch_dma_coherent_ops
-static inline int set_arch_dma_coherent_ops(struct device *dev)
-{
-       return 0;
-}
+#ifndef arch_setup_dma_ops
+static inline void arch_setup_dma_ops(struct device *dev, bool coherent) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
--

2.1.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 17 lines

drivers/of/platform.c +0 -13

@@ -178,28 +180,21 @@ static void of_dma_configure(struct device *dev)
        if (!dev->dma_mask)
                dev->dma_mask = &dev->coherent_dma_mask;
 
-       /*
-        * if dma-coherent property exist, call arch hook to setup
-        * dma coherent operations.
-        */
-       if (of_dma_is_coherent(dev->of_node)) {
-               set_arch_dma_coherent_ops(dev);
-               dev_dbg(dev, "device is dma coherent\n");
-       }
-
-       /*
-        * if dma-ranges property doesn't exist - just return else
-        * setup the dma offset
-        */

drivers/of/platform.c static void of_dma_configure(struct device *dev) · line 186

Quoted 6 lines

drivers/of/platform.c +2 -2

        ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
        if (ret < 0) {
-               dev_dbg(dev, "no dma range information to setup\n");
-               return;
+               dma_addr = offset = 0;
+               size = dev->coherent_dma_mask;

It looks like size is not used.

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 21 ++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 24 insertions(+), 9 deletions(-)

arch/arm/include/asm/dma-mapping.h +3 -1

index dc3420e77758..f3c0d953f6a2 100644
@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent)
 {
        if (coherent)
                set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +14 -7

index ff1f4e9afccb..b89caf8c7586 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct device *dev)
        int ret;
        bool coherent;
        unsigned long offset;
+       struct iommu_ops *iommu;
 
        /*
         * Set default dma-mask to 32 bit. Drivers are expected to setup

@@ -194,7 +196,16 @@ static void of_dma_configure(struct device *dev)
        dev_dbg(dev, "device is%sdma coherent\n",
                coherent ? " " : " not ");
 
-       arch_setup_dma_ops(dev, coherent);
+       iommu = of_iommu_configure(dev);
+       dev_dbg(dev, "device is%sbehind an iommu\n",
+               iommu ? " " : " not ");
+
+       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+}
+
+static void of_dma_deconfigure(struct device *dev)
+{
+       arch_teardown_dma_ops(dev);
 }
 
 /**

@@ -223,16 +234,12 @@ static struct platform_device *of_platform_device_create_pdata(
        if (!dev)
                goto err_clear_flag;
 
-       of_dma_configure(&dev->dev);
        dev->dev.bus = &platform_bus_type;
        dev->dev.platform_data = platform_data;
-
-       /* We do not fill the DMA ops for platform devices by default.
-        * This is currently the responsibility of the platform code
-        * to do such, possibly using a device notifier
-        */
+       of_dma_configure(&dev->dev);
 
        if (of_device_add(dev) != 0) {
+               of_dma_deconfigure(&dev->dev);
                platform_device_put(dev);
                goto err_clear_flag;
        }

include/linux/dma-mapping.h +7 -2

index 8a1560f95d4a..c3007cb4bfa6 100644
@@ -130,7 +130,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef arch_setup_dma_ops
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent) { }
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent) { }
+#endif
+
+#ifndef arch_teardown_dma_ops
+static inline void arch_teardown_dma_ops(struct device *dev) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
--

2.1.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 21 ++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 24 insertions(+), 9 deletions(-)

arch/arm/include/asm/dma-mapping.h +3 -1

index dc3420e77758..f3c0d953f6a2 100644
@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent)
 {
        if (coherent)
                set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index ff1f4e9afccb..b89caf8c7586 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct device *dev)
        int ret;
        bool coherent;
        unsigned long offset;
+       struct iommu_ops *iommu;
 
        /*
         * Set default dma-mask to 32 bit. Drivers are expected to setup

drivers/of/platform.c +7 -6

+}
+
+static void of_dma_deconfigure(struct device *dev)
+{
+       arch_teardown_dma_ops(dev);
 }
 
 /**

@@ -223,16 +234,12 @@ static struct platform_device *of_platform_device_create_pdata(
        if (!dev)
                goto err_clear_flag;
 
-       of_dma_configure(&dev->dev);
        dev->dev.bus = &platform_bus_type;
        dev->dev.platform_data = platform_data;
-
-       /* We do not fill the DMA ops for platform devices by default.
-        * This is currently the responsibility of the platform code
-        * to do such, possibly using a device notifier
-        */
+       of_dma_configure(&dev->dev);
 
        if (of_device_add(dev) != 0) {
+               of_dma_deconfigure(&dev->dev);
                platform_device_put(dev);
                goto err_clear_flag;
        }

include/linux/dma-mapping.h +7 -2

index 8a1560f95d4a..c3007cb4bfa6 100644
@@ -130,7 +130,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 extern u64 dma_get_required_mask(struct device *dev);
 
 #ifndef arch_setup_dma_ops
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent) { }
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent) { }
+#endif
+
+#ifndef arch_teardown_dma_ops
+static inline void arch_teardown_dma_ops(struct device *dev) { }
 #endif
 
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
--

2.1.1

_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Quoted 52 lines · Will Deacon

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/include/asm/dma-mapping.h |  4 +++-
 drivers/of/platform.c              | 21 ++++++++++++++-------
 include/linux/dma-mapping.h        |  8 +++++++-
 3 files changed, 24 insertions(+), 9 deletions(-)

arch/arm/include/asm/dma-mapping.h +3 -1

index dc3420e77758..f3c0d953f6a2 100644
@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent)
 {
        if (coherent)
                set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index ff1f4e9afccb..b89caf8c7586 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct device *dev)
        int ret;
        bool coherent;
        unsigned long offset;
+       struct iommu_ops *iommu;
 
        /*
         * Set default dma-mask to 32 bit. Drivers are expected to setup

@@ -194,7 +196,16 @@ static void of_dma_configure(struct device *dev)
        dev_dbg(dev, "device is%sdma coherent\n",
                coherent ? " " : " not ");
 

drivers/of/platform.c static void of_dma_configure(struct device *dev) · line 203

Quoted 6 lines

drivers/of/platform.c +5 -1

-       arch_setup_dma_ops(dev, coherent);
+       iommu = of_iommu_configure(dev);
+       dev_dbg(dev, "device is%sbehind an iommu\n",
+               iommu ? " " : " not ");
+
+       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);

so, what is the way for a driver that explicitly wants to manage it's own device virtual address space to opt out of this? I suspect that won't be the common case, but for a gpu, if dma layer all of a sudden thinks it is in control of the gpu's virtual address space, things are going to end in tears..

Quoted 53 lines · Rob Clark

Quoted 52 lines · Will Deacon

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
  arch/arm/include/asm/dma-mapping.h |  4 +++-
  drivers/of/platform.c              | 21 ++++++++++++++-------
  include/linux/dma-mapping.h        |  8 +++++++-
  3 files changed, 24 insertions(+), 9 deletions(-)

arch/arm/include/asm/dma-mapping.h +3 -1

index dc3420e77758..f3c0d953f6a2 100644
@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
  }
  #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent)
  {
         if (coherent)
                 set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +2 -0

index ff1f4e9afccb..b89caf8c7586 100644
@@ -19,6 +19,7 @@
  #include <linux/slab.h>
  #include <linux/of_address.h>
  #include <linux/of_device.h>
+#include <linux/of_iommu.h>
  #include <linux/of_irq.h>
  #include <linux/of_platform.h>
  #include <linux/platform_device.h>

@@ -166,6 +167,7 @@ static void of_dma_configure(struct device *dev)
         int ret;
         bool coherent;
         unsigned long offset;
+       struct iommu_ops *iommu;
 
         /*
          * Set default dma-mask to 32 bit. Drivers are expected to setup

@@ -194,7 +196,16 @@ static void of_dma_configure(struct device *dev)
         dev_dbg(dev, "device is%sdma coherent\n",
                 coherent ? " " : " not ");
 

Quoted 13 lines

drivers/of/platform.c static void of_dma_configure(struct device *dev) · line 203

Quoted 6 lines

drivers/of/platform.c +5 -1

-       arch_setup_dma_ops(dev, coherent);
+       iommu = of_iommu_configure(dev);
+       dev_dbg(dev, "device is%sbehind an iommu\n",
+               iommu ? " " : " not ");
+
+       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);

so, what is the way for a driver that explicitly wants to manage it's own device virtual address space to opt out of this? I suspect that won't be the common case, but for a gpu, if dma layer all of a sudden thinks it is in control of the gpu's virtual address space, things are going to end in tears..

I think you'll need to detach from the DMA domain, then have the driver manage everything itself. As you say, it's not the common case, so you may need to add some hooks for detaching from the default domain and swizzling your DMA ops.

Quoted 86 lines · Will Deacon

Quoted 72 lines · Rob Clark

Quoted 71 lines · Will Deacon

This patch extends of_dma_configure so that it sets up the IOMMU for a device, as well as the coherent/non-coherent DMA mapping ops.

Acked-by Arnd Bergmann <[email protected]>

Acked-by Marek Szyprowski <[email protected]>

Tested-by Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

  arch/arm/include/asm/dma-mapping.h |  4 +++-
  drivers/of/platform.c              | 21 ++++++++++++++-------
  include/linux/dma-mapping.h        |  8 +++++++-
  3 files changed, 24 insertions(+), 9 deletions(-)

(quoted diff) +0 -0

b/arch/arm/include/asm/dma-mapping.h index dc3420e77758..f3c0d953f6a2 100644

arch/arm/include/asm/dma-mapping.h +0 -0

@@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct

device *dev)>>>
}
#define dma_max_pfn(dev) dma_max_pfn(dev)

-static inline void arch_setup_dma_ops(struct device *dev, bool coherent)

arch/arm/include/asm/dma-mapping.h +3 -0

+static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
+                                     u64 size, struct iommu_ops *iommu,
+                                     bool coherent)
 
  {
 
         if (coherent)
 
                 set_dma_ops(dev, &arm_coherent_dma_ops);

drivers/of/platform.c +1 -0

index ff1f4e9afccb..b89caf8c7586 100644
@@ -19,6 +19,7 @@
 
  #include <linux/slab.h>
  #include <linux/of_address.h>
  #include <linux/of_device.h>
 
+#include <linux/of_iommu.h>
 

  #include <linux/of_irq.h>
  #include <linux/of_platform.h>
  #include <linux/platform_device.h>

drivers/of/platform.c +1 -0

@@ -166,6 +167,7 @@ static void of_dma_configure(struct device *dev)
 
         int ret;
         bool coherent;
         unsigned long offset;
 
+       struct iommu_ops *iommu;
 

/*

* Set default dma-mask to 32 bit. Drivers are expected to setup

drivers/of/platform.c +0 -0

@@ -194,7 +196,16 @@ static void of_dma_configure(struct device *dev)
 
         dev_dbg(dev, "device is%sdma coherent\n",
 
                 coherent ? " " : " not ");
 

Quoted 12 lines

drivers/of/platform.c static void of_dma_configure(struct device *dev) · line 203

Quoted 6 lines

drivers/of/platform.c +5 -1

-       arch_setup_dma_ops(dev, coherent);
+       iommu = of_iommu_configure(dev);
+       dev_dbg(dev, "device is%sbehind an iommu\n",
+               iommu ? " " : " not ");
+
+       arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);

so, what is the way for a driver that explicitly wants to manage it's own device virtual address space to opt out of this? I suspect that won't be the common case, but for a gpu, if dma layer all of a sudden thinks it is in control of the gpu's virtual address space, things are going to end in tears..

Quoted 4 lines

I think you'll need to detach from the DMA domain, then have the driver manage everything itself. As you say, it's not the common case, so you may need to add some hooks for detaching from the default domain and swizzling your DMA ops.