Changelog
v1->v2:
* Added Thierry's acks to PATCH-3~5
This is a merged set of resend for previously two series of patches
that were reviewed/acked a month ago yet have not got applied.
Series-1: https://lkml.org/lkml/2020/9/29/73
"[PATCH v4 0/2] iommu/tegra-smmu: Two followup changes"
Series-2: https://lkml.org/lkml/2020/10/9/808
"[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support"
Nicolin Chen (5):
iommu/tegra-smmu: Unwrap tegra_smmu_group_get
iommu/tegra-smmu: Expand mutex protection range
iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
iommu/tegra-smmu: Rework tegra_smmu_probe_device()
iommu/tegra-smmu: Add PCI support
drivers/iommu/tegra-smmu.c | 240 ++++++++++++++-----------------------
1 file changed, 88 insertions(+), 152 deletions(-)
--
2.17.1
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.
So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.
Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/tegra-smmu.c | 56 ++++++++++++++++----------------------
1 file changed, 23 insertions(+), 33 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
static int tegra_smmu_attach_dev(struct iommu_domain *domain,
struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
- struct of_phandle_args args;
- unsigned int index = 0;
- int err = 0;
-
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args)) {
- unsigned int swgroup = args.args[0];
-
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
+ unsigned int index;
+ int err;
- of_node_put(args.np);
+ if (!fwspec)
+ return -ENOENT;
+ for (index = 0; index < fwspec->num_ids; index++) {
err = tegra_smmu_as_prepare(smmu, as);
- if (err < 0)
- return err;
+ if (err)
+ goto disable;
- tegra_smmu_enable(smmu, swgroup, as->id);
- index++;
+ tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
if (index == 0)
return -ENODEV;
return 0;
+
+disable:
+ while (index--) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+ tegra_smmu_as_unprepare(smmu, as);
+ }
+
+ return err;
}
static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
struct tegra_smmu *smmu = as->smmu;
- struct of_phandle_args args;
- unsigned int index = 0;
-
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args)) {
- unsigned int swgroup = args.args[0];
+ unsigned int index;
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
-
- of_node_put(args.np);
+ if (!fwspec)
+ return;
- tegra_smmu_disable(smmu, swgroup, as->id);
+ for (index = 0; index < fwspec->num_ids; index++) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
tegra_smmu_as_unprepare(smmu, as);
- index++;
}
}
--
2.17.1
This is used to protect potential race condition at use_count.
since probes of client drivers, calling attach_dev(), may run
concurrently.
Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ec4c9dafff95..6a3ecc334481 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
{
unsigned long id;
- mutex_lock(&smmu->lock);
-
id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids);
- if (id >= smmu->soc->num_asids) {
- mutex_unlock(&smmu->lock);
+ if (id >= smmu->soc->num_asids)
return -ENOSPC;
- }
set_bit(id, smmu->asids);
*idp = id;
- mutex_unlock(&smmu->lock);
return 0;
}
static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id)
{
- mutex_lock(&smmu->lock);
clear_bit(id, smmu->asids);
- mutex_unlock(&smmu->lock);
}
static bool tegra_smmu_capable(enum iommu_cap cap)
@@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
{
u32 value;
- int err;
+ int err = 0;
+
+ mutex_lock(&smmu->lock);
if (as->use_count > 0) {
as->use_count++;
- return 0;
+ goto unlock;
}
as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
DMA_TO_DEVICE);
- if (dma_mapping_error(smmu->dev, as->pd_dma))
- return -ENOMEM;
+ if (dma_mapping_error(smmu->dev, as->pd_dma)) {
+ err = -ENOMEM;
+ goto unlock;
+ }
/* We can't handle 64-bit DMA addresses */
if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
@@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
as->smmu = smmu;
as->use_count++;
+ mutex_unlock(&smmu->lock);
+
return 0;
err_unmap:
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
+unlock:
+ mutex_unlock(&smmu->lock);
+
return err;
}
static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
{
- if (--as->use_count > 0)
+ mutex_lock(&smmu->lock);
+
+ if (--as->use_count > 0) {
+ mutex_unlock(&smmu->lock);
return;
+ }
tegra_smmu_free_asid(smmu, as->id);
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
as->smmu = NULL;
+
+ mutex_unlock(&smmu->lock);
}
static int tegra_smmu_attach_dev(struct iommu_domain *domain,
--
2.17.1
This patch simply adds support for PCI devices.
Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index f45ed43cf8db..4a3f095a1c26 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
#include <linux/kernel.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pci.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
group->smmu = smmu;
group->soc = soc;
- group->group = iommu_group_alloc();
+ if (dev_is_pci(dev))
+ group->group = pci_device_group(dev);
+ else
+ group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(&smmu->lock);
@@ -1075,22 +1080,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
err = iommu_device_register(&smmu->iommu);
- if (err) {
- iommu_device_sysfs_remove(&smmu->iommu);
- return ERR_PTR(err);
- }
+ if (err)
+ goto remove_sysfs;
err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
- if (err < 0) {
- iommu_device_unregister(&smmu->iommu);
- iommu_device_sysfs_remove(&smmu->iommu);
- return ERR_PTR(err);
- }
+ if (err < 0)
+ goto unregister;
+
+#ifdef CONFIG_PCI
+ err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
+ if (err < 0)
+ goto unset_platform_bus;
+#endif
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
return smmu;
+
+unset_platform_bus: __maybe_unused;
+ bus_set_iommu(&platform_bus_type, NULL);
+unregister:
+ iommu_device_unregister(&smmu->iommu);
+remove_sysfs:
+ iommu_device_sysfs_remove(&smmu->iommu);
+
+ return ERR_PTR(err);
}
void tegra_smmu_remove(struct tegra_smmu *smmu)
--
2.17.1
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".
This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.
Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.
So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.
Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().
This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
takes care of it.
This also fixes a problem that previously we could add clients to
iommu groups before iommu core initializes its default domain:
ubuntu@jetson:~$ dmesg | grep iommu
platform 50000000.host1x: Adding to iommu group 1
platform 57000000.gpu: Adding to iommu group 2
iommu: Default domain type: Translated
platform 54200000.dc: Adding to iommu group 3
platform 54240000.dc: Adding to iommu group 3
platform 54340000.vic: Adding to iommu group 4
Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
iommu: Failed to allocate default IOMMU domain of type 0 for
group (null) - Falling back to IOMMU_DOMAIN_DMA
iommu: Failed to allocate default IOMMU domain of type 0 for
group (null) - Falling back to IOMMU_DOMAIN_DMA
Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
ubuntu@jetson:~$ dmesg | grep iommu
iommu: Default domain type: Translated
tegra-host1x 50000000.host1x: Adding to iommu group 0
tegra-dc 54200000.dc: Adding to iommu group 1
tegra-dc 54240000.dc: Adding to iommu group 1
tegra-vic 54340000.vic: Adding to iommu group 2
nouveau 57000000.gpu: Adding to iommu group 3
Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.
Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/tegra-smmu.c | 96 ++++++--------------------------------
1 file changed, 15 insertions(+), 81 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..f45ed43cf8db 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
- struct platform_device *pdev;
- struct tegra_mc *mc;
-
- pdev = of_find_device_by_node(np);
- if (!pdev)
- return NULL;
-
- mc = platform_get_drvdata(pdev);
- if (!mc)
- return NULL;
-
- return mc->smmu;
-}
-
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
- struct of_phandle_args *args)
-{
- const struct iommu_ops *ops = smmu->iommu.ops;
- int err;
-
- err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
- if (err < 0) {
- dev_err(dev, "failed to initialize fwspec: %d\n", err);
- return err;
- }
-
- err = ops->of_xlate(dev, args);
- if (err < 0) {
- dev_err(dev, "failed to parse SW group ID: %d\n", err);
- iommu_fwspec_free(dev);
- return err;
- }
-
- return 0;
-}
-
static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
{
- struct device_node *np = dev->of_node;
- struct tegra_smmu *smmu = NULL;
- struct of_phandle_args args;
- unsigned int index = 0;
- int err;
-
- while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args) == 0) {
- smmu = tegra_smmu_find(args.np);
- if (smmu) {
- err = tegra_smmu_configure(smmu, dev, &args);
- of_node_put(args.np);
-
- if (err < 0)
- return ERR_PTR(err);
-
- /*
- * Only a single IOMMU master interface is currently
- * supported by the Linux kernel, so abort after the
- * first match.
- */
- dev_iommu_priv_set(dev, smmu);
-
- break;
- }
-
- of_node_put(args.np);
- index++;
- }
+ struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
if (!smmu)
return ERR_PTR(-ENODEV);
@@ -873,10 +807,7 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
return &smmu->iommu;
}
-static void tegra_smmu_release_device(struct device *dev)
-{
- dev_iommu_priv_set(dev, NULL);
-}
+static void tegra_smmu_release_device(struct device *dev) {}
static const struct tegra_smmu_group_soc *
tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
@@ -953,8 +884,21 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
static int tegra_smmu_of_xlate(struct device *dev,
struct of_phandle_args *args)
{
+ struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
+ struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
u32 id = args->args[0];
+ /*
+ * Note: we are here releasing the reference of &iommu_pdev->dev, which
+ * is mc->dev. Although some functions in tegra_smmu_ops may keep using
+ * its private data beyond this point, it's still safe to do so because
+ * the SMMU parent device is the same as the MC, so the reference count
+ * isn't strictly necessary.
+ */
+ put_device(&iommu_pdev->dev);
+
+ dev_iommu_priv_set(dev, mc->smmu);
+
return iommu_fwspec_add_ids(dev, &id, 1);
}
@@ -1079,16 +1023,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
if (!smmu)
return ERR_PTR(-ENOMEM);
- /*
- * This is a bit of a hack. Ideally we'd want to simply return this
- * value. However the IOMMU registration process will attempt to add
- * all devices to the IOMMU when bus_set_iommu() is called. In order
- * not to rely on global variables to track the IOMMU instance, we
- * set it here so that it can be looked up from the .probe_device()
- * callback via the IOMMU device's .drvdata field.
- */
- mc->smmu = smmu;
-
size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
--
2.17.1
The tegra_smmu_group_get was added to group devices in different
SWGROUPs and it'd return a NULL group pointer upon a mismatch at
tegra_smmu_find_group(), so for most of clients/devices, it very
likely would mismatch and need a fallback generic_device_group().
But now tegra_smmu_group_get handles devices in same SWGROUP too,
which means that it would allocate a group for every new SWGROUP
or would directly return an existing one upon matching a SWGROUP,
i.e. any device will go through this function.
So possibility of having a NULL group pointer in device_group()
is upon failure of either devm_kzalloc() or iommu_group_alloc().
In either case, calling generic_device_group() no longer makes a
sense. Especially for devm_kzalloc() failing case, it'd cause a
problem if it fails at devm_kzalloc() yet succeeds at a fallback
generic_device_group(), because it does not create a group->list
for other devices to match.
This patch simply unwraps the function to clean it up.
Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>
Acked-by: Thierry Reding <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/tegra-smmu.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 0becdbfea306..ec4c9dafff95 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data)
mutex_unlock(&smmu->lock);
}
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
- unsigned int swgroup)
+static struct iommu_group *tegra_smmu_device_group(struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
const struct tegra_smmu_group_soc *soc;
+ unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
struct iommu_group *grp;
@@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
return group->group;
}
-static struct iommu_group *tegra_smmu_device_group(struct device *dev)
-{
- struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
- struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
- struct iommu_group *group;
-
- group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
- if (!group)
- group = generic_device_group(dev);
-
- return group;
-}
-
static int tegra_smmu_of_xlate(struct device *dev,
struct of_phandle_args *args)
{
--
2.17.1
On Wed, Nov 25, 2020 at 02:10:08AM -0800, Nicolin Chen wrote:
> Changelog
> v1->v2:
> * Added Thierry's acks to PATCH-3~5
>
> This is a merged set of resend for previously two series of patches
> that were reviewed/acked a month ago yet have not got applied.
Thanks, and sorry I missed these (I'm doing my best). I'll queue the
lot for 5.11.
Will
On Wed, 25 Nov 2020 02:10:08 -0800, Nicolin Chen wrote:
> Changelog
> v1->v2:
> * Added Thierry's acks to PATCH-3~5
>
> This is a merged set of resend for previously two series of patches
> that were reviewed/acked a month ago yet have not got applied.
>
> [...]
Applied to arm64 (for-next/iommu/tegra-smmu), thanks!
[1/5] iommu/tegra-smmu: Unwrap tegra_smmu_group_get
https://git.kernel.org/arm64/c/cf910f61aff3
[2/5] iommu/tegra-smmu: Expand mutex protection range
https://git.kernel.org/arm64/c/d5f583bf8654
[3/5] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
https://git.kernel.org/arm64/c/8750d207dc98
[4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
https://git.kernel.org/arm64/c/25938c73cd79
[5/5] iommu/tegra-smmu: Add PCI support
https://git.kernel.org/arm64/c/541f29bb0643
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
Hi Nicolin,
A regression was detected by kernelci.org in IGT's drm_read tests
on mainline, it was first seen on 17th December 2020. You can
find some details here:
https://kernelci.org/test/case/id/600b82dc1e3208f123d3dffc/
Then an automated bisection was run and it landed on this
patch (v5.10-rc3-4-g25938c73cd79 on mainline). Normally, an
email is generated automatically but I had to start this one by
hand as there were issues getting it to complete.
You can see the failing test cases with this patch:
https://lava.collabora.co.uk/results/3126405/0_igt-kms-tegra
Some errors are seen around this point in the log:
https://lava.collabora.co.uk/scheduler/job/3126405#L1005
[ 3.029729] tegra-mc 70019000.memory-controller: display0a: read @0xfe000000: EMEM address decode error (SMMU translation error [--S])
[ 3.042058] tegra-mc 70019000.memory-controller: display0a: read @0xfe000000: Page fault (SMMU translation error [--S])
Here's the same test passing with this patch reverted:
https://lava.collabora.co.uk/results/3126570/0_igt-kms-tegra
For completeness, you can see all the test jobs run by the
automated bisection here:
https://lava.collabora.co.uk/scheduler/device_type/tegra124-nyan-big?dt_length=25&dt_search=bisection-gtucker-12#dt_
Please let us know if you need any help debugging this issue or
to try a fix on this platform.
Best wishes,
Guillaume
On 25/11/2020 10:10, Nicolin Chen wrote:
> The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
> to call in tegra_smmu_probe_device() where each client searches
> its DT node for smmu pointer and swgroup ID, so as to configure
> an fwspec. But this requires a valid smmu pointer even before mc
> and smmu drivers are probed. So in tegra_smmu_probe() we added a
> line of code to fill mc->smmu, marking "a bit of a hack".
>
> This works for most of clients in the DTB, however, doesn't work
> for a client that doesn't exist in DTB, a PCI device for example.
>
> Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
> it's called from bus_set_iommu(), iommu core will let everything
> carry on. Then when a client gets probed, of_iommu_configure() in
> iommu core will search DTB for swgroup ID and call ->of_xlate()
> to prepare an fwspec, similar to tegra_smmu_probe_device() and
> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
> again, and this time we shall return smmu->iommu pointer properly.
>
> So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
> along with DT polling code by letting the iommu core handle every
> thing, except a problem that we search iommus property in DTB not
> only for swgroup ID but also for mc node to get mc->smmu pointer
> to call dev_iommu_priv_set() and return the smmu->iommu pointer.
> So we'll need to find another way to get smmu pointer.
>
> Referencing the implementation of sun50i-iommu driver, of_xlate()
> has client's dev pointer, mc node and swgroup ID. This means that
> we can call dev_iommu_priv_set() in of_xlate() instead, so we can
> simply get smmu pointer in ->probe_device().
>
> This patch reworks tegra_smmu_probe_device() by:
> 1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
> ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
> tegra_smmu_probe/tegra_mc_probe().
> 2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
> pointer in tegra_smmu_probe_device() to replace DTB polling.
> 3) Removing tegra_smmu_configure() accordingly since iommu core
> takes care of it.
>
> This also fixes a problem that previously we could add clients to
> iommu groups before iommu core initializes its default domain:
> ubuntu@jetson:~$ dmesg | grep iommu
> platform 50000000.host1x: Adding to iommu group 1
> platform 57000000.gpu: Adding to iommu group 2
> iommu: Default domain type: Translated
> platform 54200000.dc: Adding to iommu group 3
> platform 54240000.dc: Adding to iommu group 3
> platform 54340000.vic: Adding to iommu group 4
>
> Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
> warnings if switching to IOMMU_DOMAIN_DMA:
> iommu: Failed to allocate default IOMMU domain of type 0 for
> group (null) - Falling back to IOMMU_DOMAIN_DMA
> iommu: Failed to allocate default IOMMU domain of type 0 for
> group (null) - Falling back to IOMMU_DOMAIN_DMA
>
> Now, bypassing the first probe_device() call from bus_set_iommu()
> fixes the sequence:
> ubuntu@jetson:~$ dmesg | grep iommu
> iommu: Default domain type: Translated
> tegra-host1x 50000000.host1x: Adding to iommu group 0
> tegra-dc 54200000.dc: Adding to iommu group 1
> tegra-dc 54240000.dc: Adding to iommu group 1
> tegra-vic 54340000.vic: Adding to iommu group 2
> nouveau 57000000.gpu: Adding to iommu group 3
>
> Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Tested-by: Dmitry Osipenko <[email protected]>
> Acked-by: Thierry Reding <[email protected]>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> drivers/iommu/tegra-smmu.c | 96 ++++++--------------------------------
> 1 file changed, 15 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 297d49f3f80e..f45ed43cf8db 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -797,75 +797,9 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain,
> return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
> }
>
> -static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
> -{
> - struct platform_device *pdev;
> - struct tegra_mc *mc;
> -
> - pdev = of_find_device_by_node(np);
> - if (!pdev)
> - return NULL;
> -
> - mc = platform_get_drvdata(pdev);
> - if (!mc)
> - return NULL;
> -
> - return mc->smmu;
> -}
> -
> -static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
> - struct of_phandle_args *args)
> -{
> - const struct iommu_ops *ops = smmu->iommu.ops;
> - int err;
> -
> - err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
> - if (err < 0) {
> - dev_err(dev, "failed to initialize fwspec: %d\n", err);
> - return err;
> - }
> -
> - err = ops->of_xlate(dev, args);
> - if (err < 0) {
> - dev_err(dev, "failed to parse SW group ID: %d\n", err);
> - iommu_fwspec_free(dev);
> - return err;
> - }
> -
> - return 0;
> -}
> -
> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> {
> - struct device_node *np = dev->of_node;
> - struct tegra_smmu *smmu = NULL;
> - struct of_phandle_args args;
> - unsigned int index = 0;
> - int err;
> -
> - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> - &args) == 0) {
> - smmu = tegra_smmu_find(args.np);
> - if (smmu) {
> - err = tegra_smmu_configure(smmu, dev, &args);
> - of_node_put(args.np);
> -
> - if (err < 0)
> - return ERR_PTR(err);
> -
> - /*
> - * Only a single IOMMU master interface is currently
> - * supported by the Linux kernel, so abort after the
> - * first match.
> - */
> - dev_iommu_priv_set(dev, smmu);
> -
> - break;
> - }
> -
> - of_node_put(args.np);
> - index++;
> - }
> + struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>
> if (!smmu)
> return ERR_PTR(-ENODEV);
> @@ -873,10 +807,7 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> return &smmu->iommu;
> }
>
> -static void tegra_smmu_release_device(struct device *dev)
> -{
> - dev_iommu_priv_set(dev, NULL);
> -}
> +static void tegra_smmu_release_device(struct device *dev) {}
>
> static const struct tegra_smmu_group_soc *
> tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
> @@ -953,8 +884,21 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
> static int tegra_smmu_of_xlate(struct device *dev,
> struct of_phandle_args *args)
> {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> u32 id = args->args[0];
>
> + /*
> + * Note: we are here releasing the reference of &iommu_pdev->dev, which
> + * is mc->dev. Although some functions in tegra_smmu_ops may keep using
> + * its private data beyond this point, it's still safe to do so because
> + * the SMMU parent device is the same as the MC, so the reference count
> + * isn't strictly necessary.
> + */
> + put_device(&iommu_pdev->dev);
> +
> + dev_iommu_priv_set(dev, mc->smmu);
> +
> return iommu_fwspec_add_ids(dev, &id, 1);
> }
>
> @@ -1079,16 +1023,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
> if (!smmu)
> return ERR_PTR(-ENOMEM);
>
> - /*
> - * This is a bit of a hack. Ideally we'd want to simply return this
> - * value. However the IOMMU registration process will attempt to add
> - * all devices to the IOMMU when bus_set_iommu() is called. In order
> - * not to rely on global variables to track the IOMMU instance, we
> - * set it here so that it can be looked up from the .probe_device()
> - * callback via the IOMMU device's .drvdata field.
> - */
> - mc->smmu = smmu;
> -
> size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
>
> smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
>
Hi Guillaume,
On Thu, Feb 04, 2021 at 11:10:15AM +0000, Guillaume Tucker wrote:
> Hi Nicolin,
>
> A regression was detected by kernelci.org in IGT's drm_read tests
> on mainline, it was first seen on 17th December 2020. You can
> find some details here:
>
> https://kernelci.org/test/case/id/600b82dc1e3208f123d3dffc/
Thanks for reporting the issue. We did test on Tegra210 and Tegra30
yet not on Tegra124. I am wondering what could go wrong...
> Please let us know if you need any help debugging this issue or
> to try a fix on this platform.
Yes, I don't have any Tegra124 platform to run. It'd be very nice
if you can run some debugging patch (I can provide you) and a fix
after I root cause the issue.
Thanks
Nicolin
Hi Guillaume,
On Thu, Feb 04, 2021 at 09:24:23PM -0800, Nicolin Chen wrote:
> > Please let us know if you need any help debugging this issue or
> > to try a fix on this platform.
>
> Yes, I don't have any Tegra124 platform to run. It'd be very nice
> if you can run some debugging patch (I can provide you) and a fix
> after I root cause the issue.
Would it be possible for you to run with the given debugging patch?
It'd be nicer if I can get both logs of the vanilla kernel (failing)
and the commit-reverted version (passing), each applying this patch.
Thanks in advance!
Nicolin
On 05/02/2021 09:45, Nicolin Chen wrote:
> Hi Guillaume,
>
> On Thu, Feb 04, 2021 at 09:24:23PM -0800, Nicolin Chen wrote:
>>> Please let us know if you need any help debugging this issue or
>>> to try a fix on this platform.
>>
>> Yes, I don't have any Tegra124 platform to run. It'd be very nice
>> if you can run some debugging patch (I can provide you) and a fix
>> after I root cause the issue.
>
> Would it be possible for you to run with the given debugging patch?
>
> It'd be nicer if I can get both logs of the vanilla kernel (failing)
> and the commit-reverted version (passing), each applying this patch.
Sure, I've run 3 jobs:
* v5.11-rc6 as a reference, to see the original issue:
https://lava.collabora.co.uk/scheduler/job/3187848
* + your debug patch:
https://lava.collabora.co.uk/scheduler/job/3187849
* + the "breaking" commit reverted, passing the tests:
https://lava.collabora.co.uk/scheduler/job/3187851
You can see the history of the test branch I'm using here, with
the 3 revisions mentioned above:
https://gitlab.collabora.com/gtucker/linux/-/commits/linux-5.11-rc6-nyan-big-drm-read/
Hope that helps,
Guillaume
Hi Guillaume,
On Sat, Feb 06, 2021 at 01:40:13PM +0000, Guillaume Tucker wrote:
> > It'd be nicer if I can get both logs of the vanilla kernel (failing)
> > and the commit-reverted version (passing), each applying this patch.
>
> Sure, I've run 3 jobs:
>
> * v5.11-rc6 as a reference, to see the original issue:
> https://lava.collabora.co.uk/scheduler/job/3187848
>
> * + your debug patch:
> https://lava.collabora.co.uk/scheduler/job/3187849
>
> * + the "breaking" commit reverted, passing the tests:
> https://lava.collabora.co.uk/scheduler/job/3187851
Thanks for the help!
I am able to figure out what's probably wrong, yet not so sure
about the best solution at this point.
Would it be possible for you to run one more time with another
debugging patch? I'd like to see the same logs as previous:
1. Vanilla kernel + debug patch
2. Vanilla kernel + Reverted + debug patch
Thank you
Nicolin
On 10/02/2021 08:20, Nicolin Chen wrote:
> Hi Guillaume,
>
> On Sat, Feb 06, 2021 at 01:40:13PM +0000, Guillaume Tucker wrote:
>>> It'd be nicer if I can get both logs of the vanilla kernel (failing)
>>> and the commit-reverted version (passing), each applying this patch.
>>
>> Sure, I've run 3 jobs:
>>
>> * v5.11-rc6 as a reference, to see the original issue:
>> https://lava.collabora.co.uk/scheduler/job/3187848
>>
>> * + your debug patch:
>> https://lava.collabora.co.uk/scheduler/job/3187849
>>
>> * + the "breaking" commit reverted, passing the tests:
>> https://lava.collabora.co.uk/scheduler/job/3187851
>
> Thanks for the help!
>
> I am able to figure out what's probably wrong, yet not so sure
> about the best solution at this point.
>
> Would it be possible for you to run one more time with another
> debugging patch? I'd like to see the same logs as previous:
> 1. Vanilla kernel + debug patch
> 2. Vanilla kernel + Reverted + debug patch
As it turns out, next-20210210 is passing all the tests again so
it looks like this got fixed in the meantime:
https://lava.collabora.co.uk/scheduler/job/3210192
https://lava.collabora.co.uk/results/3210192/0_igt-kms-tegra
And here's a more extensive list of IGT tests on next-20210211,
all the regressions have been fixed:
https://kernelci.org/test/plan/id/60254c42f51df36be53abe62/
I haven't run a reversed bisection to find the fix, but I guess
it wouldn't be too hard to find out what happened by hand anyway.
I see the drm/tegra/for-5.12-rc1 tag has been merged into
linux-next, maybe that solved the issue?
FYI I've also run some jobs with your debug patch and with the
breaking patch reverted:
https://lava.collabora.co.uk/scheduler/job/3210245
https://lava.collabora.co.uk/scheduler/job/3210596
Meanwhile I'll see what can be done to improve the automated
bisection so if there are new IGT regressions they would get
reported earlier. I guess it would have saved us all some time
if it had been bisected in December.
Thanks,
Guillaume
Hi Guillaume,
Thank you for the test results! And sorry for my belated reply.
On Thu, Feb 11, 2021 at 03:50:05PM +0000, Guillaume Tucker wrote:
> > On Sat, Feb 06, 2021 at 01:40:13PM +0000, Guillaume Tucker wrote:
> >>> It'd be nicer if I can get both logs of the vanilla kernel (failing)
> >>> and the commit-reverted version (passing), each applying this patch.
> >>
> >> Sure, I've run 3 jobs:
> >>
> >> * v5.11-rc6 as a reference, to see the original issue:
> >> https://lava.collabora.co.uk/scheduler/job/3187848
> >>
> >> * + your debug patch:
> >> https://lava.collabora.co.uk/scheduler/job/3187849
> >>
> >> * + the "breaking" commit reverted, passing the tests:
> >> https://lava.collabora.co.uk/scheduler/job/3187851
> >
> > Thanks for the help!
> >
> > I am able to figure out what's probably wrong, yet not so sure
> > about the best solution at this point.
> >
> > Would it be possible for you to run one more time with another
> > debugging patch? I'd like to see the same logs as previous:
> > 1. Vanilla kernel + debug patch
> > 2. Vanilla kernel + Reverted + debug patch
>
> As it turns out, next-20210210 is passing all the tests again so
> it looks like this got fixed in the meantime:
>
> https://lava.collabora.co.uk/scheduler/job/3210192
I checked this passing log, however, found that the regression is
still there though test passed, as the prints below aren't normal:
tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
EMEM address decode error (SMMU translation error [--S])
tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
Page fault (SMMU translation error [--S])
I was trying to think of a simpler solution than a revert. However,
given the fact that the callback sequence could change -- guessing
likely a recent change in iommu core, I feel it safer to revert my
previous change, not necessarily being a complete revert though.
I attached my partial reverting change in this email. Would it be
possible for you to run one more test for me to confirm it? It'd
keep the tests passing while eliminating all error prints above.
If the fix works, I'll re-send it to mail list by adding a commit
message.
Thanks!
Nicolin
On 18/02/2021 10:35, Nicolin Chen wrote:
> Hi Guillaume,
>
> Thank you for the test results! And sorry for my belated reply.
No worries :)
> On Thu, Feb 11, 2021 at 03:50:05PM +0000, Guillaume Tucker wrote:
>>> On Sat, Feb 06, 2021 at 01:40:13PM +0000, Guillaume Tucker wrote:
>>>>> It'd be nicer if I can get both logs of the vanilla kernel (failing)
>>>>> and the commit-reverted version (passing), each applying this patch.
>>>>
>>>> Sure, I've run 3 jobs:
>>>>
>>>> * v5.11-rc6 as a reference, to see the original issue:
>>>> https://lava.collabora.co.uk/scheduler/job/3187848
>>>>
>>>> * + your debug patch:
>>>> https://lava.collabora.co.uk/scheduler/job/3187849
>>>>
>>>> * + the "breaking" commit reverted, passing the tests:
>>>> https://lava.collabora.co.uk/scheduler/job/3187851
>>>
>>> Thanks for the help!
>>>
>>> I am able to figure out what's probably wrong, yet not so sure
>>> about the best solution at this point.
>>>
>>> Would it be possible for you to run one more time with another
>>> debugging patch? I'd like to see the same logs as previous:
>>> 1. Vanilla kernel + debug patch
>>> 2. Vanilla kernel + Reverted + debug patch
>>
>> As it turns out, next-20210210 is passing all the tests again so
>> it looks like this got fixed in the meantime:
>>
>> https://lava.collabora.co.uk/scheduler/job/3210192
>
> I checked this passing log, however, found that the regression is
> still there though test passed, as the prints below aren't normal:
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
> EMEM address decode error (SMMU translation error [--S])
> tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
> Page fault (SMMU translation error [--S])
Ah yes sorry, there are other KernelCI checks for kernel errors
but that wasn't enabled in the bisection so I didn't notice them.
> I was trying to think of a simpler solution than a revert. However,
> given the fact that the callback sequence could change -- guessing
> likely a recent change in iommu core, I feel it safer to revert my
> previous change, not necessarily being a complete revert though.
>
> I attached my partial reverting change in this email. Would it be
> possible for you to run one more test for me to confirm it? It'd
> keep the tests passing while eliminating all error prints above.
>
> If the fix works, I'll re-send it to mail list by adding a commit
> message.
Sure, here's next-20210218 as a reference:
https://lava.collabora.co.uk/scheduler/job/3241236
and here with your patch applied on top of it:
https://lava.collabora.co.uk/scheduler/job/3241246
The git branch I've used where your patch is applied:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210218-nyan-big-drm-read/
The errors seem to have disappeared but I'll let you double check
that things are all back to a working state.
BTW: This thread is a good example of how having an "on-demand"
KernelCI service to let developers re-run tests with extra
patches would allow them to fix issues independently. We'll keep
that in mind for the future.
Best wishes,
Guillaume