IOMMU: Tegra GART driver clean up and optimization

Remove unneeded headers inclusion and sort the headers in alphabet order. Remove pr_fmt macro since there is no pr_*() in the code and it doesn't affect dev_*() functions.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Properly clean up allocated resources on the drivers probe failure and remove unneeded checks.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

GART can't handle all devices, hence ignore devices that aren't related to GART. IOMMU phandle must be explicitly assign to devices in the device tree.

Quoted 3 lines · Dmitry Osipenko

GART can't handle all devices, hence ignore devices that aren't related to GART. IOMMU phandle must be explicitly assign to devices in the device tree.

I think technically the GART can indeed handle all devices since it is just a physical address region that can be used to remap other physical addresses. That's not to say that doing so would be a good idea. So the commit message here is slightly confusing, but other than that the idea is good, so:

Acked-by Thierry Reding <[email protected]>

Introduce iotlb_sync_map() callback that is invoked in the end of iommu_map(). This new callback allows IOMMU drivers to avoid syncing after mapping of each contiguous chunk and sync only when the whole mapping is completed, optimizing performance of the mapping operation.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Robin Murphy <[email protected]>

---
 drivers/iommu/iommu.c | 8 ++++++--
 include/linux/iommu.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Currently GART writes one page entry at a time. More optimal would be to aggregate the writes and flush BUS buffer in the end, this gives map/unmap 10-40% performance boost (depending on size of mapping) in comparison to flushing after each page entry update.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Splitting GART and Memory Controller wasn't a good decision that was made back in the day. Given that the GART driver wasn't ever been used by anything in the kernel, we decided that it will be better to correct the mistakes of the past and merge two bindings into a single one. As a result there is a DT ABI change for the Memory Controller that allows not to break newer kernels using older DT and not to break older kernels using newer DT, that is done by changing the 'compatible' of the node to 'tegra20-mc-gart' and adding a new-required clock property. The new clock property also puts the tegra20-mc binding in line with the bindings of the later Tegra generations.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/iommu/nvidia,tegra20-gart.txt    | 14 ----------
 .../memory-controllers/nvidia,tegra20-mc.txt  | 27 +++++++++++++------
 2 files changed, 19 insertions(+), 22 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iommu/nvidia,tegra20-gart.txt

Documentation/devicetree/bindings/iommu/nvidia,tegra20-gart.txt deleted +0 -14

deleted file mode 100644
index 099d9362ebc1..000000000000
@@ -1,14 +0,0 @@
-NVIDIA Tegra 20 GART
-
-Required properties:
-- compatible: "nvidia,tegra20-gart"
-- reg: Two pairs of cells specifying the physical address and size of
-  the memory controller registers and the GART aperture respectively.
-
-Example:
-
-	gart {
-		compatible = "nvidia,tegra20-gart";
-		reg = <0x7000f024 0x00000018	/* controller registers */
-		       0x58000000 0x02000000>;	/* GART aperture */
-	};

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-mc.txt +0 -0

index 7d60a50a4fa1..e55328237df4 100644

Quoted 45 lines · Dmitry Osipenko

Splitting GART and Memory Controller wasn't a good decision that was made back in the day. Given that the GART driver wasn't ever been used by anything in the kernel, we decided that it will be better to correct the mistakes of the past and merge two bindings into a single one. As a result there is a DT ABI change for the Memory Controller that allows not to break newer kernels using older DT and not to break older kernels using newer DT, that is done by changing the 'compatible' of the node to 'tegra20-mc-gart' and adding a new-required clock property. The new clock property also puts the tegra20-mc binding in line with the bindings of the later Tegra generations.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/iommu/nvidia,tegra20-gart.txt    | 14 ----------
 .../memory-controllers/nvidia,tegra20-mc.txt  | 27 +++++++++++++------
 2 files changed, 19 insertions(+), 22 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iommu/nvidia,tegra20-gart.txt

Documentation/devicetree/bindings/iommu/nvidia,tegra20-gart.txt deleted +0 -14

deleted file mode 100644
index 099d9362ebc1..000000000000
@@ -1,14 +0,0 @@
-NVIDIA Tegra 20 GART
-
-Required properties:
-- compatible: "nvidia,tegra20-gart"
-- reg: Two pairs of cells specifying the physical address and size of
-  the memory controller registers and the GART aperture respectively.
-
-Example:
-
-	gart {
-		compatible = "nvidia,tegra20-gart";
-		reg = <0x7000f024 0x00000018	/* controller registers */
-		       0x58000000 0x02000000>;	/* GART aperture */
-	};

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-mc.txt +0 -0

index 7d60a50a4fa1..e55328237df4 100644
@@ -1,26 +1,37 @@
 NVIDIA Tegra20 MC(Memory Controller)
 

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-mc.txt line 6

Quoted 8 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra20-mc.txt +3 -4

 Required properties:
-- compatible : "nvidia,tegra20-mc"
-- reg : Should contain 2 register ranges(address and length); see the
-  example below. Note that the MC registers are interleaved with the
-  GART registers, and hence must be represented as multiple ranges.
+- compatible : "nvidia,tegra20-mc-gart"
+- reg : Should contain 2 register ranges: physical base address and length of
+  the controller's registers and the GART aperture respectively.

Couldn't we have achieved the same thing by adding a reg-names property instead of using a different compatible string? After all we only change what information the DT provides, but the device is still a "tegra20-mc" device.

Splitting GART and Memory Controller wasn't a good decision that was made back in the day. Given that the GART driver wasn't ever been used by anything in the kernel, we decided that it will be better to correct the mistakes of the past and merge two bindings into a single one. As a result there is a DT ABI change for the Memory Controller that allows not to break newer kernels using older DT and not to break older kernels using newer DT, that is done by changing the 'compatible' of the node to 'tegra20-mc-gart' and adding a new-required clock property. The new clock property also puts the tegra20-mc binding in line with the bindings of the later Tegra generations.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/iommu/nvidia,tegra20-gart.txt    | 14 ----------
 .../memory-controllers/nvidia,tegra20-mc.txt  | 27 +++++++++++++------
 2 files changed, 19 insertions(+), 22 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iommu/nvidia,tegra20-gart.txt

The tegra20-mc device-tree binding has been changed, GART has been squashed into Memory Controller and now the clock property is mandatory for Tegra20, the DT compatible has been changed as well. Adapt driver to the DT changes.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/memory/tegra/mc.c | 21 ++++++++-------------
 drivers/memory/tegra/mc.h |  6 ------
 include/soc/tegra/mc.h    |  2 +-
 3 files changed, 9 insertions(+), 20 deletions(-)

drivers/memory/tegra/mc.c +0 -0

index e56862495f36..1b4ceefd82f9 100644

Quoted 17 lines · Dmitry Osipenko

The tegra20-mc device-tree binding has been changed, GART has been squashed into Memory Controller and now the clock property is mandatory for Tegra20, the DT compatible has been changed as well. Adapt driver to the DT changes.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/memory/tegra/mc.c | 21 ++++++++-------------
  drivers/memory/tegra/mc.h |  6 ------
  include/soc/tegra/mc.h    |  2 +-
  3 files changed, 9 insertions(+), 20 deletions(-)

drivers/memory/tegra/mc.c +0 -0

index e56862495f36..1b4ceefd82f9 100644

drivers/memory/tegra/mc.c line 54

Quoted 5 lines

drivers/memory/tegra/mc.c +1 -1

@@ -51,7 +51,7 @@
 
  static const struct of_device_id tegra_mc_of_match[] = {
  #ifdef CONFIG_ARCH_TEGRA_2x_SOC
-	{ .compatible = "nvidia,tegra20-mc", .data = &tegra20_mc_soc },
+	{ .compatible = "nvidia,tegra20-mc-gart", .data = &tegra20_mc_soc },

Technically we now regress because we no longer support the older device tree bindings. I know that it doesn't really matter because this driver doesn't really do much interesting yet other than reporting memory access violations, but if that's enough to warrant a change of the compatible string, then I think we also need to preserve compatibility in the code.

That said, I think compatibility would be easier to preserve if we stuck with the old compatible string and used a "reg-names" property to specify which version of the binding we're referring to.

For example, we could have:

	memory-controller@7000f000 {
		compatible = "nvidia,tegra20-mc";
		reg = <0x7000f000 0x024
		       0x7000f03c 0x3c4>;
		...
	};

for the old binding and:

	memory-controller@7000f000 {
		compatible = "nvidia,tegra20-mc";
		reg = <0x7000f000 0x00000400>,
		      <0x58000000 0x02000000>;
		reg-names = "mc", "gart";
		...
	};

for the new binding. The driver can then easily check for the existence of the reg-names property and take the legacy or new code paths.

Quoted 54 lines · Thierry Reding

Quoted 17 lines · Dmitry Osipenko

The tegra20-mc device-tree binding has been changed, GART has been squashed into Memory Controller and now the clock property is mandatory for Tegra20, the DT compatible has been changed as well. Adapt driver to the DT changes.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/memory/tegra/mc.c | 21 ++++++++------------- drivers/memory/tegra/mc.h |  6 ------ include/soc/tegra/mc.h    |  2 +-

  3 files changed, 9 insertions(+), 20 deletions(-)

drivers/memory/tegra/mc.c +0 -0

index e56862495f36..1b4ceefd82f9 100644

drivers/memory/tegra/mc.c line 54

Quoted 5 lines

drivers/memory/tegra/mc.c +1 -1

@@ -51,7 +51,7 @@
     static const struct of_device_id tegra_mc_of_match[] = {
   #ifdef CONFIG_ARCH_TEGRA_2x_SOC
-    { .compatible = "nvidia,tegra20-mc", .data = &tegra20_mc_soc },
+    { .compatible = "nvidia,tegra20-mc-gart", .data =
 &tegra20_mc_soc },

Technically we now regress because we no longer support the older device tree bindings. I know that it doesn't really matter because this driver doesn't really do much interesting yet other than reporting memory access violations, but if that's enough to warrant a change of the compatible string, then I think we also need to preserve compatibility in the code.

That said, I think compatibility would be easier to preserve if we stuck with the old compatible string and used a "reg-names" property to specify which version of the binding we're referring to.

For example, we could have:

    memory-controller@7000f000 {
        compatible = "nvidia,tegra20-mc";
        reg = <0x7000f000 0x024
               0x7000f03c 0x3c4>;
        ...
    };

for the old binding and:

    memory-controller@7000f000 {
        compatible = "nvidia,tegra20-mc";
        reg = <0x7000f000 0x00000400>,
              <0x58000000 0x02000000>;
        reg-names = "mc", "gart";
        ...
    };

Quoted 2 lines

for the new binding. The driver can then easily check for the existence of the reg-names property and take the legacy or new code paths.

There is no problem with keeping compatibility for newer kernels with the older binding, it just not worth the effort. The real problem is keeping compatibility of older kernels with the new binding, the older kernels won't care about the reg-names and will treat GART registers as the second registers bank of the Memory Controller. Unfortunately I don't see how your suggestion is supposed to help with the problem.

There is no need to match device with the DT node since it was already matched, use of_device_get_match_data() helper to get the match-data.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/memory/tegra/mc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/memory/tegra/mc.c +0 -0

index 5454ffe5b2e0..cdc33f93cf7c 100644

Quoted 8 lines · Dmitry Osipenko

There is no need to match device with the DT node since it was already matched, use of_device_get_match_data() helper to get the match-data.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/memory/tegra/mc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/memory/tegra/mc.c line 15

Quoted 10 lines

drivers/memory/tegra/mc.c +0 -2

index 5454ffe5b2e0..cdc33f93cf7c 100644
@@ -11,8 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/platform_device.h>

It's better not to remove these two because the code still uses functions declared in them. If ever we were going to remove code using linux/of_device.h and then remove the linux/of_device.h include, we'd break the build and have to reintroduce the includes.

The same would happen if linux/of_device.h were ever to stop including linux/platform_device.h or linux/of.h. That may sound unlikely, but it has happened in the past with other includes. It can also happen that some restructuring takes place in some headers that is not so obvious and then things can still start falling apart miles away.

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Dmitry Osipenko

There is no need to match device with the DT node since it was already matched, use of_device_get_match_data() helper to get the match-data.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/memory/tegra/mc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Quoted 15 lines

drivers/memory/tegra/mc.c line 15

Quoted 10 lines

drivers/memory/tegra/mc.c +0 -2

index 5454ffe5b2e0..cdc33f93cf7c 100644
@@ -11,8 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/platform_device.h>

It's better not to remove these two because the code still uses functions declared in them. If ever we were going to remove code using linux/of_device.h and then remove the linux/of_device.h include, we'd break the build and have to reintroduce the includes.

That doesn't sound like a good argument. You're way too picky here ;)

Quoted 5 lines

The same would happen if linux/of_device.h were ever to stop including linux/platform_device.h or linux/of.h. That may sound unlikely, but it has happened in the past with other includes. It can also happen that some restructuring takes place in some headers that is not so obvious and then things can still start falling apart miles away.

Restructuring will be somebody else problem. Not sure that we really should care about it, I think it is unnecessary. But since you're insisting..

Quoted 34 lines · Dmitry Osipenko

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Dmitry Osipenko

There is no need to match device with the DT node since it was already matched, use of_device_get_match_data() helper to get the match-data.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/memory/tegra/mc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Quoted 15 lines

drivers/memory/tegra/mc.c line 15

Quoted 10 lines

drivers/memory/tegra/mc.c +0 -2

index 5454ffe5b2e0..cdc33f93cf7c 100644
@@ -11,8 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/platform_device.h>

It's better not to remove these two because the code still uses functions declared in them. If ever we were going to remove code using linux/of_device.h and then remove the linux/of_device.h include, we'd break the build and have to reintroduce the includes.

That doesn't sound like a good argument. You're way too picky here ;)

Quoted 5 lines

The same would happen if linux/of_device.h were ever to stop including linux/platform_device.h or linux/of.h. That may sound unlikely, but it has happened in the past with other includes. It can also happen that some restructuring takes place in some headers that is not so obvious and then things can still start falling apart miles away.

Quoted 3 lines

Restructuring will be somebody else problem. Not sure that we really should care about it, I think it is unnecessary. But since you're insisting..

It's actually a very common argument and I've seen patches in the past that add includes just for the purpose of making sure the right definitions get pulled in. This happens quite frequently as a preamble to some major rework of some header files that would otherwise cause a lot of breakage.

So I think it's best to be proactive about this and make sure we explicitly pull in all the necessary headers in the first place, irrespective of whether or not they may already get pulled in indirectly by some other headers.

The device-tree binding has been changed. There is no separate GART device anymore, it is squashed into the Memory Controller. Integrate GART module with the MC in a way it is done for the SMMU of Tegra30+.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/Kconfig      |  1 +
 drivers/iommu/tegra-gart.c | 98 ++++++++++----------------------------
 drivers/memory/tegra/mc.c  | 41 ++++++++++++++++
 include/soc/tegra/mc.h     | 27 +++++++++++
 4 files changed, 93 insertions(+), 74 deletions(-)

Quoted 11 lines · Dmitry Osipenko

The device-tree binding has been changed. There is no separate GART device anymore, it is squashed into the Memory Controller. Integrate GART module with the MC in a way it is done for the SMMU of Tegra30+.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/Kconfig      |  1 +
 drivers/iommu/tegra-gart.c | 98 ++++++++++----------------------------
 drivers/memory/tegra/mc.c  | 41 ++++++++++++++++
 include/soc/tegra/mc.h     | 27 +++++++++++
 4 files changed, 93 insertions(+), 74 deletions(-)

I think this could technically have been two patches, but since they'd have a compile-time dependency either way they need to be applied in the correct order, so some coordination between IOMMU and Tegra trees is going to have to happen anyway and might as well just stick this into a single patch.

Quoted 13 lines · Thierry Reding

Quoted 11 lines · Dmitry Osipenko

The device-tree binding has been changed. There is no separate GART device anymore, it is squashed into the Memory Controller. Integrate GART module with the MC in a way it is done for the SMMU of Tegra30+.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/Kconfig      |  1 +
 drivers/iommu/tegra-gart.c | 98 ++++++++++----------------------------
 drivers/memory/tegra/mc.c  | 41 ++++++++++++++++
 include/soc/tegra/mc.h     | 27 +++++++++++
 4 files changed, 93 insertions(+), 74 deletions(-)

Quoted 5 lines

I think this could technically have been two patches, but since they'd have a compile-time dependency either way they need to be applied in the correct order, so some coordination between IOMMU and Tegra trees is going to have to happen anyway and might as well just stick this into a single patch.

I assume that Joerg will take the whole series once it's ready (no?), hence your ACK will be needed here and in other patches.

Fix spinlock recursion bug that happens on IOMMU domain destruction if any of the allocated domains have devices attached to them.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Fix NULL pointer dereference on IOMMU domain destruction that happens because clients list is being iterated unsafely and its elements are getting deleted during the iteration.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

GART has a single address space that is shared by all devices, hence only one domain could be active at a time.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/iommu/tegra-gart.c +0 -0

index 1d45b023adea..9f7d3afb686f 100644

GART is a part of the Memory Controller driver that is always built-in, hence there is no benefit from the use of managed resources.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Quoted 7 lines · Dmitry Osipenko

GART is a part of the Memory Controller driver that is always built-in, hence there is no benefit from the use of managed resources.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

One benefit would be cleanup on probe error. Also, we may eventually want to make even the memory controller driver buildable as a module.

Quoted 9 lines · Thierry Reding

Quoted 7 lines · Dmitry Osipenko

GART is a part of the Memory Controller driver that is always built-in, hence there is no benefit from the use of managed resources.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Quoted 2 lines

One benefit would be cleanup on probe error. Also, we may eventually want to make even the memory controller driver buildable as a module.

Please, let's keep that patch as is and re-introduce devm once it will become really relevant.

Quoted 10 lines · Dmitry Osipenko

Quoted 9 lines · Thierry Reding

Quoted 7 lines · Dmitry Osipenko

GART is a part of the Memory Controller driver that is always built-in, hence there is no benefit from the use of managed resources.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Quoted 5 lines

Quoted 2 lines

One benefit would be cleanup on probe error. Also, we may eventually want to make even the memory controller driver buildable as a module.

Please, let's keep that patch as is and re-introduce devm once it will become really relevant.

Alright, fine with me.

GART became a part of Memory Controller, hence now the drivers device is Memory Controller and not GART. As a result all printed messages are prepended with the "tegra-mc 7000f000.memory-controller:", so let's prepend GART's messages with "GART:" in order to differentiate them from the MC.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Quoted 10 lines · Dmitry Osipenko

GART became a part of Memory Controller, hence now the drivers device is Memory Controller and not GART. As a result all printed messages are prepended with the "tegra-mc 7000f000.memory-controller:", so let's prepend GART's messages with "GART:" in order to differentiate them from the MC.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

There's a macro called dev_fmt (similar to pr_fmt) to do this for dev_* printers. Also I think this would be more readable if the prefix was "gart: " rather than "GART: ". At least from my personal experience I get easily distracted by all-caps words in logs, because they usually indicate something that requires immediate attention. I think it's better to leave that up to higher level mechanisms, such as the color keying of messages based on level by tools like dmesg.

There could be unlimited number of allocated domains, but only one domain can be active at a time. Hence devices must be detached only from the active domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 8 lines · Dmitry Osipenko

There could be unlimited number of allocated domains, but only one domain can be active at a time. Hence devices must be detached only from the active domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Do we have a mechanism of switching out different domains? I don't think we do, so I'm wondering if perhaps a better solution to this would be to just refuse to create more than one domain in the first place. That would also allow us to get rid of the global variable gart_handle.

Quoted 10 lines · Thierry Reding

Quoted 8 lines · Dmitry Osipenko

There could be unlimited number of allocated domains, but only one domain can be active at a time. Hence devices must be detached only from the active domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Quoted 4 lines

Do we have a mechanism of switching out different domains? I don't think we do, so I'm wondering if perhaps a better solution to this would be to just refuse to create more than one domain in the first place. That would also allow us to get rid of the global variable gart_handle.

That's what was done in v1, Robin Murphy suggested that it will be better not to restrict allocation of unpopulated domains. It is mentioned in the changelog, see comment to v2 changes.

Quoted 16 lines · Dmitry Osipenko

Quoted 14 lines · Thierry Reding

Quoted 8 lines · Dmitry Osipenko

There could be unlimited number of allocated domains, but only one domain can be active at a time. Hence devices must be detached only from the active domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Do we have a mechanism of switching out different domains? I don't think we do, so I'm wondering if perhaps a better solution to this would be to just refuse to create more than one domain in the first place. That would also allow us to get rid of the global variable gart_handle.

Quoted 3 lines

That's what was done in v1, Robin Murphy suggested that it will be better not to restrict allocation of unpopulated domains. It is mentioned in the changelog, see comment to v2 changes.

Okay, fine.

GART is a simple IOMMU provider that has single address space. There is no need to setup global clients list and manage it for tracking of the active domain, hence lot's of code could be safely removed and replaced with a simpler alternative.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 157 +++++++++----------------------------
 1 file changed, 39 insertions(+), 118 deletions(-)

drivers/iommu/tegra-gart.c +4 -64

index 306e9644a676..7182445c3b76 100644
@@ -19,7 +19,6 @@
 
 #include <linux/io.h>
 #include <linux/iommu.h>
-#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>

@@ -42,30 +41,20 @@
 #define GART_PAGE_MASK						\
 	(~(GART_PAGE_SIZE - 1) & ~GART_ENTRY_PHYS_ADDR_VALID)
 
-struct gart_client {
-	struct device		*dev;
-	struct list_head	list;
-};
-
 struct gart_device {
 	void __iomem		*regs;
 	u32			*savedata;
 	u32			page_count;	/* total remappable size */
 	dma_addr_t		iovmm_base;	/* offset to vmm_area */
 	spinlock_t		pte_lock;	/* for pagetable */
-	struct list_head	client;
-	spinlock_t		client_lock;	/* for client list */
+	spinlock_t		dom_lock;	/* for active domain */
+	unsigned int		active_devices;	/* number of active devices */
 	struct iommu_domain	*active_domain;	/* current active domain */
 	struct device		*dev;
 
 	struct iommu_device	iommu;		/* IOMMU Core handle */
 };
 
-struct gart_domain {
-	struct iommu_domain domain;		/* generic domain handle */
-	struct gart_device *gart;		/* link to gart device   */
-};
-
 static struct gart_device *gart_handle; /* unique for a system */
 
 static bool gart_debug;

@@ -73,11 +62,6 @@ static bool gart_debug;
 #define GART_PTE(_pfn)						\
 	(GART_ENTRY_PHYS_ADDR_VALID | ((_pfn) << PAGE_SHIFT))
 
-static struct gart_domain *to_gart_domain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct gart_domain, domain);
-}
-
 /*
  * Any interaction between any block on PPSB and a block on APB or AHB
  * must have these read-back to ensure the APB/AHB bus transaction is

@@ -166,128 +150,69 @@ static inline bool gart_iova_range_valid(struct gart_device *gart,
 static int gart_iommu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
 	struct gart_device *gart = gart_handle;
-	struct gart_client *client, *c;
-	int err = 0;
-
-	client = kzalloc(sizeof(*c), GFP_KERNEL);
-	if (!client)
-		return -ENOMEM;
-	client->dev = dev;
-
-	spin_lock(&gart->client_lock);
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			dev_err(gart->dev, "GART: %s is already attached\n",
-				dev_name(dev));
-			err = -EINVAL;
-			goto fail;
-		}
-	}
-	if (gart->active_domain && gart->active_domain != domain) {
-		dev_err(gart->dev,
-			"GART: Only one domain can be active at a time\n");
-		err = -EINVAL;
-		goto fail;
-	}
-	gart->active_domain = domain;
-	gart_domain->gart = gart;
-	list_add(&client->list, &gart->client);
-	spin_unlock(&gart->client_lock);
-	dev_dbg(gart->dev, "GART: Attached %s\n", dev_name(dev));
-	return 0;
+	int ret = 0;
 
-fail:
-	kfree(client);
-	spin_unlock(&gart->client_lock);
-	return err;
-}
+	spin_lock(&gart->dom_lock);
 
-static void __gart_iommu_detach_dev(struct iommu_domain *domain,
-				    struct device *dev)
-{
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-	struct gart_client *c;
-
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			list_del(&c->list);
-			kfree(c);

drivers/iommu/tegra-gart.c +27 -28

+	} else if (dev->archdata.iommu != domain) {
+		dev->archdata.iommu = domain;
+		gart->active_domain = domain;
+		gart->active_devices++;
 	}
 
-	dev_err(gart->dev, "GART: Couldn't find %s to detach\n",
-		dev_name(dev));
+	spin_unlock(&gart->dom_lock);
+
+	return ret;
 }
 
 static void gart_iommu_detach_dev(struct iommu_domain *domain,
 				  struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;
+
+	spin_lock(&gart->dom_lock);
 
-	spin_lock(&gart->client_lock);
-	__gart_iommu_detach_dev(domain, dev);
-	spin_unlock(&gart->client_lock);
+	if (dev->archdata.iommu == domain) {
+		dev->archdata.iommu = NULL;
+
+		if (--gart->active_devices == 0)
+			gart->active_domain = NULL;
+	}
+
+	spin_unlock(&gart->dom_lock);
 }
 
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
-	struct gart_domain *gart_domain;
-	struct gart_device *gart;
+	struct gart_device *gart = gart_handle;
+	struct iommu_domain *domain;
 
 	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
-	gart = gart_handle;
-	if (!gart)
-		return NULL;
-
-	gart_domain = kzalloc(sizeof(*gart_domain), GFP_KERNEL);
-	if (!gart_domain)
-		return NULL;
-
-	gart_domain->domain.geometry.aperture_start = gart->iovmm_base;
-	gart_domain->domain.geometry.aperture_end = gart->iovmm_base +
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (domain) {
+		domain->geometry.aperture_start = gart->iovmm_base;
+		domain->geometry.aperture_end = gart->iovmm_base +
 					gart->page_count * GART_PAGE_SIZE - 1;
-	gart_domain->domain.geometry.force_aperture = true;
+		domain->geometry.force_aperture = true;
+	}
 
-	return &gart_domain->domain;
+	return domain;
 }
 
 static void gart_iommu_domain_free(struct iommu_domain *domain)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-
-	if (gart) {
-		spin_lock(&gart->client_lock);
-		if (!list_empty(&gart->client)) {
-			struct gart_client *c, *tmp;

Quoted 129 lines · Dmitry Osipenko

GART is a simple IOMMU provider that has single address space. There is no need to setup global clients list and manage it for tracking of the active domain, hence lot's of code could be safely removed and replaced with a simpler alternative.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 157 +++++++++----------------------------
 1 file changed, 39 insertions(+), 118 deletions(-)

drivers/iommu/tegra-gart.c +4 -71

index 306e9644a676..7182445c3b76 100644
@@ -19,7 +19,6 @@
 
 #include <linux/io.h>
 #include <linux/iommu.h>
-#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>

@@ -42,30 +41,20 @@
 #define GART_PAGE_MASK						\
 	(~(GART_PAGE_SIZE - 1) & ~GART_ENTRY_PHYS_ADDR_VALID)
 
-struct gart_client {
-	struct device		*dev;
-	struct list_head	list;
-};
-
 struct gart_device {
 	void __iomem		*regs;
 	u32			*savedata;
 	u32			page_count;	/* total remappable size */
 	dma_addr_t		iovmm_base;	/* offset to vmm_area */
 	spinlock_t		pte_lock;	/* for pagetable */
-	struct list_head	client;
-	spinlock_t		client_lock;	/* for client list */
+	spinlock_t		dom_lock;	/* for active domain */
+	unsigned int		active_devices;	/* number of active devices */
 	struct iommu_domain	*active_domain;	/* current active domain */
 	struct device		*dev;
 
 	struct iommu_device	iommu;		/* IOMMU Core handle */
 };
 
-struct gart_domain {
-	struct iommu_domain domain;		/* generic domain handle */
-	struct gart_device *gart;		/* link to gart device   */
-};
-
 static struct gart_device *gart_handle; /* unique for a system */
 
 static bool gart_debug;

@@ -73,11 +62,6 @@ static bool gart_debug;
 #define GART_PTE(_pfn)						\
 	(GART_ENTRY_PHYS_ADDR_VALID | ((_pfn) << PAGE_SHIFT))
 
-static struct gart_domain *to_gart_domain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct gart_domain, domain);
-}
-
 /*
  * Any interaction between any block on PPSB and a block on APB or AHB
  * must have these read-back to ensure the APB/AHB bus transaction is

@@ -166,128 +150,69 @@ static inline bool gart_iova_range_valid(struct gart_device *gart,
 static int gart_iommu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
 	struct gart_device *gart = gart_handle;
-	struct gart_client *client, *c;
-	int err = 0;
-
-	client = kzalloc(sizeof(*c), GFP_KERNEL);
-	if (!client)
-		return -ENOMEM;
-	client->dev = dev;
-
-	spin_lock(&gart->client_lock);
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			dev_err(gart->dev, "GART: %s is already attached\n",
-				dev_name(dev));
-			err = -EINVAL;
-			goto fail;
-		}
-	}
-	if (gart->active_domain && gart->active_domain != domain) {
-		dev_err(gart->dev,
-			"GART: Only one domain can be active at a time\n");
-		err = -EINVAL;
-		goto fail;
-	}
-	gart->active_domain = domain;
-	gart_domain->gart = gart;
-	list_add(&client->list, &gart->client);
-	spin_unlock(&gart->client_lock);
-	dev_dbg(gart->dev, "GART: Attached %s\n", dev_name(dev));
-	return 0;
+	int ret = 0;
 
-fail:
-	kfree(client);
-	spin_unlock(&gart->client_lock);
-	return err;
-}
+	spin_lock(&gart->dom_lock);
 
-static void __gart_iommu_detach_dev(struct iommu_domain *domain,
-				    struct device *dev)
-{
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-	struct gart_client *c;
-
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			list_del(&c->list);
-			kfree(c);
-			if (list_empty(&gart->client)) {
-				gart->active_domain = NULL;
-				gart_domain->gart = NULL;
-			}
-			dev_dbg(gart->dev, "GART: Detached %s\n",
-				dev_name(dev));
-			return;

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 159

Quoted 3 lines

drivers/iommu/tegra-gart.c +2 -1

-		}
+	if (gart->active_domain && gart->active_domain != domain) {
+		ret = -EBUSY;

This omits the error message and returns -EBUSY instead of -EINVAL. Was this intended? For what it's worth, I do agree with the changes, it's just that I think you could've made those in the earlier patch that introduced them.

But this is all one series and the end result looks fine, so no need to be that picky.

Quoted 81 lines

drivers/iommu/tegra-gart.c +27 -32

+	} else if (dev->archdata.iommu != domain) {
+		dev->archdata.iommu = domain;
+		gart->active_domain = domain;
+		gart->active_devices++;
 	}
 
-	dev_err(gart->dev, "GART: Couldn't find %s to detach\n",
-		dev_name(dev));
+	spin_unlock(&gart->dom_lock);
+
+	return ret;
 }
 
 static void gart_iommu_detach_dev(struct iommu_domain *domain,
 				  struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;
+
+	spin_lock(&gart->dom_lock);
 
-	spin_lock(&gart->client_lock);
-	__gart_iommu_detach_dev(domain, dev);
-	spin_unlock(&gart->client_lock);
+	if (dev->archdata.iommu == domain) {
+		dev->archdata.iommu = NULL;
+
+		if (--gart->active_devices == 0)
+			gart->active_domain = NULL;
+	}
+
+	spin_unlock(&gart->dom_lock);
 }
 
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
-	struct gart_domain *gart_domain;
-	struct gart_device *gart;
+	struct gart_device *gart = gart_handle;
+	struct iommu_domain *domain;
 
 	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
-	gart = gart_handle;
-	if (!gart)
-		return NULL;
-
-	gart_domain = kzalloc(sizeof(*gart_domain), GFP_KERNEL);
-	if (!gart_domain)
-		return NULL;
-
-	gart_domain->domain.geometry.aperture_start = gart->iovmm_base;
-	gart_domain->domain.geometry.aperture_end = gart->iovmm_base +
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (domain) {
+		domain->geometry.aperture_start = gart->iovmm_base;
+		domain->geometry.aperture_end = gart->iovmm_base +
 					gart->page_count * GART_PAGE_SIZE - 1;
-	gart_domain->domain.geometry.force_aperture = true;
+		domain->geometry.force_aperture = true;
+	}
 
-	return &gart_domain->domain;
+	return domain;
 }
 
 static void gart_iommu_domain_free(struct iommu_domain *domain)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-
-	if (gart) {
-		spin_lock(&gart->client_lock);
-		if (!list_empty(&gart->client)) {
-			struct gart_client *c, *tmp;
-
-			list_for_each_entry_safe(c, tmp, &gart->client, list)
-				__gart_iommu_detach_dev(domain, c->dev);
-		}

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 210

Quoted 6 lines

drivers/iommu/tegra-gart.c +1 -4

-		spin_unlock(&gart->client_lock);
-	}
-
-	kfree(gart_domain);
+	kfree(domain);
 }

Doesn't this now make it possible to free a potentially active domain?

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 215

Quoted 7 lines

drivers/iommu/tegra-gart.c +1 -2

 static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t pa, size_t bytes, int prot)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;

Hmm... this now introduces more uses of the gart_handle that I hoped we could get rid of. I think we could still keep around struct gart_domain and just make sure it is unique. The small amounts of casting here seem mostly harmless to me, especially since they will be nops, so we end up with just one dereference to get at the struct gart_device. I think the benefits of not having this global variable around are worth the one dereference here.

Quoted 130 lines · Thierry Reding

Quoted 129 lines · Dmitry Osipenko

GART is a simple IOMMU provider that has single address space. There is no need to setup global clients list and manage it for tracking of the active domain, hence lot's of code could be safely removed and replaced with a simpler alternative.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 157 +++++++++----------------------------
 1 file changed, 39 insertions(+), 118 deletions(-)

drivers/iommu/tegra-gart.c +4 -71

index 306e9644a676..7182445c3b76 100644
@@ -19,7 +19,6 @@
 
 #include <linux/io.h>
 #include <linux/iommu.h>
-#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>

@@ -42,30 +41,20 @@
 #define GART_PAGE_MASK						\
 	(~(GART_PAGE_SIZE - 1) & ~GART_ENTRY_PHYS_ADDR_VALID)
 
-struct gart_client {
-	struct device		*dev;
-	struct list_head	list;
-};
-
 struct gart_device {
 	void __iomem		*regs;
 	u32			*savedata;
 	u32			page_count;	/* total remappable size */
 	dma_addr_t		iovmm_base;	/* offset to vmm_area */
 	spinlock_t		pte_lock;	/* for pagetable */
-	struct list_head	client;
-	spinlock_t		client_lock;	/* for client list */
+	spinlock_t		dom_lock;	/* for active domain */
+	unsigned int		active_devices;	/* number of active devices */
 	struct iommu_domain	*active_domain;	/* current active domain */
 	struct device		*dev;
 
 	struct iommu_device	iommu;		/* IOMMU Core handle */
 };
 
-struct gart_domain {
-	struct iommu_domain domain;		/* generic domain handle */
-	struct gart_device *gart;		/* link to gart device   */
-};
-
 static struct gart_device *gart_handle; /* unique for a system */
 
 static bool gart_debug;

@@ -73,11 +62,6 @@ static bool gart_debug;
 #define GART_PTE(_pfn)						\
 	(GART_ENTRY_PHYS_ADDR_VALID | ((_pfn) << PAGE_SHIFT))
 
-static struct gart_domain *to_gart_domain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct gart_domain, domain);
-}
-
 /*
  * Any interaction between any block on PPSB and a block on APB or AHB
  * must have these read-back to ensure the APB/AHB bus transaction is

@@ -166,128 +150,69 @@ static inline bool gart_iova_range_valid(struct gart_device *gart,
 static int gart_iommu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
 	struct gart_device *gart = gart_handle;
-	struct gart_client *client, *c;
-	int err = 0;
-
-	client = kzalloc(sizeof(*c), GFP_KERNEL);
-	if (!client)
-		return -ENOMEM;
-	client->dev = dev;
-
-	spin_lock(&gart->client_lock);
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			dev_err(gart->dev, "GART: %s is already attached\n",
-				dev_name(dev));
-			err = -EINVAL;
-			goto fail;
-		}
-	}
-	if (gart->active_domain && gart->active_domain != domain) {
-		dev_err(gart->dev,
-			"GART: Only one domain can be active at a time\n");
-		err = -EINVAL;
-		goto fail;
-	}
-	gart->active_domain = domain;
-	gart_domain->gart = gart;
-	list_add(&client->list, &gart->client);
-	spin_unlock(&gart->client_lock);
-	dev_dbg(gart->dev, "GART: Attached %s\n", dev_name(dev));
-	return 0;
+	int ret = 0;
 
-fail:
-	kfree(client);
-	spin_unlock(&gart->client_lock);
-	return err;
-}
+	spin_lock(&gart->dom_lock);
 
-static void __gart_iommu_detach_dev(struct iommu_domain *domain,
-				    struct device *dev)
-{
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-	struct gart_client *c;
-
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			list_del(&c->list);
-			kfree(c);
-			if (list_empty(&gart->client)) {
-				gart->active_domain = NULL;
-				gart_domain->gart = NULL;
-			}
-			dev_dbg(gart->dev, "GART: Detached %s\n",
-				dev_name(dev));
-			return;

Quoted 8 lines

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 159

Quoted 3 lines

drivers/iommu/tegra-gart.c +2 -1

-		}
+	if (gart->active_domain && gart->active_domain != domain) {
+		ret = -EBUSY;

This omits the error message and returns -EBUSY instead of -EINVAL. Was this intended? For what it's worth, I do agree with the changes, it's just that I think you could've made those in the earlier patch that introduced them.

The message isn't really needed and EBUSY seems fit better than EINVAL here.

Quoted 2 lines

But this is all one series and the end result looks fine, so no need to be that picky.

Good, thanks.

Quoted 81 lines

Quoted 81 lines

drivers/iommu/tegra-gart.c +27 -32

+	} else if (dev->archdata.iommu != domain) {
+		dev->archdata.iommu = domain;
+		gart->active_domain = domain;
+		gart->active_devices++;
 	}
 
-	dev_err(gart->dev, "GART: Couldn't find %s to detach\n",
-		dev_name(dev));
+	spin_unlock(&gart->dom_lock);
+
+	return ret;
 }
 
 static void gart_iommu_detach_dev(struct iommu_domain *domain,
 				  struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;
+
+	spin_lock(&gart->dom_lock);
 
-	spin_lock(&gart->client_lock);
-	__gart_iommu_detach_dev(domain, dev);
-	spin_unlock(&gart->client_lock);
+	if (dev->archdata.iommu == domain) {
+		dev->archdata.iommu = NULL;
+
+		if (--gart->active_devices == 0)
+			gart->active_domain = NULL;
+	}
+
+	spin_unlock(&gart->dom_lock);
 }
 
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
-	struct gart_domain *gart_domain;
-	struct gart_device *gart;
+	struct gart_device *gart = gart_handle;
+	struct iommu_domain *domain;
 
 	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
-	gart = gart_handle;
-	if (!gart)
-		return NULL;
-
-	gart_domain = kzalloc(sizeof(*gart_domain), GFP_KERNEL);
-	if (!gart_domain)
-		return NULL;
-
-	gart_domain->domain.geometry.aperture_start = gart->iovmm_base;
-	gart_domain->domain.geometry.aperture_end = gart->iovmm_base +
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (domain) {
+		domain->geometry.aperture_start = gart->iovmm_base;
+		domain->geometry.aperture_end = gart->iovmm_base +
 					gart->page_count * GART_PAGE_SIZE - 1;
-	gart_domain->domain.geometry.force_aperture = true;
+		domain->geometry.force_aperture = true;
+	}
 
-	return &gart_domain->domain;
+	return domain;
 }
 
 static void gart_iommu_domain_free(struct iommu_domain *domain)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-
-	if (gart) {
-		spin_lock(&gart->client_lock);
-		if (!list_empty(&gart->client)) {
-			struct gart_client *c, *tmp;
-
-			list_for_each_entry_safe(c, tmp, &gart->client, list)
-				__gart_iommu_detach_dev(domain, c->dev);
-		}

Quoted 8 lines

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 210

Quoted 6 lines

drivers/iommu/tegra-gart.c +1 -4

-		spin_unlock(&gart->client_lock);
-	}
-
-	kfree(gart_domain);
+	kfree(domain);
 }

Doesn't this now make it possible to free a potentially active domain?

Yes, don't do it. I can add a WARN_ON() here, though I think IOMMU core should be the one taking care about that.

Quoted 7 lines

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 215

Quoted 6 lines

drivers/iommu/tegra-gart.c +1 -2

 static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t pa, size_t bytes, int prot)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;

Quoted 7 lines

Hmm... this now introduces more uses of the gart_handle that I hoped we could get rid of. I think we could still keep around struct gart_domain and just make sure it is unique. The small amounts of casting here seem mostly harmless to me, especially since they will be nops, so we end up with just one dereference to get at the struct gart_device. I think the benefits of not having this global variable around are worth the one dereference here.

What are the benefits? I don't see anything other than the pedantic oddity.

I've removed gart_domain in the end because it is an extra code (and consumed resources) without any benefit. Let's keep that part as it is now. I'll be happy to change that code if you'll explain why it is worth it.

Quoted 237 lines · Dmitry Osipenko

Quoted 130 lines · Thierry Reding

Quoted 129 lines · Dmitry Osipenko

GART is a simple IOMMU provider that has single address space. There is no need to setup global clients list and manage it for tracking of the active domain, hence lot's of code could be safely removed and replaced with a simpler alternative.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 157 +++++++++----------------------------
 1 file changed, 39 insertions(+), 118 deletions(-)

drivers/iommu/tegra-gart.c +4 -71

index 306e9644a676..7182445c3b76 100644
@@ -19,7 +19,6 @@
 
 #include <linux/io.h>
 #include <linux/iommu.h>
-#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>

@@ -42,30 +41,20 @@
 #define GART_PAGE_MASK						\
 	(~(GART_PAGE_SIZE - 1) & ~GART_ENTRY_PHYS_ADDR_VALID)
 
-struct gart_client {
-	struct device		*dev;
-	struct list_head	list;
-};
-
 struct gart_device {
 	void __iomem		*regs;
 	u32			*savedata;
 	u32			page_count;	/* total remappable size */
 	dma_addr_t		iovmm_base;	/* offset to vmm_area */
 	spinlock_t		pte_lock;	/* for pagetable */
-	struct list_head	client;
-	spinlock_t		client_lock;	/* for client list */
+	spinlock_t		dom_lock;	/* for active domain */
+	unsigned int		active_devices;	/* number of active devices */
 	struct iommu_domain	*active_domain;	/* current active domain */
 	struct device		*dev;
 
 	struct iommu_device	iommu;		/* IOMMU Core handle */
 };
 
-struct gart_domain {
-	struct iommu_domain domain;		/* generic domain handle */
-	struct gart_device *gart;		/* link to gart device   */
-};
-
 static struct gart_device *gart_handle; /* unique for a system */
 
 static bool gart_debug;

@@ -73,11 +62,6 @@ static bool gart_debug;
 #define GART_PTE(_pfn)						\
 	(GART_ENTRY_PHYS_ADDR_VALID | ((_pfn) << PAGE_SHIFT))
 
-static struct gart_domain *to_gart_domain(struct iommu_domain *dom)
-{
-	return container_of(dom, struct gart_domain, domain);
-}
-
 /*
  * Any interaction between any block on PPSB and a block on APB or AHB
  * must have these read-back to ensure the APB/AHB bus transaction is

@@ -166,128 +150,69 @@ static inline bool gart_iova_range_valid(struct gart_device *gart,
 static int gart_iommu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
 	struct gart_device *gart = gart_handle;
-	struct gart_client *client, *c;
-	int err = 0;
-
-	client = kzalloc(sizeof(*c), GFP_KERNEL);
-	if (!client)
-		return -ENOMEM;
-	client->dev = dev;
-
-	spin_lock(&gart->client_lock);
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			dev_err(gart->dev, "GART: %s is already attached\n",
-				dev_name(dev));
-			err = -EINVAL;
-			goto fail;
-		}
-	}
-	if (gart->active_domain && gart->active_domain != domain) {
-		dev_err(gart->dev,
-			"GART: Only one domain can be active at a time\n");
-		err = -EINVAL;
-		goto fail;
-	}
-	gart->active_domain = domain;
-	gart_domain->gart = gart;
-	list_add(&client->list, &gart->client);
-	spin_unlock(&gart->client_lock);
-	dev_dbg(gart->dev, "GART: Attached %s\n", dev_name(dev));
-	return 0;
+	int ret = 0;
 
-fail:
-	kfree(client);
-	spin_unlock(&gart->client_lock);
-	return err;
-}
+	spin_lock(&gart->dom_lock);
 
-static void __gart_iommu_detach_dev(struct iommu_domain *domain,
-				    struct device *dev)
-{
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-	struct gart_client *c;
-
-	list_for_each_entry(c, &gart->client, list) {
-		if (c->dev == dev) {
-			list_del(&c->list);
-			kfree(c);
-			if (list_empty(&gart->client)) {
-				gart->active_domain = NULL;
-				gart_domain->gart = NULL;
-			}
-			dev_dbg(gart->dev, "GART: Detached %s\n",
-				dev_name(dev));
-			return;

Quoted 8 lines

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 159

Quoted 3 lines

drivers/iommu/tegra-gart.c +2 -1

-		}
+	if (gart->active_domain && gart->active_domain != domain) {
+		ret = -EBUSY;

This omits the error message and returns -EBUSY instead of -EINVAL. Was this intended? For what it's worth, I do agree with the changes, it's just that I think you could've made those in the earlier patch that introduced them.

The message isn't really needed and EBUSY seems fit better than EINVAL here.

Quoted 2 lines

But this is all one series and the end result looks fine, so no need to be that picky.

Good, thanks.

Quoted 81 lines

Quoted 81 lines

drivers/iommu/tegra-gart.c +27 -32

+	} else if (dev->archdata.iommu != domain) {
+		dev->archdata.iommu = domain;
+		gart->active_domain = domain;
+		gart->active_devices++;
 	}
 
-	dev_err(gart->dev, "GART: Couldn't find %s to detach\n",
-		dev_name(dev));
+	spin_unlock(&gart->dom_lock);
+
+	return ret;
 }
 
 static void gart_iommu_detach_dev(struct iommu_domain *domain,
 				  struct device *dev)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;
+
+	spin_lock(&gart->dom_lock);
 
-	spin_lock(&gart->client_lock);
-	__gart_iommu_detach_dev(domain, dev);
-	spin_unlock(&gart->client_lock);
+	if (dev->archdata.iommu == domain) {
+		dev->archdata.iommu = NULL;
+
+		if (--gart->active_devices == 0)
+			gart->active_domain = NULL;
+	}
+
+	spin_unlock(&gart->dom_lock);
 }
 
 static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
 {
-	struct gart_domain *gart_domain;
-	struct gart_device *gart;
+	struct gart_device *gart = gart_handle;
+	struct iommu_domain *domain;
 
 	if (type != IOMMU_DOMAIN_UNMANAGED)
 		return NULL;
 
-	gart = gart_handle;
-	if (!gart)
-		return NULL;
-
-	gart_domain = kzalloc(sizeof(*gart_domain), GFP_KERNEL);
-	if (!gart_domain)
-		return NULL;
-
-	gart_domain->domain.geometry.aperture_start = gart->iovmm_base;
-	gart_domain->domain.geometry.aperture_end = gart->iovmm_base +
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (domain) {
+		domain->geometry.aperture_start = gart->iovmm_base;
+		domain->geometry.aperture_end = gart->iovmm_base +
 					gart->page_count * GART_PAGE_SIZE - 1;
-	gart_domain->domain.geometry.force_aperture = true;
+		domain->geometry.force_aperture = true;
+	}
 
-	return &gart_domain->domain;
+	return domain;
 }
 
 static void gart_iommu_domain_free(struct iommu_domain *domain)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
-
-	if (gart) {
-		spin_lock(&gart->client_lock);
-		if (!list_empty(&gart->client)) {
-			struct gart_client *c, *tmp;
-
-			list_for_each_entry_safe(c, tmp, &gart->client, list)
-				__gart_iommu_detach_dev(domain, c->dev);
-		}

Quoted 8 lines

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 210

Quoted 6 lines

drivers/iommu/tegra-gart.c +1 -4

-		spin_unlock(&gart->client_lock);
-	}
-
-	kfree(gart_domain);
+	kfree(domain);
 }

Doesn't this now make it possible to free a potentially active domain?

Quoted 2 lines

Yes, don't do it. I can add a WARN_ON() here, though I think IOMMU core should be the one taking care about that.

Yeah, might be good to have the WARN_ON() either here or in the IOMMU core. Force-detaching is probably a good idea, too, otherwise the users of the freed domain are just going to crash anyway, right? Maybe something to discuss more generally with Joerg.

I think in the meantime just having the WARN_ON() here is probably good enough. It should point out the cases where we do free the domain with devices still attached, which hopefully don't exist, and we can fix them.

Quoted 17 lines

Quoted 7 lines

drivers/iommu/tegra-gart.c static inline bool gart_iova_range_valid(struct gart_device *gart, · line 215

Quoted 6 lines

drivers/iommu/tegra-gart.c +1 -2

 static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 			  phys_addr_t pa, size_t bytes, int prot)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
+	struct gart_device *gart = gart_handle;

Quoted 7 lines

Hmm... this now introduces more uses of the gart_handle that I hoped we could get rid of. I think we could still keep around struct gart_domain and just make sure it is unique. The small amounts of casting here seem mostly harmless to me, especially since they will be nops, so we end up with just one dereference to get at the struct gart_device. I think the benefits of not having this global variable around are worth the one dereference here.

What are the benefits? I don't see anything other than the pedantic oddity.

Quoted 4 lines

I've removed gart_domain in the end because it is an extra code (and consumed resources) without any benefit. Let's keep that part as it is now. I'll be happy to change that code if you'll explain why it is worth it.

I thought I did explain. Anyway, it's always been like this, so no need to change it as part of this series.

Perform a major code cleanup to make it more readable and as a result easier to maintain. I removed some redundant safety-checks in the code and some debug code that isn't actually very useful for debugging, like enormous pagetable dump on each fault. The majority of the changes are code reshuffling, variables/whitespaces clean up and removal of debug messages that duplicate messages of the IOMMU-core.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 215 +++++++++++++++----------------------
 1 file changed, 84 insertions(+), 131 deletions(-)

drivers/iommu/tegra-gart.c +7 -11

index 7182445c3b76..a36d0c568536 100644
@@ -34,63 +34,56 @@
 #define GART_CONFIG		(0x24 - GART_REG_BASE)
 #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
 #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
-#define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
+
+#define GART_ENTRY_PHYS_ADDR_VALID	BIT(31)
 
 #define GART_PAGE_SHIFT		12
 #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
-#define GART_PAGE_MASK						\
-	(~(GART_PAGE_SIZE - 1) & ~GART_ENTRY_PHYS_ADDR_VALID)
+#define GART_PAGE_MASK		GENMASK(30, GART_PAGE_SHIFT)
 
 struct gart_device {
 	void __iomem		*regs;
 	u32			*savedata;
-	u32			page_count;	/* total remappable size */
-	dma_addr_t		iovmm_base;	/* offset to vmm_area */
+	unsigned long		iovmm_base;	/* offset to vmm_area start */
+	unsigned long		iovmm_end;	/* offset to vmm_area end */
 	spinlock_t		pte_lock;	/* for pagetable */
 	spinlock_t		dom_lock;	/* for active domain */
 	unsigned int		active_devices;	/* number of active devices */
 	struct iommu_domain	*active_domain;	/* current active domain */
-	struct device		*dev;
-
 	struct iommu_device	iommu;		/* IOMMU Core handle */
+	struct device		*dev;
 };
 
 static struct gart_device *gart_handle; /* unique for a system */
 
 static bool gart_debug;
 
-#define GART_PTE(_pfn)						\
-	(GART_ENTRY_PHYS_ADDR_VALID | ((_pfn) << PAGE_SHIFT))
-
 /*
  * Any interaction between any block on PPSB and a block on APB or AHB
  * must have these read-back to ensure the APB/AHB bus transaction is
  * complete before initiating activity on the PPSB block.
  */
-#define FLUSH_GART_REGS(gart)	((void)readl((gart)->regs + GART_CONFIG))
+#define FLUSH_GART_REGS(gart)	readl_relaxed((gart)->regs + GART_CONFIG)