[PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hello,

This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using recently introduce device
links, which lets SYSMMU controller's runtime PM to follow master's device
runtime PM state (the device which actually performs DMA transaction).
The main idea behind this solution is an observation that any DMA activity
from master device can be done only when master device is active, thus when
master device is suspended SYSMMU controller device can also be suspended.

This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).

Patch requires second version of Rafeal's "Functional dependencies
between devices" patchset, which is available here:
https://lkml.org/lkml/2016/9/8/798

If one wants to test this patchset, I've provided a branch with all needed
patches (some fixes for Exynos4 FIMC-IS driver are needed):
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v3

Patches are based on vanilla v4.8-rc6 kernel with Rafael's patches applied.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v3:
- rebased on top of latest device dependencies/links patchset
- added proper locking between runtime pm, iommu_attach/detach and sysmmu
  enable/disable(added per iommu owner device's rpm lock)

v2:
- replaced PM notifiers with generic device dependencies/links developped
  by Rafael J. Wysocki

v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version


Patch summary:

Marek Szyprowski (2):
  iommu/exynos: Remove excessive, useless debug
  iommu/exynos: Add proper runtime pm support

 drivers/iommu/exynos-iommu.c | 228 ++++++++++++++++++-------------------------
 1 file changed, 94 insertions(+), 134 deletions(-)

-- 
1.9.1

[PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hello,

This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using recently introduce device
links, which lets SYSMMU controller's runtime PM to follow master's device
runtime PM state (the device which actually performs DMA transaction).
The main idea behind this solution is an observation that any DMA activity
from master device can be done only when master device is active, thus when
master device is suspended SYSMMU controller device can also be suspended.

This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).

Patch requires second version of Rafeal's "Functional dependencies
between devices" patchset, which is available here:
https://lkml.org/lkml/2016/9/8/798

If one wants to test this patchset, I've provided a branch with all needed
patches (some fixes for Exynos4 FIMC-IS driver are needed):
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v3

Patches are based on vanilla v4.8-rc6 kernel with Rafael's patches applied.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v3:
- rebased on top of latest device dependencies/links patchset
- added proper locking between runtime pm, iommu_attach/detach and sysmmu
  enable/disable(added per iommu owner device's rpm lock)

v2:
- replaced PM notifiers with generic device dependencies/links developped
  by Rafael J. Wysocki

v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version


Patch summary:

Marek Szyprowski (2):
  iommu/exynos: Remove excessive, useless debug
  iommu/exynos: Add proper runtime pm support

 drivers/iommu/exynos-iommu.c | 228 ++++++++++++++++++-------------------------
 1 file changed, 94 insertions(+), 134 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] iommu/exynos: Remove excessive, useless debug

[Marek Szyprowski]

Remove excessive, useless debug about skipping TLB invalidation, which
is a normal situation when more aggressive power management is enabled.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 75fbe5d30cb3..b0fa4d432e71 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -579,9 +579,6 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 			sysmmu_unblock(data);
 		}
 		clk_disable(data->clk_master);
-	} else {
-		dev_dbg(data->master,
-			"disabled. Skipping TLB invalidation @ %#x\n", iova);
 	}
 	spin_unlock_irqrestore(&data->lock, flags);
 }
-- 
1.9.1

[PATCH v3 1/2] iommu/exynos: Remove excessive, useless debug

[Marek Szyprowski]

Remove excessive, useless debug about skipping TLB invalidation, which
is a normal situation when more aggressive power management is enabled.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/exynos-iommu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 75fbe5d30cb3..b0fa4d432e71 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -579,9 +579,6 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 			sysmmu_unblock(data);
 		}
 		clk_disable(data->clk_master);
-	} else {
-		dev_dbg(data->master,
-			"disabled. Skipping TLB invalidation @ %#x\n", iova);
 	}
 	spin_unlock_irqrestore(&data->lock, flags);
 }
-- 
1.9.1

[PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Marek Szyprowski]

This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active and can save/restore its state
instead of being enabled all the time. This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
 1 file changed, 94 insertions(+), 131 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0fa4d432e71..34717a0b1902 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -206,6 +206,7 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
 struct exynos_iommu_owner {
 	struct list_head controllers;	/* list of sysmmu_drvdata.owner_node */
 	struct iommu_domain *domain;	/* domain this device is attached */
+	struct mutex rpm_lock;		/* for runtime pm of all sysmmus */
 };
 
 /*
@@ -237,8 +238,8 @@ struct sysmmu_drvdata {
 	struct clk *aclk;		/* SYSMMU's aclk clock */
 	struct clk *pclk;		/* SYSMMU's pclk clock */
 	struct clk *clk_master;		/* master's device clock */
-	int activations;		/* number of calls to sysmmu_enable */
 	spinlock_t lock;		/* lock for modyfying state */
+	int active;			/* current status */
 	struct exynos_iommu_domain *domain; /* domain we belong to */
 	struct list_head domain_node;	/* node for domain clients list */
 	struct list_head owner_node;	/* node for owner controllers list */
@@ -251,25 +252,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
 	return container_of(dom, struct exynos_iommu_domain, domain);
 }
 
-static bool set_sysmmu_active(struct sysmmu_drvdata *data)
-{
-	/* return true if the System MMU was not active previously
-	   and it needs to be initialized */
-	return ++data->activations == 1;
-}
-
-static bool set_sysmmu_inactive(struct sysmmu_drvdata *data)
-{
-	/* return true if the System MMU is needed to be disabled */
-	BUG_ON(data->activations < 1);
-	return --data->activations == 0;
-}
-
-static bool is_sysmmu_active(struct sysmmu_drvdata *data)
-{
-	return data->activations > 0;
-}
-
 static void sysmmu_unblock(struct sysmmu_drvdata *data)
 {
 	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
@@ -389,7 +371,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	unsigned short reg_status, reg_clear;
 	int ret = -ENOSYS;
 
-	WARN_ON(!is_sysmmu_active(data));
+	WARN_ON(!data->active);
 
 	if (MMU_MAJ_VER(data->version) < 5) {
 		reg_status = REG_INT_STATUS;
@@ -435,40 +417,19 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_disable(struct sysmmu_drvdata *data)
 {
+	unsigned long flags;
+
 	clk_enable(data->clk_master);
 
+	spin_lock_irqsave(&data->lock, flags);
 	writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
 	writel(0, data->sfrbase + REG_MMU_CFG);
-
-	__sysmmu_disable_clocks(data);
-}
-
-static bool __sysmmu_disable(struct sysmmu_drvdata *data)
-{
-	bool disabled;
-	unsigned long flags;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	disabled = set_sysmmu_inactive(data);
-
-	if (disabled) {
-		data->pgtable = 0;
-		data->domain = NULL;
-
-		__sysmmu_disable_nocount(data);
-
-		dev_dbg(data->sysmmu, "Disabled\n");
-	} else  {
-		dev_dbg(data->sysmmu, "%d times left to disable\n",
-					data->activations);
-	}
-
+	data->active = false;
 	spin_unlock_irqrestore(&data->lock, flags);
 
-	return disabled;
+	__sysmmu_disable_clocks(data);
 }
 
 static void __sysmmu_init_config(struct sysmmu_drvdata *data)
@@ -485,17 +446,19 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 	writel(cfg, data->sfrbase + REG_MMU_CFG);
 }
 
-static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_enable(struct sysmmu_drvdata *data)
 {
+	unsigned long flags;
+
 	__sysmmu_enable_clocks(data);
 
+	spin_lock_irqsave(&data->lock, flags);
 	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
-
 	__sysmmu_init_config(data);
-
 	__sysmmu_set_ptbase(data, data->pgtable);
-
 	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
+	data->active = true;
+	spin_unlock_irqrestore(&data->lock, flags);
 
 	/*
 	 * SYSMMU driver keeps master's clock enabled only for the short
@@ -506,48 +469,18 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
 	clk_disable(data->clk_master);
 }
 
-static int __sysmmu_enable(struct sysmmu_drvdata *data, phys_addr_t pgtable,
-			   struct exynos_iommu_domain *domain)
-{
-	int ret = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&data->lock, flags);
-	if (set_sysmmu_active(data)) {
-		data->pgtable = pgtable;
-		data->domain = domain;
-
-		__sysmmu_enable_nocount(data);
-
-		dev_dbg(data->sysmmu, "Enabled\n");
-	} else {
-		ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
-
-		dev_dbg(data->sysmmu, "already enabled\n");
-	}
-
-	if (WARN_ON(ret < 0))
-		set_sysmmu_inactive(data); /* decrement count */
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return ret;
-}
-
 static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
 					    sysmmu_iova_t iova)
 {
 	unsigned long flags;
 
-
 	spin_lock_irqsave(&data->lock, flags);
-	if (is_sysmmu_active(data) && data->version >= MAKE_MMU_VER(3, 3)) {
+	if (data->active && data->version >= MAKE_MMU_VER(3, 3)) {
 		clk_enable(data->clk_master);
 		__sysmmu_tlb_invalidate_entry(data, iova, 1);
 		clk_disable(data->clk_master);
 	}
 	spin_unlock_irqrestore(&data->lock, flags);
-
 }
 
 static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
@@ -556,7 +489,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 	unsigned long flags;
 
 	spin_lock_irqsave(&data->lock, flags);
-	if (is_sysmmu_active(data)) {
+	if (data->active) {
 		unsigned int num_inv = 1;
 
 		clk_enable(data->clk_master);
@@ -657,40 +590,55 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 	}
 
 	pm_runtime_enable(dev);
-
 	of_iommu_set_ops(dev->of_node, &exynos_iommu_ops);
 
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int exynos_sysmmu_suspend(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	struct exynos_iommu_owner *owner;
 
-	dev_dbg(dev, "suspend\n");
-	if (is_sysmmu_active(data)) {
-		__sysmmu_disable_nocount(data);
-		pm_runtime_put(dev);
+	if (!data->master)
+		return 0;
+
+	owner = data->master->archdata.iommu;
+
+	mutex_lock(&owner->rpm_lock);
+	if (data->domain) {
+		dev_dbg(data->sysmmu, "saving state\n");
+		__sysmmu_disable(data);
 	}
+	mutex_unlock(&owner->rpm_lock);
+
 	return 0;
 }
 
 static int exynos_sysmmu_resume(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	struct exynos_iommu_owner *owner;
 
-	dev_dbg(dev, "resume\n");
-	if (is_sysmmu_active(data)) {
-		pm_runtime_get_sync(dev);
-		__sysmmu_enable_nocount(data);
+	if (!data->master)
+		return 0;
+
+	owner = data->master->archdata.iommu;
+
+	mutex_lock(&owner->rpm_lock);
+	if (data->domain) {
+		dev_dbg(data->sysmmu, "restoring state\n");
+		__sysmmu_enable(data);
 	}
+	mutex_unlock(&owner->rpm_lock);
+
 	return 0;
 }
-#endif
 
 static const struct dev_pm_ops sysmmu_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume)
+	SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
 };
 
 static const struct of_device_id sysmmu_of_match[] __initconst = {
@@ -794,9 +742,12 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
 	spin_lock_irqsave(&domain->lock, flags);
 
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
-		if (__sysmmu_disable(data))
-			data->master = NULL;
+		spin_lock(&data->lock);
+		__sysmmu_disable(data);
+		data->pgtable = 0;
+		data->domain = NULL;
 		list_del_init(&data->domain_node);
+		spin_unlock(&data->lock);
 	}
 
 	spin_unlock_irqrestore(&domain->lock, flags);
@@ -830,31 +781,32 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
 	struct sysmmu_drvdata *data, *next;
 	unsigned long flags;
-	bool found = false;
 
 	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
 		return;
 
+	mutex_lock(&owner->rpm_lock);
+
+	list_for_each_entry(data, &owner->controllers, owner_node) {
+		if (pm_runtime_active(data->sysmmu))
+			__sysmmu_disable(data);
+	}
+
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
-		if (data->master == dev) {
-			if (__sysmmu_disable(data)) {
-				data->master = NULL;
-				list_del_init(&data->domain_node);
-			}
-			pm_runtime_put(data->sysmmu);
-			found = true;
-		}
+		spin_lock(&data->lock);
+		data->pgtable = 0;
+		data->domain = NULL;
+		list_del_init(&data->domain_node);
+		spin_unlock(&data->lock);
 	}
+	owner->domain = NULL;
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-	owner->domain = NULL;
+	mutex_unlock(&owner->rpm_lock);
 
-	if (found)
-		dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n",
-					__func__, &pagetable);
-	else
-		dev_err(dev, "%s: No IOMMU is attached\n", __func__);
+	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
+		&pagetable);
 }
 
 static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
@@ -865,7 +817,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	struct sysmmu_drvdata *data;
 	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
 	unsigned long flags;
-	int ret = -ENODEV;
 
 	if (!has_sysmmu(dev))
 		return -ENODEV;
@@ -873,29 +824,30 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	if (owner->domain)
 		exynos_iommu_detach_device(owner->domain, dev);
 
+	mutex_lock(&owner->rpm_lock);
+
+	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry(data, &owner->controllers, owner_node) {
-		pm_runtime_get_sync(data->sysmmu);
-		ret = __sysmmu_enable(data, pagetable, domain);
-		if (ret >= 0) {
-			data->master = dev;
-
-			spin_lock_irqsave(&domain->lock, flags);
-			list_add_tail(&data->domain_node, &domain->clients);
-			spin_unlock_irqrestore(&domain->lock, flags);
-		}
+		spin_lock(&data->lock);
+		data->pgtable = pagetable;
+		data->domain = domain;
+		list_add_tail(&data->domain_node, &domain->clients);
+		spin_unlock(&data->lock);
 	}
+	owner->domain = iommu_domain;
+	spin_unlock_irqrestore(&domain->lock, flags);
 
-	if (ret < 0) {
-		dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n",
-					__func__, &pagetable);
-		return ret;
+	list_for_each_entry(data, &owner->controllers, owner_node) {
+		if (pm_runtime_active(data->sysmmu))
+			__sysmmu_enable(data);
 	}
 
-	owner->domain = iommu_domain;
-	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n",
-		__func__, &pagetable, (ret == 0) ? "" : ", again");
+	mutex_unlock(&owner->rpm_lock);
 
-	return ret;
+	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
+		&pagetable);
+
+	return 0;
 }
 
 static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
@@ -1266,10 +1218,21 @@ static int exynos_iommu_of_xlate(struct device *dev,
 			return -ENOMEM;
 
 		INIT_LIST_HEAD(&owner->controllers);
+		mutex_init(&owner->rpm_lock);
 		dev->archdata.iommu = owner;
 	}
 
 	list_add_tail(&data->owner_node, &owner->controllers);
+	data->master = dev;
+
+	/*
+	 * SYSMMU will be runtime enabled via device link (dependency) to its
+	 * master device, so there are no direct calls to pm_runtime_get/put
+	 * in this driver.
+	 */
+	device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
+			DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Ulf Hansson]

On 13 September 2016 at 14:49, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> This patch uses recently introduced device links to track the runtime pm
> state of the master's device. This way each SYSMMU controller is runtime
> activated when its master's device is active and can save/restore its state
> instead of being enabled all the time. This way SYSMMU controllers no
> longer prevents respective power domains to be turned off when master's
> device is not used.

Apologize for not reviewing earlier and if you find my
questions/suggestions being silly. You may ignore them, if you don't
think they deserves a proper answer. :-)

I am not so familiar with the IOMMU subsystem, but I am wondering
whether the issue you are solving, is similar to what can be observed
for DMA and serial drivers. And of course also for other IOMMU
drivers.

In general the DMA/serial drivers requires to use the
pm_runtime_irq_safe() option, to be able to easily deploy runtime PM
support (of course there are some other workarounds as well).

As we know, using the pm_runtime_irq_safe() option comes with some
limitations, such as the runtime PM callbacks is not allowed to sleep.
For a PM domain (genpd) that is attached to the device, this also
means it must not be powered off.

To solve this problem, I was thinking we could convert to use the
asynchronous pm_runtime_get() API, when trying to runtime resume the
device from atomic contexts.

Of course when it turns out that the device isn't yet runtime resumed
immediately after calling pm_runtime_get(), the request needs to be
put on a request queue to be managed shortly after instead. Doing it
like this, would remove the need to use the pm_runtime_irq_safe()
option.

I realize that such change needs to be implemented in common code for
IOMMU drivers, if at all possible.

Anyway, I hope you at least get the idea and I just wanted to mention
that I have been exploring this option for DMA and serial drivers.

Kind regards
Uffe

>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
>  1 file changed, 94 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index b0fa4d432e71..34717a0b1902 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -206,6 +206,7 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
>  struct exynos_iommu_owner {
>         struct list_head controllers;   /* list of sysmmu_drvdata.owner_node */
>         struct iommu_domain *domain;    /* domain this device is attached */
> +       struct mutex rpm_lock;          /* for runtime pm of all sysmmus */
>  };
>
>  /*
> @@ -237,8 +238,8 @@ struct sysmmu_drvdata {
>         struct clk *aclk;               /* SYSMMU's aclk clock */
>         struct clk *pclk;               /* SYSMMU's pclk clock */
>         struct clk *clk_master;         /* master's device clock */
> -       int activations;                /* number of calls to sysmmu_enable */
>         spinlock_t lock;                /* lock for modyfying state */
> +       int active;                     /* current status */
>         struct exynos_iommu_domain *domain; /* domain we belong to */
>         struct list_head domain_node;   /* node for domain clients list */
>         struct list_head owner_node;    /* node for owner controllers list */
> @@ -251,25 +252,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
>         return container_of(dom, struct exynos_iommu_domain, domain);
>  }
>
> -static bool set_sysmmu_active(struct sysmmu_drvdata *data)
> -{
> -       /* return true if the System MMU was not active previously
> -          and it needs to be initialized */
> -       return ++data->activations == 1;
> -}
> -
> -static bool set_sysmmu_inactive(struct sysmmu_drvdata *data)
> -{
> -       /* return true if the System MMU is needed to be disabled */
> -       BUG_ON(data->activations < 1);
> -       return --data->activations == 0;
> -}
> -
> -static bool is_sysmmu_active(struct sysmmu_drvdata *data)
> -{
> -       return data->activations > 0;
> -}
> -
>  static void sysmmu_unblock(struct sysmmu_drvdata *data)
>  {
>         writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> @@ -389,7 +371,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>         unsigned short reg_status, reg_clear;
>         int ret = -ENOSYS;
>
> -       WARN_ON(!is_sysmmu_active(data));
> +       WARN_ON(!data->active);
>
>         if (MMU_MAJ_VER(data->version) < 5) {
>                 reg_status = REG_INT_STATUS;
> @@ -435,40 +417,19 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
>         return IRQ_HANDLED;
>  }
>
> -static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
> +static void __sysmmu_disable(struct sysmmu_drvdata *data)
>  {
> +       unsigned long flags;
> +
>         clk_enable(data->clk_master);
>
> +       spin_lock_irqsave(&data->lock, flags);
>         writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
>         writel(0, data->sfrbase + REG_MMU_CFG);
> -
> -       __sysmmu_disable_clocks(data);
> -}
> -
> -static bool __sysmmu_disable(struct sysmmu_drvdata *data)
> -{
> -       bool disabled;
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&data->lock, flags);
> -
> -       disabled = set_sysmmu_inactive(data);
> -
> -       if (disabled) {
> -               data->pgtable = 0;
> -               data->domain = NULL;
> -
> -               __sysmmu_disable_nocount(data);
> -
> -               dev_dbg(data->sysmmu, "Disabled\n");
> -       } else  {
> -               dev_dbg(data->sysmmu, "%d times left to disable\n",
> -                                       data->activations);
> -       }
> -
> +       data->active = false;
>         spin_unlock_irqrestore(&data->lock, flags);
>
> -       return disabled;
> +       __sysmmu_disable_clocks(data);
>  }
>
>  static void __sysmmu_init_config(struct sysmmu_drvdata *data)
> @@ -485,17 +446,19 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
>         writel(cfg, data->sfrbase + REG_MMU_CFG);
>  }
>
> -static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
> +static void __sysmmu_enable(struct sysmmu_drvdata *data)
>  {
> +       unsigned long flags;
> +
>         __sysmmu_enable_clocks(data);
>
> +       spin_lock_irqsave(&data->lock, flags);
>         writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
> -
>         __sysmmu_init_config(data);
> -
>         __sysmmu_set_ptbase(data, data->pgtable);
> -
>         writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
> +       data->active = true;
> +       spin_unlock_irqrestore(&data->lock, flags);
>
>         /*
>          * SYSMMU driver keeps master's clock enabled only for the short
> @@ -506,48 +469,18 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
>         clk_disable(data->clk_master);
>  }
>
> -static int __sysmmu_enable(struct sysmmu_drvdata *data, phys_addr_t pgtable,
> -                          struct exynos_iommu_domain *domain)
> -{
> -       int ret = 0;
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&data->lock, flags);
> -       if (set_sysmmu_active(data)) {
> -               data->pgtable = pgtable;
> -               data->domain = domain;
> -
> -               __sysmmu_enable_nocount(data);
> -
> -               dev_dbg(data->sysmmu, "Enabled\n");
> -       } else {
> -               ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
> -
> -               dev_dbg(data->sysmmu, "already enabled\n");
> -       }
> -
> -       if (WARN_ON(ret < 0))
> -               set_sysmmu_inactive(data); /* decrement count */
> -
> -       spin_unlock_irqrestore(&data->lock, flags);
> -
> -       return ret;
> -}
> -
>  static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
>                                             sysmmu_iova_t iova)
>  {
>         unsigned long flags;
>
> -
>         spin_lock_irqsave(&data->lock, flags);
> -       if (is_sysmmu_active(data) && data->version >= MAKE_MMU_VER(3, 3)) {
> +       if (data->active && data->version >= MAKE_MMU_VER(3, 3)) {
>                 clk_enable(data->clk_master);
>                 __sysmmu_tlb_invalidate_entry(data, iova, 1);
>                 clk_disable(data->clk_master);
>         }
>         spin_unlock_irqrestore(&data->lock, flags);
> -
>  }
>
>  static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> @@ -556,7 +489,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
>         unsigned long flags;
>
>         spin_lock_irqsave(&data->lock, flags);
> -       if (is_sysmmu_active(data)) {
> +       if (data->active) {
>                 unsigned int num_inv = 1;
>
>                 clk_enable(data->clk_master);
> @@ -657,40 +590,55 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
>         }
>
>         pm_runtime_enable(dev);
> -
>         of_iommu_set_ops(dev->of_node, &exynos_iommu_ops);
>
>         return 0;
>  }
>
> -#ifdef CONFIG_PM_SLEEP
>  static int exynos_sysmmu_suspend(struct device *dev)
>  {
>         struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> +       struct exynos_iommu_owner *owner;
>
> -       dev_dbg(dev, "suspend\n");
> -       if (is_sysmmu_active(data)) {
> -               __sysmmu_disable_nocount(data);
> -               pm_runtime_put(dev);
> +       if (!data->master)
> +               return 0;
> +
> +       owner = data->master->archdata.iommu;
> +
> +       mutex_lock(&owner->rpm_lock);
> +       if (data->domain) {
> +               dev_dbg(data->sysmmu, "saving state\n");
> +               __sysmmu_disable(data);
>         }
> +       mutex_unlock(&owner->rpm_lock);
> +
>         return 0;
>  }
>
>  static int exynos_sysmmu_resume(struct device *dev)
>  {
>         struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> +       struct exynos_iommu_owner *owner;
>
> -       dev_dbg(dev, "resume\n");
> -       if (is_sysmmu_active(data)) {
> -               pm_runtime_get_sync(dev);
> -               __sysmmu_enable_nocount(data);
> +       if (!data->master)
> +               return 0;
> +
> +       owner = data->master->archdata.iommu;
> +
> +       mutex_lock(&owner->rpm_lock);
> +       if (data->domain) {
> +               dev_dbg(data->sysmmu, "restoring state\n");
> +               __sysmmu_enable(data);
>         }
> +       mutex_unlock(&owner->rpm_lock);
> +
>         return 0;
>  }
> -#endif
>
>  static const struct dev_pm_ops sysmmu_pm_ops = {
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume)
> +       SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
> +       SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                                    pm_runtime_force_resume)
>  };
>
>  static const struct of_device_id sysmmu_of_match[] __initconst = {
> @@ -794,9 +742,12 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
>         spin_lock_irqsave(&domain->lock, flags);
>
>         list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
> -               if (__sysmmu_disable(data))
> -                       data->master = NULL;
> +               spin_lock(&data->lock);
> +               __sysmmu_disable(data);
> +               data->pgtable = 0;
> +               data->domain = NULL;
>                 list_del_init(&data->domain_node);
> +               spin_unlock(&data->lock);
>         }
>
>         spin_unlock_irqrestore(&domain->lock, flags);
> @@ -830,31 +781,32 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
>         phys_addr_t pagetable = virt_to_phys(domain->pgtable);
>         struct sysmmu_drvdata *data, *next;
>         unsigned long flags;
> -       bool found = false;
>
>         if (!has_sysmmu(dev) || owner->domain != iommu_domain)
>                 return;
>
> +       mutex_lock(&owner->rpm_lock);
> +
> +       list_for_each_entry(data, &owner->controllers, owner_node) {
> +               if (pm_runtime_active(data->sysmmu))
> +                       __sysmmu_disable(data);
> +       }
> +
>         spin_lock_irqsave(&domain->lock, flags);
>         list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
> -               if (data->master == dev) {
> -                       if (__sysmmu_disable(data)) {
> -                               data->master = NULL;
> -                               list_del_init(&data->domain_node);
> -                       }
> -                       pm_runtime_put(data->sysmmu);
> -                       found = true;
> -               }
> +               spin_lock(&data->lock);
> +               data->pgtable = 0;
> +               data->domain = NULL;
> +               list_del_init(&data->domain_node);
> +               spin_unlock(&data->lock);
>         }
> +       owner->domain = NULL;
>         spin_unlock_irqrestore(&domain->lock, flags);
>
> -       owner->domain = NULL;
> +       mutex_unlock(&owner->rpm_lock);
>
> -       if (found)
> -               dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n",
> -                                       __func__, &pagetable);
> -       else
> -               dev_err(dev, "%s: No IOMMU is attached\n", __func__);
> +       dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
> +               &pagetable);
>  }
>
>  static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
> @@ -865,7 +817,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>         struct sysmmu_drvdata *data;
>         phys_addr_t pagetable = virt_to_phys(domain->pgtable);
>         unsigned long flags;
> -       int ret = -ENODEV;
>
>         if (!has_sysmmu(dev))
>                 return -ENODEV;
> @@ -873,29 +824,30 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>         if (owner->domain)
>                 exynos_iommu_detach_device(owner->domain, dev);
>
> +       mutex_lock(&owner->rpm_lock);
> +
> +       spin_lock_irqsave(&domain->lock, flags);
>         list_for_each_entry(data, &owner->controllers, owner_node) {
> -               pm_runtime_get_sync(data->sysmmu);
> -               ret = __sysmmu_enable(data, pagetable, domain);
> -               if (ret >= 0) {
> -                       data->master = dev;
> -
> -                       spin_lock_irqsave(&domain->lock, flags);
> -                       list_add_tail(&data->domain_node, &domain->clients);
> -                       spin_unlock_irqrestore(&domain->lock, flags);
> -               }
> +               spin_lock(&data->lock);
> +               data->pgtable = pagetable;
> +               data->domain = domain;
> +               list_add_tail(&data->domain_node, &domain->clients);
> +               spin_unlock(&data->lock);
>         }
> +       owner->domain = iommu_domain;
> +       spin_unlock_irqrestore(&domain->lock, flags);
>
> -       if (ret < 0) {
> -               dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n",
> -                                       __func__, &pagetable);
> -               return ret;
> +       list_for_each_entry(data, &owner->controllers, owner_node) {
> +               if (pm_runtime_active(data->sysmmu))
> +                       __sysmmu_enable(data);
>         }
>
> -       owner->domain = iommu_domain;
> -       dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n",
> -               __func__, &pagetable, (ret == 0) ? "" : ", again");
> +       mutex_unlock(&owner->rpm_lock);
>
> -       return ret;
> +       dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
> +               &pagetable);
> +
> +       return 0;
>  }
>
>  static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
> @@ -1266,10 +1218,21 @@ static int exynos_iommu_of_xlate(struct device *dev,
>                         return -ENOMEM;
>
>                 INIT_LIST_HEAD(&owner->controllers);
> +               mutex_init(&owner->rpm_lock);
>                 dev->archdata.iommu = owner;
>         }
>
>         list_add_tail(&data->owner_node, &owner->controllers);
> +       data->master = dev;
> +
> +       /*
> +        * SYSMMU will be runtime enabled via device link (dependency) to its
> +        * master device, so there are no direct calls to pm_runtime_get/put
> +        * in this driver.
> +        */
> +       device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
> +                       DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
> +
>         return 0;
>  }
>
> --
> 1.9.1
>

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[kbuild test robot]

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

Hi Marek,

[auto build test ERROR on iommu/next]
[also build test ERROR on v4.8-rc6 next-20160913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Marek-Szyprowski/Exynos-IOMMU-proper-runtime-PM-support-use-device-dependencies/20160913-205434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/iommu/exynos-iommu.c: In function 'exynos_iommu_of_xlate':
>> drivers/iommu/exynos-iommu.c:1232:2: error: implicit declaration of function 'device_link_add' [-Werror=implicit-function-declaration]
     device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
     ^
>> drivers/iommu/exynos-iommu.c:1232:37: error: 'DEVICE_LINK_AVAILABLE' undeclared (first use in this function)
     device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
                                        ^
   drivers/iommu/exynos-iommu.c:1232:37: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iommu/exynos-iommu.c:1233:4: error: 'DEVICE_LINK_PERSISTENT' undeclared (first use in this function)
       DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
       ^
>> drivers/iommu/exynos-iommu.c:1233:29: error: 'DEVICE_LINK_PM_RUNTIME' undeclared (first use in this function)
       DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
                                ^
   cc1: some warnings being treated as errors

vim +/device_link_add +1232 drivers/iommu/exynos-iommu.c

  1226	
  1227		/*
  1228		 * SYSMMU will be runtime enabled via device link (dependency) to its
  1229		 * master device, so there are no direct calls to pm_runtime_get/put
  1230		 * in this driver.
  1231		 */
> 1232		device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
> 1233				DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
  1234	
  1235		return 0;
  1236	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 58501 bytes --]

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[kbuild test robot]

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

Hi Marek,

[auto build test ERROR on iommu/next]
[also build test ERROR on v4.8-rc6 next-20160913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Marek-Szyprowski/Exynos-IOMMU-proper-runtime-PM-support-use-device-dependencies/20160913-205434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/iommu/exynos-iommu.c: In function 'exynos_iommu_of_xlate':
>> drivers/iommu/exynos-iommu.c:1232:2: error: implicit declaration of function 'device_link_add' [-Werror=implicit-function-declaration]
     device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
     ^
>> drivers/iommu/exynos-iommu.c:1232:37: error: 'DEVICE_LINK_AVAILABLE' undeclared (first use in this function)
     device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
                                        ^
   drivers/iommu/exynos-iommu.c:1232:37: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iommu/exynos-iommu.c:1233:4: error: 'DEVICE_LINK_PERSISTENT' undeclared (first use in this function)
       DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
       ^
>> drivers/iommu/exynos-iommu.c:1233:29: error: 'DEVICE_LINK_PM_RUNTIME' undeclared (first use in this function)
       DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
                                ^
   cc1: some warnings being treated as errors

vim +/device_link_add +1232 drivers/iommu/exynos-iommu.c

  1226	
  1227		/*
  1228		 * SYSMMU will be runtime enabled via device link (dependency) to its
  1229		 * master device, so there are no direct calls to pm_runtime_get/put
  1230		 * in this driver.
  1231		 */
> 1232		device_link_add(dev, data->sysmmu, DEVICE_LINK_AVAILABLE,
> 1233				DEVICE_LINK_PERSISTENT | DEVICE_LINK_PM_RUNTIME);
  1234	
  1235		return 0;
  1236	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 58501 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Marek Szyprowski]

Hi Ulf,

On 2016-09-13 16:20, Ulf Hansson wrote:
> On 13 September 2016 at 14:49, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> This patch uses recently introduced device links to track the runtime pm
>> state of the master's device. This way each SYSMMU controller is runtime
>> activated when its master's device is active and can save/restore its state
>> instead of being enabled all the time. This way SYSMMU controllers no
>> longer prevents respective power domains to be turned off when master's
>> device is not used.
> Apologize for not reviewing earlier and if you find my
> questions/suggestions being silly. You may ignore them, if you don't
> think they deserves a proper answer. :-)

No problem. There are no silly questions, but there might be some silly
answers ;)

> I am not so familiar with the IOMMU subsystem, but I am wondering
> whether the issue you are solving, is similar to what can be observed
> for DMA and serial drivers. And of course also for other IOMMU
> drivers.
>
> In general the DMA/serial drivers requires to use the
> pm_runtime_irq_safe() option, to be able to easily deploy runtime PM
> support (of course there are some other workarounds as well).

There are some similarities between IOMMU and DMA engine devices (serial
drivers are imho completely different case). Both hw blocks do their work
on behalf of some other hardware block, which I will call master device.
DMA engine performs some DMA transaction on master's device request, while
IOMMU usually sits between system memory and master's device memory
interface, remapping addresses of each DMA transaction according to its
configuration and provided mapping tables (master device has some kind
of internal DMA controller and performs DMA transactions on their own).
IOMMU is usually used for a) mapping physically discontinuous memory into
contiguous DMA addresses and b) isolating devices, so they can access
only memory, which is dedicated or allocated for them.

DMA engine devices provide explicit API for their master's device drivers,
while IOMMU drivers are usually hidden behind DMA-mapping API (for most
use cases, although it would be possible for master's device driver to
call IOMMU API directly and some GPU/DRM drivers do that).

However from runtime pm perspective the DMA engine and IOMMU devices are
a bit different.

DMA engine drivers have well defined start and end of operation (queuing
dma request and irq from hw about having it finished). During that time
the device has to be runtime active all the time. The problem with using
current implementation of runtime pm is the fact that both start and end
of operation can be triggered from atomic context, what is not really
suitable for runtime pm. So the problem is mainly about API
incompatibility and lack of something like dma_engine_prepare()/unprepare()
(as an analogy to clocks api).

In case of IOMMU the main problem is determining weather IOMMU controller
has to be activated. There is no calls in IOMMU and DMA-mapping API, which
would bracket all DMA transactions performed by the master device. Someone
proposed to keep IOMMU runtime active when there exist at least one
mapping created by the IOMMU/DMA-mapping layers. This however does not
cover all the cases. In case of our IOMMU, when it is disabled or runtime
suspended, it enters "pass-thought" mode, so master device can still
perform DMA operations with identity mappings (so DMA address equals to
physical memory address). Till now Exynos IOMMU called pm_runtime_get()
on attaching to the iommu domain (what happens during initialization of
dma-mapping structures for given master device) and kept it active all
the time.

This patch series tries to address Exynos IOMMU runtime pm issue by forcing
IOMMU controller to follow runtime pm status of its master device. This way
we ensure that anytime when master's device is runtime activated, the iommu
will be also active and master device won't be able to bypass during its
DMA transactions mappings created by the IOMMU layer.

Quite long answer, but I hope I managed to give you a bit more background
on this topic.

> As we know, using the pm_runtime_irq_safe() option comes with some
> limitations, such as the runtime PM callbacks is not allowed to sleep.
> For a PM domain (genpd) that is attached to the device, this also
> means it must not be powered off.

Right, if possible I would like to avoid using pm_runtime_irq_safe()
option, because it is really impractical.

> To solve this problem, I was thinking we could convert to use the
> asynchronous pm_runtime_get() API, when trying to runtime resume the
> device from atomic contexts.

I'm not sure if this will work for DMA engine devices. If I understand
correctly some client's of DMA engine device might rely on the DMA
engine being configured and operational after queuing a request and
they might lock up if the DMA engine device activation if postponed
because of async runtime pm activation.

> Of course when it turns out that the device isn't yet runtime resumed
> immediately after calling pm_runtime_get(), the request needs to be
> put on a request queue to be managed shortly after instead. Doing it
> like this, would remove the need to use the pm_runtime_irq_safe()
> option.
>
> I realize that such change needs to be implemented in common code for
> IOMMU drivers, if at all possible.
>
> Anyway, I hope you at least get the idea and I just wanted to mention
> that I have been exploring this option for DMA and serial drivers.

I also have runtime pm for serial driver on my todo list, but it doesn't
have high priority. The other runtime pm integration subsystem that I
want to work on first is pinctrl. It is needed to fully support Exynos
5433 SoCs, because registers of some audio related pins are in the audio
power domain, what now prevent us from enabling support for audio power
domain.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Ulf Hansson]

[...]

>
>
> There are some similarities between IOMMU and DMA engine devices (serial
> drivers are imho completely different case). Both hw blocks do their work
> on behalf of some other hardware block, which I will call master device.
> DMA engine performs some DMA transaction on master's device request, while
> IOMMU usually sits between system memory and master's device memory
> interface, remapping addresses of each DMA transaction according to its
> configuration and provided mapping tables (master device has some kind
> of internal DMA controller and performs DMA transactions on their own).
> IOMMU is usually used for a) mapping physically discontinuous memory into
> contiguous DMA addresses and b) isolating devices, so they can access
> only memory, which is dedicated or allocated for them.
>
> DMA engine devices provide explicit API for their master's device drivers,
> while IOMMU drivers are usually hidden behind DMA-mapping API (for most
> use cases, although it would be possible for master's device driver to
> call IOMMU API directly and some GPU/DRM drivers do that).
>
> However from runtime pm perspective the DMA engine and IOMMU devices are
> a bit different.
>
> DMA engine drivers have well defined start and end of operation (queuing
> dma request and irq from hw about having it finished). During that time
> the device has to be runtime active all the time. The problem with using
> current implementation of runtime pm is the fact that both start and end
> of operation can be triggered from atomic context, what is not really
> suitable for runtime pm. So the problem is mainly about API
> incompatibility and lack of something like dma_engine_prepare()/unprepare()
> (as an analogy to clocks api).

That's also a viable option. Although, DMA clients would then need to
invoke such APIs from non-atomic contexts. Typically that would be
from client driver's runtime PM callbacks.

Me personally would rather avoid such solution, as it would sprinkle
lots of drivers to deal with this. Although, perhaps this is the only
option that actually works.

>
> In case of IOMMU the main problem is determining weather IOMMU controller
> has to be activated. There is no calls in IOMMU and DMA-mapping API, which
> would bracket all DMA transactions performed by the master device. Someone
> proposed to keep IOMMU runtime active when there exist at least one
> mapping created by the IOMMU/DMA-mapping layers. This however does not
> cover all the cases. In case of our IOMMU, when it is disabled or runtime
> suspended, it enters "pass-thought" mode, so master device can still
> perform DMA operations with identity mappings (so DMA address equals to
> physical memory address). Till now Exynos IOMMU called pm_runtime_get()
> on attaching to the iommu domain (what happens during initialization of
> dma-mapping structures for given master device) and kept it active all
> the time.
>
> This patch series tries to address Exynos IOMMU runtime pm issue by forcing
> IOMMU controller to follow runtime pm status of its master device. This way
> we ensure that anytime when master's device is runtime activated, the iommu
> will be also active and master device won't be able to bypass during its
> DMA transactions mappings created by the IOMMU layer.
>
> Quite long answer, but I hope I managed to give you a bit more background
> on this topic.

Yes, indeed. Thank you for taking the time to respond!

>
>> As we know, using the pm_runtime_irq_safe() option comes with some
>> limitations, such as the runtime PM callbacks is not allowed to sleep.
>> For a PM domain (genpd) that is attached to the device, this also
>> means it must not be powered off.
>
>
> Right, if possible I would like to avoid using pm_runtime_irq_safe()
> option, because it is really impractical.
>
>> To solve this problem, I was thinking we could convert to use the
>> asynchronous pm_runtime_get() API, when trying to runtime resume the
>> device from atomic contexts.
>
>
> I'm not sure if this will work for DMA engine devices. If I understand
> correctly some client's of DMA engine device might rely on the DMA
> engine being configured and operational after queuing a request and
> they might lock up if the DMA engine device activation if postponed
> because of async runtime pm activation.

I didn't know about this. If you have an example from the top of your
head, could you perhaps point me to it?

[...]

Kind regards
Uffe

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Marek Szyprowski]

Hi Ulf,


On 2016-09-14 12:28, Ulf Hansson wrote:
> [...]
>
>> There are some similarities between IOMMU and DMA engine devices (serial
>> drivers are imho completely different case). Both hw blocks do their work
>> on behalf of some other hardware block, which I will call master device.
>> DMA engine performs some DMA transaction on master's device request, while
>> IOMMU usually sits between system memory and master's device memory
>> interface, remapping addresses of each DMA transaction according to its
>> configuration and provided mapping tables (master device has some kind
>> of internal DMA controller and performs DMA transactions on their own).
>> IOMMU is usually used for a) mapping physically discontinuous memory into
>> contiguous DMA addresses and b) isolating devices, so they can access
>> only memory, which is dedicated or allocated for them.
>>
>> DMA engine devices provide explicit API for their master's device drivers,
>> while IOMMU drivers are usually hidden behind DMA-mapping API (for most
>> use cases, although it would be possible for master's device driver to
>> call IOMMU API directly and some GPU/DRM drivers do that).
>>
>> However from runtime pm perspective the DMA engine and IOMMU devices are
>> a bit different.
>>
>> DMA engine drivers have well defined start and end of operation (queuing
>> dma request and irq from hw about having it finished). During that time
>> the device has to be runtime active all the time. The problem with using
>> current implementation of runtime pm is the fact that both start and end
>> of operation can be triggered from atomic context, what is not really
>> suitable for runtime pm. So the problem is mainly about API
>> incompatibility and lack of something like dma_engine_prepare()/unprepare()
>> (as an analogy to clocks api).
> That's also a viable option. Although, DMA clients would then need to
> invoke such APIs from non-atomic contexts. Typically that would be
> from client driver's runtime PM callbacks.
>
> Me personally would rather avoid such solution, as it would sprinkle
> lots of drivers to deal with this. Although, perhaps this is the only
> option that actually works.

One might also introduce optional functions and notify DMA engine core with
some flag that the client driver wants to use them or not. If not core will
prepare dma engine on initialization. This is not really nice from the API
clearness perspective, but it would allow to have some time for transition
to the new approach till all clients gets updated.

>> In case of IOMMU the main problem is determining weather IOMMU controller
>> has to be activated. There is no calls in IOMMU and DMA-mapping API, which
>> would bracket all DMA transactions performed by the master device. Someone
>> proposed to keep IOMMU runtime active when there exist at least one
>> mapping created by the IOMMU/DMA-mapping layers. This however does not
>> cover all the cases. In case of our IOMMU, when it is disabled or runtime
>> suspended, it enters "pass-thought" mode, so master device can still
>> perform DMA operations with identity mappings (so DMA address equals to
>> physical memory address). Till now Exynos IOMMU called pm_runtime_get()
>> on attaching to the iommu domain (what happens during initialization of
>> dma-mapping structures for given master device) and kept it active all
>> the time.
>>
>> This patch series tries to address Exynos IOMMU runtime pm issue by forcing
>> IOMMU controller to follow runtime pm status of its master device. This way
>> we ensure that anytime when master's device is runtime activated, the iommu
>> will be also active and master device won't be able to bypass during its
>> DMA transactions mappings created by the IOMMU layer.
>>
>> Quite long answer, but I hope I managed to give you a bit more background
>> on this topic.
> Yes, indeed. Thank you for taking the time to respond!
>
>>> As we know, using the pm_runtime_irq_safe() option comes with some
>>> limitations, such as the runtime PM callbacks is not allowed to sleep.
>>> For a PM domain (genpd) that is attached to the device, this also
>>> means it must not be powered off.
>>
>> Right, if possible I would like to avoid using pm_runtime_irq_safe()
>> option, because it is really impractical.
>>
>>> To solve this problem, I was thinking we could convert to use the
>>> asynchronous pm_runtime_get() API, when trying to runtime resume the
>>> device from atomic contexts.
>> I'm not sure if this will work for DMA engine devices. If I understand
>> correctly some client's of DMA engine device might rely on the DMA
>> engine being configured and operational after queuing a request and
>> they might lock up if the DMA engine device activation if postponed
>> because of async runtime pm activation.
> I didn't know about this. If you have an example from the top of your
> head, could you perhaps point me to it?

No, I don't have any example. This is just my fear that there might be some
hardware which works this way. I can imagine a driver, which queue dma 
engine
request and then triggers 'start' command to its hw block. HW blocks usually
uses some additional signal lines for DMA, so the HW block might check 
if all
needed signals from DMA engine HW are ready. If not it will fail to start
avoid lockup of starting without DMA engine HW being ready.

However I don't have much experience with DMA engine and drivers. I only
helped in adding DMA engine support to Samsung UART driver and in the
hardware manual there is information about additional lines between DMA
controller and UART module, which are used in the DMA mode.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Marek Szyprowski]

Hi Ulf,


On 2016-09-14 12:28, Ulf Hansson wrote:
> [...]
>
>> There are some similarities between IOMMU and DMA engine devices (serial
>> drivers are imho completely different case). Both hw blocks do their work
>> on behalf of some other hardware block, which I will call master device.
>> DMA engine performs some DMA transaction on master's device request, while
>> IOMMU usually sits between system memory and master's device memory
>> interface, remapping addresses of each DMA transaction according to its
>> configuration and provided mapping tables (master device has some kind
>> of internal DMA controller and performs DMA transactions on their own).
>> IOMMU is usually used for a) mapping physically discontinuous memory into
>> contiguous DMA addresses and b) isolating devices, so they can access
>> only memory, which is dedicated or allocated for them.
>>
>> DMA engine devices provide explicit API for their master's device drivers,
>> while IOMMU drivers are usually hidden behind DMA-mapping API (for most
>> use cases, although it would be possible for master's device driver to
>> call IOMMU API directly and some GPU/DRM drivers do that).
>>
>> However from runtime pm perspective the DMA engine and IOMMU devices are
>> a bit different.
>>
>> DMA engine drivers have well defined start and end of operation (queuing
>> dma request and irq from hw about having it finished). During that time
>> the device has to be runtime active all the time. The problem with using
>> current implementation of runtime pm is the fact that both start and end
>> of operation can be triggered from atomic context, what is not really
>> suitable for runtime pm. So the problem is mainly about API
>> incompatibility and lack of something like dma_engine_prepare()/unprepare()
>> (as an analogy to clocks api).
> That's also a viable option. Although, DMA clients would then need to
> invoke such APIs from non-atomic contexts. Typically that would be
> from client driver's runtime PM callbacks.
>
> Me personally would rather avoid such solution, as it would sprinkle
> lots of drivers to deal with this. Although, perhaps this is the only
> option that actually works.

One might also introduce optional functions and notify DMA engine core with
some flag that the client driver wants to use them or not. If not core will
prepare dma engine on initialization. This is not really nice from the API
clearness perspective, but it would allow to have some time for transition
to the new approach till all clients gets updated.

>> In case of IOMMU the main problem is determining weather IOMMU controller
>> has to be activated. There is no calls in IOMMU and DMA-mapping API, which
>> would bracket all DMA transactions performed by the master device. Someone
>> proposed to keep IOMMU runtime active when there exist at least one
>> mapping created by the IOMMU/DMA-mapping layers. This however does not
>> cover all the cases. In case of our IOMMU, when it is disabled or runtime
>> suspended, it enters "pass-thought" mode, so master device can still
>> perform DMA operations with identity mappings (so DMA address equals to
>> physical memory address). Till now Exynos IOMMU called pm_runtime_get()
>> on attaching to the iommu domain (what happens during initialization of
>> dma-mapping structures for given master device) and kept it active all
>> the time.
>>
>> This patch series tries to address Exynos IOMMU runtime pm issue by forcing
>> IOMMU controller to follow runtime pm status of its master device. This way
>> we ensure that anytime when master's device is runtime activated, the iommu
>> will be also active and master device won't be able to bypass during its
>> DMA transactions mappings created by the IOMMU layer.
>>
>> Quite long answer, but I hope I managed to give you a bit more background
>> on this topic.
> Yes, indeed. Thank you for taking the time to respond!
>
>>> As we know, using the pm_runtime_irq_safe() option comes with some
>>> limitations, such as the runtime PM callbacks is not allowed to sleep.
>>> For a PM domain (genpd) that is attached to the device, this also
>>> means it must not be powered off.
>>
>> Right, if possible I would like to avoid using pm_runtime_irq_safe()
>> option, because it is really impractical.
>>
>>> To solve this problem, I was thinking we could convert to use the
>>> asynchronous pm_runtime_get() API, when trying to runtime resume the
>>> device from atomic contexts.
>> I'm not sure if this will work for DMA engine devices. If I understand
>> correctly some client's of DMA engine device might rely on the DMA
>> engine being configured and operational after queuing a request and
>> they might lock up if the DMA engine device activation if postponed
>> because of async runtime pm activation.
> I didn't know about this. If you have an example from the top of your
> head, could you perhaps point me to it?

No, I don't have any example. This is just my fear that there might be some
hardware which works this way. I can imagine a driver, which queue dma 
engine
request and then triggers 'start' command to its hw block. HW blocks usually
uses some additional signal lines for DMA, so the HW block might check 
if all
needed signals from DMA engine HW are ready. If not it will fail to start
avoid lockup of starting without DMA engine HW being ready.

However I don't have much experience with DMA engine and drivers. I only
helped in adding DMA engine support to Samsung UART driver and in the
hardware manual there is information about additional lines between DMA
controller and UART module, which are used in the DMA mode.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 2/2] iommu/exynos: Add proper runtime pm support

[Ulf Hansson]

[...]

>>>
>>>> To solve this problem, I was thinking we could convert to use the
>>>> asynchronous pm_runtime_get() API, when trying to runtime resume the
>>>> device from atomic contexts.
>>>
>>> I'm not sure if this will work for DMA engine devices. If I understand
>>> correctly some client's of DMA engine device might rely on the DMA
>>> engine being configured and operational after queuing a request and
>>> they might lock up if the DMA engine device activation if postponed
>>> because of async runtime pm activation.
>>
>> I didn't know about this. If you have an example from the top of your
>> head, could you perhaps point me to it?
>
>
> No, I don't have any example. This is just my fear that there might be some
> hardware which works this way. I can imagine a driver, which queue dma
> engine
> request and then triggers 'start' command to its hw block. HW blocks usually
> uses some additional signal lines for DMA, so the HW block might check if
> all
> needed signals from DMA engine HW are ready. If not it will fail to start
> avoid lockup of starting without DMA engine HW being ready.

Well, I think this reasoning makes lots of sense! Clearly we wouldn't
be able to guarantee that there's isn't a glitch in an undefined HW
behaviour.

I will drop my suggested approach and try out another one.

>
> However I don't have much experience with DMA engine and drivers. I only
> helped in adding DMA engine support to Samsung UART driver and in the
> hardware manual there is information about additional lines between DMA
> controller and UART module, which are used in the DMA mode.

Thanks for sharing your experience!

Now, I should allow you to get back to the original review! :-)

Kind regards
Uffe

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Tobias Jakobi]

Hello Marek,

I did some tests with the new version today. Sadly the reboot/shutdown
issues are still present.

Here's what appears on the UART last:
> [  399.538147] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [  404.970649] smsc95xx 1-2.1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
> [  404.972426] smsc95xx 1-2.1.1:1.0 eth0: Error reading MII_ACCESS
> [  404.978336] smsc95xx 1-2.1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
> [  404.985020] smsc95xx 1-2.1.1:1.0 eth0: Failed to read MII_BMSR
> [  434.170626] usb 1-2-port2: cannot reset (err = -110)
> [  435.210242] usb 1-2-port2: cannot reset (err = -110)
> [  436.250261] usb 1-2-port2: cannot reset (err = -110)
> [  437.290241] usb 1-2-port2: cannot reset (err = -110)
> [  438.330241] usb 1-2-port2: cannot reset (err = -110)
> [  438.330346] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  439.370608] usb 1-2-port2: cannot disable (err = -110)
> [  440.410225] usb 1-2-port2: cannot reset (err = -110)
> [  441.450236] usb 1-2-port2: cannot reset (err = -110)
> [  442.490222] usb 1-2-port2: cannot reset (err = -110)
> [  443.530224] usb 1-2-port2: cannot reset (err = -110)
> [  444.570228] usb 1-2-port2: cannot reset (err = -110)
> [  444.570331] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  445.610227] usb 1-2-port2: cannot disable (err = -110)
> [  446.650230] usb 1-2-port2: cannot reset (err = -110)
> [  447.690231] usb 1-2-port2: cannot reset (err = -110)
> [  448.730235] usb 1-2-port2: cannot reset (err = -110)
> [  449.770235] usb 1-2-port2: cannot reset (err = -110)
> [  450.810239] usb 1-2-port2: cannot reset (err = -110)
> [  450.810342] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  451.850240] usb 1-2-port2: cannot disable (err = -110)
> [  452.890243] usb 1-2-port2: cannot reset (err = -110)
> [  453.930271] usb 1-2-port2: cannot reset (err = -110)
> [  454.970247] usb 1-2-port2: cannot reset (err = -110)
> [  456.010250] usb 1-2-port2: cannot reset (err = -110)
> [  457.050251] usb 1-2-port2: cannot reset (err = -110)
> [  457.050353] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
> [  458.090250] usb 1-2-port2: cannot disable (err = -110)
> [  459.130603] usb 1-2-port2: cannot disable (err = -110)
> [  464.330286] hub 1-2:1.0: hub_ext_port_status failed (err = -110)


If I trigger a stack trace via SysRq at this point, I get this here:
> [  672.679321] sysrq: SysRq : Show backtrace of all active CPUs
> [  672.679448] Sending NMI to all CPUs:
> [  672.682901] NMI backtrace for cpu 0
> [  672.686377] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.8.0-rc7-debug+ #6
> [  672.693149] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  672.699223] Backtrace: 
> [  672.701657] [<c010c9bc>] (dump_backtrace) from [<c010cb68>] (show_stack+0x18/0x1c)
> [  672.709212]  r6:c0c25e40 r5:600401d3 r4:00000000 r3:00040800
> [  672.714845] [<c010cb50>] (show_stack) from [<c039bef8>] (dump_stack+0xb0/0xdc)
> [  672.722062] [<c039be48>] (dump_stack) from [<c03a089c>] (nmi_cpu_backtrace+0x88/0x8c)
> [  672.729870]  r8:c0c27f88 r7:c010e87c r6:00000000 r5:00000000 r4:00000000 r3:00000000
> [  672.737590] [<c03a0814>] (nmi_cpu_backtrace) from [<c010e8d8>] (raise_nmi+0x5c/0x60)
> [  672.745320]  r5:c0c0281c r4:c0c0281c
> [  672.748868] [<c010e87c>] (raise_nmi) from [<c03a0800>] (nmi_trigger_all_cpu_backtrace+0x120/0x134)
> [  672.757821]  r4:00000001 r3:00000000
> [  672.761367] [<c03a06e0>] (nmi_trigger_all_cpu_backtrace) from [<c010f5c0>] (arch_trigger_all_cpu_backtrace+0x18/0x1c)
> [  672.771972]  r7:00000001 r6:00000008 r5:0000006c r4:c0c0cf0c
> [  672.777601] [<c010f5a8>] (arch_trigger_all_cpu_backtrace) from [<c03fc7c0>] (sysrq_handle_showallcpus+0x14/0x18)
> [  672.787774] [<c03fc7ac>] (sysrq_handle_showallcpus) from [<c03fcfc8>] (__handle_sysrq+0x104/0x268)
> [  672.796711] [<c03fcec4>] (__handle_sysrq) from [<c03fd5d4>] (handle_sysrq+0x34/0x38)
> [  672.804431]  r8:c0c02100 r7:c0c2b994 r6:00000040 r5:00000000 r4:c0c2b964
> [  672.811106] [<c03fd5a0>] (handle_sysrq) from [<c0412564>] (s3c24xx_serial_rx_drain_fifo+0x1fc/0x22c)
> [  672.820236] [<c0412368>] (s3c24xx_serial_rx_drain_fifo) from [<c04133f8>] (s3c24xx_serial_rx_chars+0x74/0x1a8)
> [  672.830214]  r10:0001000f r9:ee1f2200 r8:200401d3 r7:ed01cc00 r6:c0c2b994 r5:ee2a5550
> [  672.838013]  r4:c0c2b964
> [  672.840529] [<c0413384>] (s3c24xx_serial_rx_chars) from [<c041357c>] (s3c64xx_serial_handle_irq+0x50/0x68)
> [  672.850178]  r10:c0c01e24 r9:00000000 r8:00000047 r7:c0c00000 r6:00000047 r5:00000001
> [  672.857977]  r4:c0c2b964
> [  672.860496] [<c041352c>] (s3c64xx_serial_handle_irq) from [<c01843b8>] (__handle_irq_event_percpu+0x7c/0x4c4)
> [  672.870403]  r6:c0c02848 r5:eeb0a100 r4:ed76b8c0 r3:c041352c
> [  672.876030] [<c018433c>] (__handle_irq_event_percpu) from [<c0184824>] (handle_irq_event_percpu+0x24/0x60)
> [  672.885679]  r10:c0c01ee8 r9:ee808000 r8:00000001 r7:00000000 r6:c0c02848 r5:eeb0a100
> [  672.893478]  r4:eeb0a100
> [  672.895994] [<c0184800>] (handle_irq_event_percpu) from [<c01848a0>] (handle_irq_event+0x40/0x64)
> [  672.904860]  r5:eeb0a160 r4:eeb0a100
> [  672.908406] [<c0184860>] (handle_irq_event) from [<c01881d0>] (handle_fasteoi_irq+0xd8/0x1a8)
> [  672.916925]  r6:c0c02848 r5:eeb0a160 r4:eeb0a100 r3:00000000
> [  672.922554] [<c01880f8>] (handle_fasteoi_irq) from [<c0183ae8>] (generic_handle_irq+0x2c/0x3c)
> [  672.931160]  r7:00000000 r6:00000047 r5:00000000 r4:c0b42270
> [  672.936790] [<c0183abc>] (generic_handle_irq) from [<c0183b7c>] (__handle_domain_irq+0x84/0xf4)
> [  672.945486] [<c0183af8>] (__handle_domain_irq) from [<c0101510>] (gic_handle_irq+0x5c/0xa0)
> [  672.953815]  r10:c0c02524 r9:f0821000 r8:f0820000 r7:c0c01ee8 r6:f082000c r5:c0c26380
> [  672.961616]  r4:c0c02848 r3:c0c01ee8
> [  672.965172] [<c01014b4>] (gic_handle_irq) from [<c010d7b0>] (__irq_svc+0x70/0xb0)
> [  672.972651] Exception stack(0xc0c01ee8 to 0xc0c01f30)
> [  672.977679] 1ee0:                   00000001 00000001 00000000 c011b660 c0c00000 c0c448cc
> [  672.985846] 1f00: c0c024d4 00000001 00000000 00000000 c0c02524 c0c01f44 c0c01f08 c0c01f38
> [  672.994005] 1f20: c01743d4 c01089d4 20040053 ffffffff
> [  672.999029]  r9:c0c00000 r8:00000000 r7:c0c01f1c r6:ffffffff r5:20040053 r4:c01089d4
> [  673.006758] [<c01089a8>] (arch_cpu_idle) from [<c016cdd8>] (default_idle_call+0x28/0x38)
> [  673.014838] [<c016cdb0>] (default_idle_call) from [<c016d190>] (cpu_startup_entry+0x3a8/0x484)
> [  673.023436] [<c016cde8>] (cpu_startup_entry) from [<c0807b00>] (rest_init+0x168/0x178)
> [  673.031327]  r7:ffffffff
> [  673.033834] [<c0807998>] (rest_init) from [<c0b00cf8>] (start_kernel+0x384/0x390)
> [  673.041308]  r5:c0c49000 r4:00000001
> [  673.044855] [<c0b00974>] (start_kernel) from [<40008078>] (0x40008078)
> [  673.051486] NMI backtrace for cpu 3
> [  673.054844] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.8.0-rc7-debug+ #6
> [  673.061615] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  673.067692] task: ee8b3200 task.stack: ee8be000
> [  673.072201] PC is at arch_cpu_idle+0x2c/0x44
> [  673.076455] LR is at trace_hardirqs_on_caller+0x158/0x200
> [  673.081840] pc : [<c01089d4>]    lr : [<c01743d4>]    psr: 20010053
> [  673.088089] sp : ee8bff90  ip : ee8bff60  fp : ee8bff9c
> [  673.093294] r10: c0c02524  r9 : 00000000  r8 : 00000000
> [  673.098503] r7 : 00000008  r6 : c0c024d4  r5 : c0c448cc  r4 : ee8be000
> [  673.105014] r3 : c011b660  r2 : 00000000  r1 : 00000001  r0 : 00000001
> [  673.111525] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> [  673.118736] Control: 10c5387d  Table: 6dc5804a  DAC: 00000051
> [  673.124459] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.8.0-rc7-debug+ #6
> [  673.131229] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  673.137306] Backtrace: 
> [  673.139730] [<c010c9bc>] (dump_backtrace) from [<c010cb68>] (show_stack+0x18/0x1c)
> [  673.147292]  r6:c0c25e40 r5:600101d3 r4:00000000 r3:00040800
> [  673.152929] [<c010cb50>] (show_stack) from [<c039bef8>] (dump_stack+0xb0/0xdc)
> [  673.160138] [<c039be48>] (dump_stack) from [<c0108c5c>] (show_regs+0x14/0x18)
> [  673.167251]  r8:f082c000 r7:ee8bff40 r6:c0b42270 r5:00000003 r4:ee8bff40 r3:00040800
> [  673.174982] [<c0108c48>] (show_regs) from [<c03a0880>] (nmi_cpu_backtrace+0x6c/0x8c)
> [  673.182707] [<c03a0814>] (nmi_cpu_backtrace) from [<c010f1c0>] (handle_IPI+0x14c/0x434)
> [  673.190692]  r5:c0b42270 r4:00000007
> [  673.194241] [<c010f074>] (handle_IPI) from [<c0101550>] (gic_handle_irq+0x9c/0xa0)
> [  673.201803]  r10:c0c02524 r9:f082d000 r8:f082c000 r7:ee8bff40 r6:f082c00c r5:c0c26380
> [  673.209615]  r4:c0c02848
> [  673.212121] [<c01014b4>] (gic_handle_irq) from [<c010d7b0>] (__irq_svc+0x70/0xb0)
> [  673.219596] Exception stack(0xee8bff40 to 0xee8bff88)
> [  673.224624] ff40: 00000001 00000001 00000000 c011b660 ee8be000 c0c448cc c0c024d4 00000008
> [  673.232790] ff60: 00000000 00000000 c0c02524 ee8bff9c ee8bff60 ee8bff90 c01743d4 c01089d4
> [  673.240952] ff80: 20010053 ffffffff
> [  673.244411]  r9:ee8be000 r8:00000000 r7:ee8bff74 r6:ffffffff r5:20010053 r4:c01089d4
> [  673.252147] [<c01089a8>] (arch_cpu_idle) from [<c016cdd8>] (default_idle_call+0x28/0x38)
> [  673.260221] [<c016cdb0>] (default_idle_call) from [<c016d190>] (cpu_startup_entry+0x3a8/0x484)
> [  673.268816] [<c016cde8>] (cpu_startup_entry) from [<c010ee88>] (secondary_start_kernel+0xf8/0x100)
> [  673.277755]  r7:c0c49328
> [  673.280259] [<c010ed90>] (secondary_start_kernel) from [<401015ec>] (0x401015ec)
> [  673.287648]  r5:00000051 r4:6e89406a
> [  673.291197] NMI backtrace for cpu 2
> [  673.294670] CPU: 2 PID: 3729 Comm: reboot Not tainted 4.8.0-rc7-debug+ #6
> [  673.301443] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  673.307518] task: ee030c80 task.stack: edec8000
> [  673.312029] PC is at _raw_spin_unlock_irq+0x30/0x64
> [  673.316891] LR is at mark_held_locks+0x74/0x9c
> [  673.321317] pc : [<c080ee84>]    lr : [<c0174254>]    psr: 200a0053
> [  673.327569] sp : edec9df0  ip : c0d9a4d4  fp : edec9e04
> [  673.332775] r10: 00000000  r9 : ee029c44  r8 : c0c49020
> [  673.337984] r7 : ee029cb0  r6 : 00000000  r5 : ee029d84  r4 : ee029cb0
> [  673.344495] r3 : 600a00d3  r2 : 00000000  r1 : ee031158  r0 : 00000001
> [  673.351005] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> [  673.358215] Control: 10c5387d  Table: 6d7ec04a  DAC: 00000051
> [  673.363937] CPU: 2 PID: 3729 Comm: reboot Not tainted 4.8.0-rc7-debug+ #6
> [  673.370709] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  673.376784] Backtrace: 
> [  673.379209] [<c010c9bc>] (dump_backtrace) from [<c010cb68>] (show_stack+0x18/0x1c)
> [  673.386773]  r6:c0c25e40 r5:600a01d3 r4:00000000 r3:00040800
> [  673.392408] [<c010cb50>] (show_stack) from [<c039bef8>] (dump_stack+0xb0/0xdc)
> [  673.399618] [<c039be48>] (dump_stack) from [<c0108c5c>] (show_regs+0x14/0x18)
> [  673.406732]  r8:f0828000 r7:edec9da0 r6:c0b42270 r5:00000002 r4:edec9da0 r3:00040800
> [  673.414462] [<c0108c48>] (show_regs) from [<c03a0880>] (nmi_cpu_backtrace+0x6c/0x8c)
> [  673.422187] [<c03a0814>] (nmi_cpu_backtrace) from [<c010f1c0>] (handle_IPI+0x14c/0x434)
> [  673.430173]  r5:c0b42270 r4:00000007
> [  673.433722] [<c010f074>] (handle_IPI) from [<c0101550>] (gic_handle_irq+0x9c/0xa0)
> [  673.441283]  r10:00000000 r9:f0829000 r8:f0828000 r7:edec9da0 r6:f082800c r5:c0c26380
> [  673.449095]  r4:c0c02848
> [  673.451601] [<c01014b4>] (gic_handle_irq) from [<c010d7b0>] (__irq_svc+0x70/0xb0)
> [  673.459076] Exception stack(0xedec9da0 to 0xedec9de8)
> [  673.464103] 9da0: 00000001 ee031158 00000000 600a00d3 ee029cb0 ee029d84 00000000 ee029cb0
> [  673.472272] 9dc0: c0c49020 ee029c44 00000000 edec9e04 c0d9a4d4 edec9df0 c0174254 c080ee84
> [  673.480430] 9de0: 200a0053 ffffffff
> [  673.483892]  r9:edec8000 r8:c0c49020 r7:edec9dd4 r6:ffffffff r5:200a0053 r4:c080ee84
> [  673.491627] [<c080ee54>] (_raw_spin_unlock_irq) from [<c047ab90>] (pm_runtime_barrier+0x68/0xbc)
> [  673.500395]  r4:ee029c10 r3:00000080
> [  673.503942] [<c047ab28>] (pm_runtime_barrier) from [<c046c6d4>] (device_shutdown+0x110/0x1c8)
> [  673.512460]  r7:c140755c r6:ee029c10 r5:c0c2e210 r4:ee029c1c
> [  673.518094] [<c046c5c4>] (device_shutdown) from [<c014ad10>] (kernel_restart_prepare+0x3c/0x40)
> [  673.526782]  r9:c0c0ae1c r8:00000010 r7:c0c0b244 r6:01234567 r5:00000000 r4:00000000
> [  673.534506] [<c014acd4>] (kernel_restart_prepare) from [<c014ae68>] (kernel_restart+0x14/0x58)
> [  673.543100] [<c014ae54>] (kernel_restart) from [<c014b13c>] (SyS_reboot+0x114/0x200)
> [  673.550824]  r4:c0c02448 r3:01234567
> [  673.554374] [<c014b028>] (SyS_reboot) from [<c0107ee0>] (ret_fast_syscall+0x0/0x1c)
> [  673.562021]  r9:edec8000 r8:c0108084 r7:00000058 r6:010a0008 r5:be9f6ce0 r4:00000001
> [  673.569748] NMI backtrace for cpu 1
> [  673.573211] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0-rc7-debug+ #6
> [  673.579984] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  673.586059] task: ee8b1900 task.stack: ee8ba000
> [  673.590570] PC is at arch_cpu_idle+0x2c/0x44
> [  673.594823] LR is at trace_hardirqs_on_caller+0x158/0x200
> [  673.600206] pc : [<c01089d4>]    lr : [<c01743d4>]    psr: 200b0053
> [  673.606457] sp : ee8bbf90  ip : ee8bbf60  fp : ee8bbf9c
> [  673.611664] r10: c0c02524  r9 : 00000000  r8 : 00000000
> [  673.616872] r7 : 00000002  r6 : c0c024d4  r5 : c0c448cc  r4 : ee8ba000
> [  673.623383] r3 : c011b660  r2 : 00000000  r1 : 00000001  r0 : 00000001
> [  673.629894] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment none
> [  673.637103] Control: 10c5387d  Table: 6df9804a  DAC: 00000051
> [  673.642827] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0-rc7-debug+ #6
> [  673.649598] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [  673.655672] Backtrace: 
> [  673.658098] [<c010c9bc>] (dump_backtrace) from [<c010cb68>] (show_stack+0x18/0x1c)
> [  673.665660]  r6:c0c25e40 r5:600b01d3 r4:00000000 r3:00040800
> [  673.671296] [<c010cb50>] (show_stack) from [<c039bef8>] (dump_stack+0xb0/0xdc)
> [  673.678506] [<c039be48>] (dump_stack) from [<c0108c5c>] (show_regs+0x14/0x18)
> [  673.685620]  r8:f0824000 r7:ee8bbf40 r6:c0b42270 r5:00000001 r4:ee8bbf40 r3:00040800
> [  673.693350] [<c0108c48>] (show_regs) from [<c03a0880>] (nmi_cpu_backtrace+0x6c/0x8c)
> [  673.701075] [<c03a0814>] (nmi_cpu_backtrace) from [<c010f1c0>] (handle_IPI+0x14c/0x434)
> [  673.709061]  r5:c0b42270 r4:00000007
> [  673.712610] [<c010f074>] (handle_IPI) from [<c0101550>] (gic_handle_irq+0x9c/0xa0)
> [  673.720171]  r10:c0c02524 r9:f0825000 r8:f0824000 r7:ee8bbf40 r6:f082400c r5:c0c26380
> [  673.727983]  r4:c0c02848
> [  673.730489] [<c01014b4>] (gic_handle_irq) from [<c010d7b0>] (__irq_svc+0x70/0xb0)
> [  673.737964] Exception stack(0xee8bbf40 to 0xee8bbf88)
> [  673.742992] bf40: 00000001 00000001 00000000 c011b660 ee8ba000 c0c448cc c0c024d4 00000002
> [  673.751160] bf60: 00000000 00000000 c0c02524 ee8bbf9c ee8bbf60 ee8bbf90 c01743d4 c01089d4
> [  673.759319] bf80: 200b0053 ffffffff
> [  673.762780]  r9:ee8ba000 r8:00000000 r7:ee8bbf74 r6:ffffffff r5:200b0053 r4:c01089d4
> [  673.770515] [<c01089a8>] (arch_cpu_idle) from [<c016cdd8>] (default_idle_call+0x28/0x38)
> [  673.778588] [<c016cdb0>] (default_idle_call) from [<c016d190>] (cpu_startup_entry+0x3a8/0x484)
> [  673.787182] [<c016cde8>] (cpu_startup_entry) from [<c010ee88>] (secondary_start_kernel+0xf8/0x100)
> [  673.796123]  r7:c0c49328
> [  673.798627] [<c010ed90>] (secondary_start_kernel) from [<401015ec>] (0x401015ec)
> [  673.806016]  r5:00000051 r4:6e89406a

So this seems to be PM related, since it hangs in pm_runtime_barrier()
in device_shutdown().

Anyway, I noticed something else on boot:
> [    7.567702] usb-storage 1-2.2:1.0: USB Mass Storage device detected
> [    7.569762] scsi host0: usb-storage 1-2.2:1.0
> [    7.579054] scsi host0: runtime PM trying to activate child device host0 but parent (1-2.2:1.0) is not active


Maybe some details about my storage setup. While boot and rootfs are on
a SD card, /var and some other things are on a USB thumb drive (/dev/sda
is that drive).

My guess is that with the runpm changes the failure to sync the thumb
drive now blocks the rest of the shutdown/reboot procedure.


With best wishes,
Tobias



Marek Szyprowski wrote:
> Hello,
> 
> This patch series finally implements proper runtime PM support in Exynos
> IOMMU driver. This has been achieved by using recently introduce device
> links, which lets SYSMMU controller's runtime PM to follow master's device
> runtime PM state (the device which actually performs DMA transaction).
> The main idea behind this solution is an observation that any DMA activity
> from master device can be done only when master device is active, thus when
> master device is suspended SYSMMU controller device can also be suspended.
> 
> This patchset solves the situation that power domains are always enabled,
> because all SYSMMU controllers (which belongs to those domains) are
> permanently active (because existing driver was simplified and kept
> SYSMMU device active all the time after initialization).
> 
> Patch requires second version of Rafeal's "Functional dependencies
> between devices" patchset, which is available here:
> https://lkml.org/lkml/2016/9/8/798
> 
> If one wants to test this patchset, I've provided a branch with all needed
> patches (some fixes for Exynos4 FIMC-IS driver are needed):
> https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v3
> 
> Patches are based on vanilla v4.8-rc6 kernel with Rafael's patches applied.
> 
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
> 
> 
> Changelog:
> v3:
> - rebased on top of latest device dependencies/links patchset
> - added proper locking between runtime pm, iommu_attach/detach and sysmmu
>   enable/disable(added per iommu owner device's rpm lock)
> 
> v2:
> - replaced PM notifiers with generic device dependencies/links developped
>   by Rafael J. Wysocki
> 
> v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
> - initial version
> 
> 
> Patch summary:
> 
> Marek Szyprowski (2):
>   iommu/exynos: Remove excessive, useless debug
>   iommu/exynos: Add proper runtime pm support
> 
>  drivers/iommu/exynos-iommu.c | 228 ++++++++++++++++++-------------------------
>  1 file changed, 94 insertions(+), 134 deletions(-)
> 

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hi All,

On 2016-09-19 23:45, Tobias Jakobi wrote:
> I did some tests with the new version today. Sadly the reboot/shutdown
> issues are still present.

Thanks for the report. I've managed to reproduce this issue and it is again
caused by modifying device on devices_kset list before it will be finally
added by device_add(). I thought that the new patchset allows creating links
to a device, which has not been yet added to system device list.

Rafael:
What is your opinion? Should it be allowed to create a link to device, which
has not yet been added to system device list by device_add()? My code 
used to
do that, because IOMMUs are configured from 
of_platform_device_create_pdata()
of_dma_configure() of_iommu_configure(), which happens before device_add().

To solve the reported corruption of devices_kset list, following change is
needed:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index aa8196508db9..4542ba9f60d4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1039,11 +1039,18 @@ static void devices_kset_move_after(struct 
device *deva, struct device *devb)
   */
  void devices_kset_move_last(struct device *dev)
  {
+       struct device *d;
+
         if (!devices_kset)
                 return;
         pr_debug("devices_kset: Moving %s to end of list\n", 
dev_name(dev));
         spin_lock(&devices_kset->list_lock);
-       list_move_tail(&dev->kobj.entry, &devices_kset->list);
+       list_for_each_entry(d, &devices_kset->list, kobj.entry) {
+               if (d == dev) {
+                       list_move_tail(&dev->kobj.entry, 
&devices_kset->list);
+                       break;
+               }
+       }
         spin_unlock(&devices_kset->list_lock);
  }


If you think that links can be created only to a device, which has been 
fully
added to the system, I will register a bus notifier and create a link on 
consumers
device probe then.

[...]

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hi All,

On 2016-09-19 23:45, Tobias Jakobi wrote:
> I did some tests with the new version today. Sadly the reboot/shutdown
> issues are still present.

Thanks for the report. I've managed to reproduce this issue and it is again
caused by modifying device on devices_kset list before it will be finally
added by device_add(). I thought that the new patchset allows creating links
to a device, which has not been yet added to system device list.

Rafael:
What is your opinion? Should it be allowed to create a link to device, which
has not yet been added to system device list by device_add()? My code 
used to
do that, because IOMMUs are configured from 
of_platform_device_create_pdata()
of_dma_configure() of_iommu_configure(), which happens before device_add().

To solve the reported corruption of devices_kset list, following change is
needed:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index aa8196508db9..4542ba9f60d4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1039,11 +1039,18 @@ static void devices_kset_move_after(struct 
device *deva, struct device *devb)
   */
  void devices_kset_move_last(struct device *dev)
  {
+       struct device *d;
+
         if (!devices_kset)
                 return;
         pr_debug("devices_kset: Moving %s to end of list\n", 
dev_name(dev));
         spin_lock(&devices_kset->list_lock);
-       list_move_tail(&dev->kobj.entry, &devices_kset->list);
+       list_for_each_entry(d, &devices_kset->list, kobj.entry) {
+               if (d == dev) {
+                       list_move_tail(&dev->kobj.entry, 
&devices_kset->list);
+                       break;
+               }
+       }
         spin_unlock(&devices_kset->list_lock);
  }


If you think that links can be created only to a device, which has been 
fully
added to the system, I will register a bus notifier and create a link on 
consumers
device probe then.

[...]

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Rafael J. Wysocki]

On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> Hi All,
> 
> On 2016-09-19 23:45, Tobias Jakobi wrote:
> > I did some tests with the new version today. Sadly the reboot/shutdown
> > issues are still present.
> 
> Thanks for the report. I've managed to reproduce this issue and it is again
> caused by modifying device on devices_kset list before it will be finally
> added by device_add(). I thought that the new patchset allows creating links
> to a device, which has not been yet added to system device list.
> 
> Rafael:
> What is your opinion?

Well, this is a problem. :-)

> Should it be allowed to create a link to device, which
> has not yet been added to system device list by device_add()?

While it would be easy to require both the consumer and producer devices to
be registered for creating a link between them, that would just make it harder
to use links in the first place.

So ideally, it should be possible to create links between devices before
registering them, but since I didn't take that into account in the current
patch series, some quite substantial changes are needed to cover that.

Additional link states come to mind, but then the "stateless" links are
affected by this problem too.

> My code used to do that, because IOMMUs are configured from 
> of_platform_device_create_pdata()
> of_dma_configure() of_iommu_configure(), which happens before device_add().
> 
> To solve the reported corruption of devices_kset list, following change is
> needed:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index aa8196508db9..4542ba9f60d4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1039,11 +1039,18 @@ static void devices_kset_move_after(struct 
> device *deva, struct device *devb)
>    */
>   void devices_kset_move_last(struct device *dev)
>   {
> +       struct device *d;
> +
>          if (!devices_kset)
>                  return;
>          pr_debug("devices_kset: Moving %s to end of list\n", 
> dev_name(dev));
>          spin_lock(&devices_kset->list_lock);
> -       list_move_tail(&dev->kobj.entry, &devices_kset->list);
> +       list_for_each_entry(d, &devices_kset->list, kobj.entry) {
> +               if (d == dev) {
> +                       list_move_tail(&dev->kobj.entry, 
> &devices_kset->list);
> +                       break;
> +               }
> +       }
>          spin_unlock(&devices_kset->list_lock);
>   }
> 
> 
> If you think that links can be created only to a device, which has been 
> fully
> added to the system, I will register a bus notifier and create a link on 
> consumers
> device probe then.

That would be a workaround for a coverage gap, so not particularly attractive.

Thanks,
Rafael

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Rafael J. Wysocki]

On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> Hi All,
> 
> On 2016-09-19 23:45, Tobias Jakobi wrote:
> > I did some tests with the new version today. Sadly the reboot/shutdown
> > issues are still present.
> 
> Thanks for the report. I've managed to reproduce this issue and it is again
> caused by modifying device on devices_kset list before it will be finally
> added by device_add(). I thought that the new patchset allows creating links
> to a device, which has not been yet added to system device list.
> 
> Rafael:
> What is your opinion?

Well, this is a problem. :-)

> Should it be allowed to create a link to device, which
> has not yet been added to system device list by device_add()?

While it would be easy to require both the consumer and producer devices to
be registered for creating a link between them, that would just make it harder
to use links in the first place.

So ideally, it should be possible to create links between devices before
registering them, but since I didn't take that into account in the current
patch series, some quite substantial changes are needed to cover that.

Additional link states come to mind, but then the "stateless" links are
affected by this problem too.

> My code used to do that, because IOMMUs are configured from 
> of_platform_device_create_pdata()
> of_dma_configure() of_iommu_configure(), which happens before device_add().
> 
> To solve the reported corruption of devices_kset list, following change is
> needed:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index aa8196508db9..4542ba9f60d4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1039,11 +1039,18 @@ static void devices_kset_move_after(struct 
> device *deva, struct device *devb)
>    */
>   void devices_kset_move_last(struct device *dev)
>   {
> +       struct device *d;
> +
>          if (!devices_kset)
>                  return;
>          pr_debug("devices_kset: Moving %s to end of list\n", 
> dev_name(dev));
>          spin_lock(&devices_kset->list_lock);
> -       list_move_tail(&dev->kobj.entry, &devices_kset->list);
> +       list_for_each_entry(d, &devices_kset->list, kobj.entry) {
> +               if (d == dev) {
> +                       list_move_tail(&dev->kobj.entry, 
> &devices_kset->list);
> +                       break;
> +               }
> +       }
>          spin_unlock(&devices_kset->list_lock);
>   }
> 
> 
> If you think that links can be created only to a device, which has been 
> fully
> added to the system, I will register a bus notifier and create a link on 
> consumers
> device probe then.

That would be a workaround for a coverage gap, so not particularly attractive.

Thanks,
Rafael

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Lukas Wunner]

On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> > On 2016-09-19 23:45, Tobias Jakobi wrote:
> > > I did some tests with the new version today. Sadly the reboot/shutdown
> > > issues are still present.
> > 
> > Thanks for the report. I've managed to reproduce this issue and it is again
> > caused by modifying device on devices_kset list before it will be finally
> > added by device_add(). I thought that the new patchset allows creating
> > links to a device, which has not been yet added to system device list.

Hm, Marek, why isn't it possible to set up the links from the consumer's
->probe hook in this case?


> > Should it be allowed to create a link to device, which
> > has not yet been added to system device list by device_add()?
> 
> While it would be easy to require both the consumer and producer devices to
> be registered for creating a link between them, that would just make it
> harder to use links in the first place.
> 
> So ideally, it should be possible to create links between devices before
> registering them, but since I didn't take that into account in the current
> patch series, some quite substantial changes are needed to cover that.
> 
> Additional link states come to mind, but then the "stateless" links are
> affected by this problem too.

device_link_add() could be changed to call device_reorder_to_tail()
only if device_is_registered(consumer) returns true.

That's an inline function defined in <linux/device.h> which returns
dev->kobj.state_in_sysfs, a flag which is set in kobject_add().

Then device_add() would have to check if any links are already
set up and reorder the consumer behind the suppliers.

Doesn't seem to be *that* complex, but probably I'm missing something,
this is just off the cuff...

Best regards,

Lukas

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Lukas Wunner]

On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> > On 2016-09-19 23:45, Tobias Jakobi wrote:
> > > I did some tests with the new version today. Sadly the reboot/shutdown
> > > issues are still present.
> > 
> > Thanks for the report. I've managed to reproduce this issue and it is again
> > caused by modifying device on devices_kset list before it will be finally
> > added by device_add(). I thought that the new patchset allows creating
> > links to a device, which has not been yet added to system device list.

Hm, Marek, why isn't it possible to set up the links from the consumer's
->probe hook in this case?


> > Should it be allowed to create a link to device, which
> > has not yet been added to system device list by device_add()?
> 
> While it would be easy to require both the consumer and producer devices to
> be registered for creating a link between them, that would just make it
> harder to use links in the first place.
> 
> So ideally, it should be possible to create links between devices before
> registering them, but since I didn't take that into account in the current
> patch series, some quite substantial changes are needed to cover that.
> 
> Additional link states come to mind, but then the "stateless" links are
> affected by this problem too.

device_link_add() could be changed to call device_reorder_to_tail()
only if device_is_registered(consumer) returns true.

That's an inline function defined in <linux/device.h> which returns
dev->kobj.state_in_sysfs, a flag which is set in kobject_add().

Then device_add() would have to check if any links are already
set up and reorder the consumer behind the suppliers.

Doesn't seem to be *that* complex, but probably I'm missing something,
this is just off the cuff...

Best regards,

Lukas

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Rafael J. Wysocki]

On Friday, September 23, 2016 03:50:02 PM Lukas Wunner wrote:
> On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> > > On 2016-09-19 23:45, Tobias Jakobi wrote:
> > > > I did some tests with the new version today. Sadly the reboot/shutdown
> > > > issues are still present.
> > > 
> > > Thanks for the report. I've managed to reproduce this issue and it is again
> > > caused by modifying device on devices_kset list before it will be finally
> > > added by device_add(). I thought that the new patchset allows creating
> > > links to a device, which has not been yet added to system device list.
> 
> Hm, Marek, why isn't it possible to set up the links from the consumer's
> ->probe hook in this case?
> 
> 
> > > Should it be allowed to create a link to device, which
> > > has not yet been added to system device list by device_add()?
> > 
> > While it would be easy to require both the consumer and producer devices to
> > be registered for creating a link between them, that would just make it
> > harder to use links in the first place.
> > 
> > So ideally, it should be possible to create links between devices before
> > registering them, but since I didn't take that into account in the current
> > patch series, some quite substantial changes are needed to cover that.
> > 
> > Additional link states come to mind, but then the "stateless" links are
> > affected by this problem too.
> 
> device_link_add() could be changed to call device_reorder_to_tail()
> only if device_is_registered(consumer) returns true.
> 
> That's an inline function defined in <linux/device.h> which returns
> dev->kobj.state_in_sysfs, a flag which is set in kobject_add().

I know what that function is, but using it alone is not sufficient,
because dev->kobj.state_in_sysfs is set before the device is added to
dpm_list.

> Then device_add() would have to check if any links are already
> set up and reorder the consumer behind the suppliers.
> 
> Doesn't seem to be *that* complex, but probably I'm missing something,
> this is just off the cuff...

There are some cases to consider and some races to avoid AFAICS.

It all gets a lot simpler if device_link_add() is allowed to return NULL when
the supplier device passed to it has not been registered yet.  That looks like
a reasonable thing to do to me, but I wonder if someone has a use case in which
it would be a substantial limitation.

Thanks,
Rafael

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hi Rafael,


On 2016-09-24 03:25, Rafael J. Wysocki wrote:
> On Friday, September 23, 2016 03:50:02 PM Lukas Wunner wrote:
>> On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
>>> On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
>>>> On 2016-09-19 23:45, Tobias Jakobi wrote:
>>>>> I did some tests with the new version today. Sadly the reboot/shutdown
>>>>> issues are still present.
>>>> Thanks for the report. I've managed to reproduce this issue and it is again
>>>> caused by modifying device on devices_kset list before it will be finally
>>>> added by device_add(). I thought that the new patchset allows creating
>>>> links to a device, which has not been yet added to system device list.
>> Hm, Marek, why isn't it possible to set up the links from the consumer's
>> ->probe hook in this case?

Because consumers are unaware of the IOMMU presence, so they are also 
unaware
of the links that have to be created.

>>>> Should it be allowed to create a link to device, which
>>>> has not yet been added to system device list by device_add()?
>>> While it would be easy to require both the consumer and producer devices to
>>> be registered for creating a link between them, that would just make it
>>> harder to use links in the first place.
>>>
>>> So ideally, it should be possible to create links between devices before
>>> registering them, but since I didn't take that into account in the current
>>> patch series, some quite substantial changes are needed to cover that.
>>>
>>> Additional link states come to mind, but then the "stateless" links are
>>> affected by this problem too.
>> device_link_add() could be changed to call device_reorder_to_tail()
>> only if device_is_registered(consumer) returns true.
>>
>> That's an inline function defined in <linux/device.h> which returns
>> dev->kobj.state_in_sysfs, a flag which is set in kobject_add().
> I know what that function is, but using it alone is not sufficient,
> because dev->kobj.state_in_sysfs is set before the device is added to
> dpm_list.

I found that checking for dev->p was enough to check if device has been
added to system or not, but this seems to be some kind of ugly workaround:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4542ba9f60d4..780495918b53 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -180,9 +180,11 @@ struct device_link *device_link_add(struct device 
*consumer,
          * It is necessary to hold dpm_list locked throughout all that 
or else
          * we may end up suspending with a wrong ordering of it.
          */
-       device_pm_lock();
-       device_reorder_to_tail(consumer, NULL);
-       device_pm_unlock();
+       if (consumer->p) {
+               device_pm_lock();
+               device_reorder_to_tail(consumer, NULL);
+               device_pm_unlock();
+       }

         list_add_tail_rcu(&link->s_node, &supplier->links_to_consumers);
         list_add_tail_rcu(&link->c_node, &consumer->links_to_suppliers);


>
>> Then device_add() would have to check if any links are already
>> set up and reorder the consumer behind the suppliers.
>>
>> Doesn't seem to be *that* complex, but probably I'm missing something,
>> this is just off the cuff...
> There are some cases to consider and some races to avoid AFAICS.
>
> It all gets a lot simpler if device_link_add() is allowed to return NULL when
> the supplier device passed to it has not been registered yet.  That looks like
> a reasonable thing to do to me, but I wonder if someone has a use case in which
> it would be a substantial limitation.

Hmmm, you are talking here about the supplier, but my case is that 
supplier is
already registered and probed, but the consumer is about to be created. 
If you
think that supporting such case makes no sense, then I will use the 
workaround
with bus notifier I mentioned earlier.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Rafael J. Wysocki]

On Monday, September 26, 2016 10:15:24 AM Marek Szyprowski wrote:
> Hi Rafael,
> 
> 
> On 2016-09-24 03:25, Rafael J. Wysocki wrote:
> > On Friday, September 23, 2016 03:50:02 PM Lukas Wunner wrote:
> >> On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
> >>> On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> >>>> On 2016-09-19 23:45, Tobias Jakobi wrote:
> >>>>> I did some tests with the new version today. Sadly the reboot/shutdown
> >>>>> issues are still present.
> >>>> Thanks for the report. I've managed to reproduce this issue and it is again
> >>>> caused by modifying device on devices_kset list before it will be finally
> >>>> added by device_add(). I thought that the new patchset allows creating
> >>>> links to a device, which has not been yet added to system device list.
> >> Hm, Marek, why isn't it possible to set up the links from the consumer's
> >> ->probe hook in this case?
> 
> Because consumers are unaware of the IOMMU presence, so they are also 
> unaware
> of the links that have to be created.
> 
> >>>> Should it be allowed to create a link to device, which
> >>>> has not yet been added to system device list by device_add()?
> >>> While it would be easy to require both the consumer and producer devices to
> >>> be registered for creating a link between them, that would just make it
> >>> harder to use links in the first place.
> >>>
> >>> So ideally, it should be possible to create links between devices before
> >>> registering them, but since I didn't take that into account in the current
> >>> patch series, some quite substantial changes are needed to cover that.
> >>>
> >>> Additional link states come to mind, but then the "stateless" links are
> >>> affected by this problem too.
> >> device_link_add() could be changed to call device_reorder_to_tail()
> >> only if device_is_registered(consumer) returns true.
> >>
> >> That's an inline function defined in <linux/device.h> which returns
> >> dev->kobj.state_in_sysfs, a flag which is set in kobject_add().
> > I know what that function is, but using it alone is not sufficient,
> > because dev->kobj.state_in_sysfs is set before the device is added to
> > dpm_list.
> 
> I found that checking for dev->p was enough to check if device has been
> added to system or not, but this seems to be some kind of ugly workaround:
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4542ba9f60d4..780495918b53 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -180,9 +180,11 @@ struct device_link *device_link_add(struct device 
> *consumer,
>           * It is necessary to hold dpm_list locked throughout all that 
> or else
>           * we may end up suspending with a wrong ordering of it.
>           */
> -       device_pm_lock();
> -       device_reorder_to_tail(consumer, NULL);
> -       device_pm_unlock();
> +       if (consumer->p) {
> +               device_pm_lock();
> +               device_reorder_to_tail(consumer, NULL);
> +               device_pm_unlock();
> +       }

This still is somewhat racy, because the device may not be in dpm_list yet
even if consumer->p is set.

There needs to be something checked and set under device_pm_lock() to avoid
that race.

Let me add it to the patchset and we'll see.

> 
>          list_add_tail_rcu(&link->s_node, &supplier->links_to_consumers);
>          list_add_tail_rcu(&link->c_node, &consumer->links_to_suppliers);
> 
> 
> >
> >> Then device_add() would have to check if any links are already
> >> set up and reorder the consumer behind the suppliers.
> >>
> >> Doesn't seem to be *that* complex, but probably I'm missing something,
> >> this is just off the cuff...
> > There are some cases to consider and some races to avoid AFAICS.
> >
> > It all gets a lot simpler if device_link_add() is allowed to return NULL when
> > the supplier device passed to it has not been registered yet.  That looks like
> > a reasonable thing to do to me, but I wonder if someone has a use case in which
> > it would be a substantial limitation.
> 
> Hmmm, you are talking here about the supplier, but my case is that 
> supplier is
> already registered and probed, but the consumer is about to be created. 

Right.

> If you
> think that supporting such case makes no sense,

I think that it does make sense.

I only was wondering if that was going to be sufficient. :-)

Thanks,
Rafael