[PATCH 0/3] link regulator consumer with driver

[PATCH 0/3] link regulator consumer with driver

[Pascal PAILLET-LME]

From: pascal paillet <p.paillet@st.com>

The goal of this patch-set is to ensure that a regulator driver is not suspended
before regulator consumer. Currently this is done by implementing suspend_late()
ops in the regulator driver but this is painful for an I2C controlled regulator.
Instead, the proposal is to add a device link between the driver and the consumer
at regulator core framework level.
To avoid storing the link pointer in the regulator core structure, we create
device_link_remove() function that use the same argument as device_link_add().

pascal paillet (3):
  driver core: Add device_link_remove function
  regulator: core: Link consumer with regulator driver
  regulator: core: Change suspend_late to suspend

 drivers/base/core.c              | 30 +++++++++++++++++++++++++++
 drivers/regulator/core.c         | 44 ++++++++++++++++++++++++++--------------
 include/linux/device.h           |  1 +
 include/linux/regulator/driver.h |  2 +-
 4 files changed, 61 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH 2/3] regulator: core: Link consumer with regulator driver

[Pascal PAILLET-LME]

From: pascal paillet <p.paillet@st.com>

Add a device link between the consumer and the driver so that
the consumer is not suspended before the driver. The goal is to avoid
implementing suspend_late ops in regulator drivers.

Signed-off-by: pascal paillet <p.paillet@st.com>
---
 drivers/regulator/core.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ed568b..607ec47 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1740,6 +1740,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 			rdev->use_count = 0;
 	}
 
+	device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
+
 	return regulator;
 }
 
@@ -1829,9 +1831,21 @@ static void _regulator_put(struct regulator *regulator)
 
 	debugfs_remove_recursive(regulator->debugfs);
 
-	/* remove any sysfs entries */
-	if (regulator->dev)
+	if (regulator->dev) {
+		int count = 0;
+		struct regulator *r;
+
+		list_for_each_entry(r, &rdev->consumer_list, list)
+			if (r->dev == regulator->dev)
+				count++;
+
+		if (count == 1)
+			device_link_remove(regulator->dev, &rdev->dev);
+
+		/* remove any sysfs entries */
 		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
+	}
+
 	regulator_lock(rdev);
 	list_del(&regulator->list);
 
-- 
1.9.1

[PATCH 1/3] driver core: Add device_link_remove function

[Pascal PAILLET-LME]

From: pascal paillet <p.paillet@st.com>

Device_link_remove uses the same arguments than device_link_add. The Goal
is to avoid storing the link pointer.

Signed-off-by: pascal paillet <p.paillet@st.com>
---
 drivers/base/core.c    | 30 ++++++++++++++++++++++++++++++
 include/linux/device.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 36622b5..3b380b1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -365,6 +365,36 @@ void device_link_del(struct device_link *link)
 }
 EXPORT_SYMBOL_GPL(device_link_del);
 
+/**
+ * device_link_remove - remove a link between two devices.
+ * @consumer: Consumer end of the link.
+ * @supplier: Supplier end of the link.
+ *
+ * The caller must ensure proper synchronization of this function with runtime
+ * PM.
+ */
+void device_link_remove(void *consumer, struct device *supplier)
+{
+	struct device_link *link;
+
+	if (WARN_ON(consumer == supplier))
+		return;
+
+	device_links_write_lock();
+	device_pm_lock();
+
+	list_for_each_entry(link, &supplier->links.consumers, s_node) {
+		if (link->consumer == consumer) {
+			kref_put(&link->kref, __device_link_del);
+			break;
+		}
+	}
+
+	device_pm_unlock();
+	device_links_write_unlock();
+}
+EXPORT_SYMBOL_GPL(device_link_remove);
+
 static void device_links_missing_supplier(struct device *dev)
 {
 	struct device_link *link;
diff --git a/include/linux/device.h b/include/linux/device.h
index 055a69d..9c1c3b1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1316,6 +1316,7 @@ extern void devm_device_remove_group(struct device *dev,
 struct device_link *device_link_add(struct device *consumer,
 				    struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
+void device_link_remove(void *consumer, struct device *supplier);
 
 #ifdef CONFIG_PRINTK
 
-- 
1.9.1

[PATCH 3/3] regulator: core: Change suspend_late to suspend

[Pascal PAILLET-LME]

From: pascal paillet <p.paillet@st.com>

Change suspend_late ops to suspend normal ops. The goal is to avoid
requesting all the regulator drivers to be operational in suspend late
phase.

Signed-off-by: pascal paillet <p.paillet@st.com>
---
 drivers/regulator/core.c         | 26 +++++++++++++-------------
 include/linux/regulator/driver.h |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 607ec47..bb1324f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4455,7 +4455,7 @@ void regulator_unregister(struct regulator_dev *rdev)
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
 #ifdef CONFIG_SUSPEND
-static int _regulator_suspend_late(struct device *dev, void *data)
+static int _regulator_suspend(struct device *dev, void *data)
 {
 	struct regulator_dev *rdev = dev_to_rdev(dev);
 	suspend_state_t *state = data;
@@ -4469,20 +4469,20 @@ static int _regulator_suspend_late(struct device *dev, void *data)
 }
 
 /**
- * regulator_suspend_late - prepare regulators for system wide suspend
+ * regulator_suspend - prepare regulators for system wide suspend
  * @state: system suspend state
  *
  * Configure each regulator with it's suspend operating parameters for state.
  */
-static int regulator_suspend_late(struct device *dev)
+static int regulator_suspend(struct device *dev)
 {
 	suspend_state_t state = pm_suspend_target_state;
 
 	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_suspend_late);
+				     _regulator_suspend);
 }
 
-static int _regulator_resume_early(struct device *dev, void *data)
+static int _regulator_resume(struct device *dev, void *data)
 {
 	int ret = 0;
 	struct regulator_dev *rdev = dev_to_rdev(dev);
@@ -4495,35 +4495,35 @@ static int _regulator_resume_early(struct device *dev, void *data)
 
 	regulator_lock(rdev);
 
-	if (rdev->desc->ops->resume_early &&
+	if (rdev->desc->ops->resume &&
 	    (rstate->enabled == ENABLE_IN_SUSPEND ||
 	     rstate->enabled == DISABLE_IN_SUSPEND))
-		ret = rdev->desc->ops->resume_early(rdev);
+		ret = rdev->desc->ops->resume(rdev);
 
 	regulator_unlock(rdev);
 
 	return ret;
 }
 
-static int regulator_resume_early(struct device *dev)
+static int regulator_resume(struct device *dev)
 {
 	suspend_state_t state = pm_suspend_target_state;
 
 	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_resume_early);
+				     _regulator_resume);
 }
 
 #else /* !CONFIG_SUSPEND */
 
-#define regulator_suspend_late	NULL
-#define regulator_resume_early	NULL
+#define regulator_suspend	NULL
+#define regulator_resume	NULL
 
 #endif /* !CONFIG_SUSPEND */
 
 #ifdef CONFIG_PM
 static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
-	.suspend_late	= regulator_suspend_late,
-	.resume_early	= regulator_resume_early,
+	.suspend	= regulator_suspend,
+	.resume		= regulator_resume,
 };
 #endif
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index fc2dc8d..3a0302b 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -220,7 +220,7 @@ struct regulator_ops {
 	/* set regulator suspend operating mode (defined in consumer.h) */
 	int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode);
 
-	int (*resume_early)(struct regulator_dev *rdev);
+	int (*resume)(struct regulator_dev *rdev);
 
 	int (*set_pull_down) (struct regulator_dev *);
 };
-- 
1.9.1

Re: [PATCH 2/3] regulator: core: Link consumer with regulator driver

[Mark Brown]

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

On Thu, Jul 05, 2018 at 02:25:56PM +0000, Pascal PAILLET-LME wrote:
> From: pascal paillet <p.paillet@st.com>
> 
> Add a device link between the consumer and the driver so that
> the consumer is not suspended before the driver. The goal is to avoid
> implementing suspend_late ops in regulator drivers.

This looks good but I'll wait for Greg's review of the link
functionality - Greg, is it OK to apply this via the regulator tree or
do you want a shared branch?

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

Re: [PATCH 1/3] driver core: Add device_link_remove function

[Mark Brown]

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

On Thu, Jul 05, 2018 at 02:25:56PM +0000, Pascal PAILLET-LME wrote:
> From: pascal paillet <p.paillet@st.com>
> 
> Device_link_remove uses the same arguments than device_link_add. The Goal
> is to avoid storing the link pointer.

Reviwed-by: Mark Brown <broonie@kernel.org>

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

Applied "regulator: core: Change suspend_late to suspend" to the regulator tree

[Mark Brown]

The patch

   regulator: core: Change suspend_late to suspend

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 0380cf7dbaca75c524e34b30979f0806124fa8e6 Mon Sep 17 00:00:00 2001
From: pascal paillet <p.paillet@st.com>
Date: Thu, 5 Jul 2018 14:25:57 +0000
Subject: [PATCH] regulator: core: Change suspend_late to suspend

Change suspend_late ops to suspend normal ops. The goal is to avoid
requesting all the regulator drivers to be operational in suspend late
phase.

Signed-off-by: pascal paillet <p.paillet@st.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c         | 26 +++++++++++++-------------
 include/linux/regulator/driver.h |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ed568b96c0e..da9b0fed8330 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4441,7 +4441,7 @@ void regulator_unregister(struct regulator_dev *rdev)
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
 #ifdef CONFIG_SUSPEND
-static int _regulator_suspend_late(struct device *dev, void *data)
+static int _regulator_suspend(struct device *dev, void *data)
 {
 	struct regulator_dev *rdev = dev_to_rdev(dev);
 	suspend_state_t *state = data;
@@ -4455,20 +4455,20 @@ static int _regulator_suspend_late(struct device *dev, void *data)
 }
 
 /**
- * regulator_suspend_late - prepare regulators for system wide suspend
+ * regulator_suspend - prepare regulators for system wide suspend
  * @state: system suspend state
  *
  * Configure each regulator with it's suspend operating parameters for state.
  */
-static int regulator_suspend_late(struct device *dev)
+static int regulator_suspend(struct device *dev)
 {
 	suspend_state_t state = pm_suspend_target_state;
 
 	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_suspend_late);
+				     _regulator_suspend);
 }
 
-static int _regulator_resume_early(struct device *dev, void *data)
+static int _regulator_resume(struct device *dev, void *data)
 {
 	int ret = 0;
 	struct regulator_dev *rdev = dev_to_rdev(dev);
@@ -4481,35 +4481,35 @@ static int _regulator_resume_early(struct device *dev, void *data)
 
 	regulator_lock(rdev);
 
-	if (rdev->desc->ops->resume_early &&
+	if (rdev->desc->ops->resume &&
 	    (rstate->enabled == ENABLE_IN_SUSPEND ||
 	     rstate->enabled == DISABLE_IN_SUSPEND))
-		ret = rdev->desc->ops->resume_early(rdev);
+		ret = rdev->desc->ops->resume(rdev);
 
 	regulator_unlock(rdev);
 
 	return ret;
 }
 
-static int regulator_resume_early(struct device *dev)
+static int regulator_resume(struct device *dev)
 {
 	suspend_state_t state = pm_suspend_target_state;
 
 	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_resume_early);
+				     _regulator_resume);
 }
 
 #else /* !CONFIG_SUSPEND */
 
-#define regulator_suspend_late	NULL
-#define regulator_resume_early	NULL
+#define regulator_suspend	NULL
+#define regulator_resume	NULL
 
 #endif /* !CONFIG_SUSPEND */
 
 #ifdef CONFIG_PM
 static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
-	.suspend_late	= regulator_suspend_late,
-	.resume_early	= regulator_resume_early,
+	.suspend	= regulator_suspend,
+	.resume		= regulator_resume,
 };
 #endif
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index dea96ee39fdc..0fd8fbb74763 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -220,7 +220,7 @@ struct regulator_ops {
 	/* set regulator suspend operating mode (defined in consumer.h) */
 	int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode);
 
-	int (*resume_early)(struct regulator_dev *rdev);
+	int (*resume)(struct regulator_dev *rdev);
 
 	int (*set_pull_down) (struct regulator_dev *);
 };
-- 
2.18.0.rc2

Re: [PATCH 2/3] regulator: core: Link consumer with regulator driver

[gregkh]

On Thu, Jul 05, 2018 at 05:53:51PM +0100, Mark Brown wrote:
> On Thu, Jul 05, 2018 at 02:25:56PM +0000, Pascal PAILLET-LME wrote:
> > From: pascal paillet <p.paillet@st.com>
> > 
> > Add a device link between the consumer and the driver so that
> > the consumer is not suspended before the driver. The goal is to avoid
> > implementing suspend_late ops in regulator drivers.
> 
> This looks good but I'll wait for Greg's review of the link
> functionality - Greg, is it OK to apply this via the regulator tree or
> do you want a shared branch?

You can take it through the regulator tree, no objection from me:
	Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied "regulator: core: Link consumer with regulator driver" to the regulator tree

[Mark Brown]

The patch

   regulator: core: Link consumer with regulator driver

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From ed1ae2dd9f242c7a36e8e39100f6a7f6bcdfdd89 Mon Sep 17 00:00:00 2001
From: pascal paillet <p.paillet@st.com>
Date: Thu, 5 Jul 2018 14:25:56 +0000
Subject: [PATCH] regulator: core: Link consumer with regulator driver

Add a device link between the consumer and the driver so that
the consumer is not suspended before the driver. The goal is to avoid
implementing suspend_late ops in regulator drivers.

Signed-off-by: pascal paillet <p.paillet@st.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index da9b0fed8330..bb1324f93143 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1740,6 +1740,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 			rdev->use_count = 0;
 	}
 
+	device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
+
 	return regulator;
 }
 
@@ -1829,9 +1831,21 @@ static void _regulator_put(struct regulator *regulator)
 
 	debugfs_remove_recursive(regulator->debugfs);
 
-	/* remove any sysfs entries */
-	if (regulator->dev)
+	if (regulator->dev) {
+		int count = 0;
+		struct regulator *r;
+
+		list_for_each_entry(r, &rdev->consumer_list, list)
+			if (r->dev == regulator->dev)
+				count++;
+
+		if (count == 1)
+			device_link_remove(regulator->dev, &rdev->dev);
+
+		/* remove any sysfs entries */
 		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
+	}
+
 	regulator_lock(rdev);
 	list_del(&regulator->list);
 
-- 
2.18.0.rc2

Applied "driver core: Add device_link_remove function" to the regulator tree

[Mark Brown]

The patch

   driver core: Add device_link_remove function

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From d8842211b6f63d3f069df973d137de0a85964965 Mon Sep 17 00:00:00 2001
From: pascal paillet <p.paillet@st.com>
Date: Thu, 5 Jul 2018 14:25:56 +0000
Subject: [PATCH] driver core: Add device_link_remove function

Device_link_remove uses the same arguments than device_link_add. The Goal
is to avoid storing the link pointer.

Signed-off-by: pascal paillet <p.paillet@st.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/core.c    | 30 ++++++++++++++++++++++++++++++
 include/linux/device.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 36622b52e419..3b380b1f2768 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -365,6 +365,36 @@ void device_link_del(struct device_link *link)
 }
 EXPORT_SYMBOL_GPL(device_link_del);
 
+/**
+ * device_link_remove - remove a link between two devices.
+ * @consumer: Consumer end of the link.
+ * @supplier: Supplier end of the link.
+ *
+ * The caller must ensure proper synchronization of this function with runtime
+ * PM.
+ */
+void device_link_remove(void *consumer, struct device *supplier)
+{
+	struct device_link *link;
+
+	if (WARN_ON(consumer == supplier))
+		return;
+
+	device_links_write_lock();
+	device_pm_lock();
+
+	list_for_each_entry(link, &supplier->links.consumers, s_node) {
+		if (link->consumer == consumer) {
+			kref_put(&link->kref, __device_link_del);
+			break;
+		}
+	}
+
+	device_pm_unlock();
+	device_links_write_unlock();
+}
+EXPORT_SYMBOL_GPL(device_link_remove);
+
 static void device_links_missing_supplier(struct device *dev)
 {
 	struct device_link *link;
diff --git a/include/linux/device.h b/include/linux/device.h
index 055a69dbcd18..9c1c3b1d5e11 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1316,6 +1316,7 @@ extern const char *dev_driver_string(const struct device *dev);
 struct device_link *device_link_add(struct device *consumer,
 				    struct device *supplier, u32 flags);
 void device_link_del(struct device_link *link);
+void device_link_remove(void *consumer, struct device *supplier);
 
 #ifdef CONFIG_PRINTK
 
-- 
2.18.0.rc2

Re: Applied "regulator: core: Link consumer with regulator driver" to the regulator tree

[Marek Szyprowski]

Dear All,

On 2018-07-05 19:55, Mark Brown wrote:
> The patch
>
>     regulator: core: Link consumer with regulator driver
>
> has been applied to the regulator tree at
>
>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
>
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
>
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
>
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
>
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
>
> Thanks,
> Mark
>
> >From ed1ae2dd9f242c7a36e8e39100f6a7f6bcdfdd89 Mon Sep 17 00:00:00 2001
> From: pascal paillet <p.paillet@st.com>
> Date: Thu, 5 Jul 2018 14:25:56 +0000
> Subject: [PATCH] regulator: core: Link consumer with regulator driver
>
> Add a device link between the consumer and the driver so that
> the consumer is not suspended before the driver. The goal is to avoid
> implementing suspend_late ops in regulator drivers.
>
> Signed-off-by: pascal paillet <p.paillet@st.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>

This patch triggers the following warning on several Exynos SoC based 
boards:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 1 at drivers/base/core.c:108 
device_is_dependent+0xa4/0xb4
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180706 #112
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0113200>] (unwind_backtrace) from [<c010edd0>] (show_stack+0x10/0x14)
[<c010edd0>] (show_stack) from [<c0a4b6e8>] (dump_stack+0x98/0xc4)
[<c0a4b6e8>] (dump_stack) from [<c0127b90>] (__warn+0x10c/0x124)
[<c0127b90>] (__warn) from [<c0127cbc>] (warn_slowpath_null+0x40/0x48)
[<c0127cbc>] (warn_slowpath_null) from [<c0580464>] 
(device_is_dependent+0xa4/0xb4)
[<c0580464>] (device_is_dependent) from [<c058037c>] 
(device_for_each_child+0x50/0x94)
[<c058037c>] (device_for_each_child) from [<c05803e0>] 
(device_is_dependent+0x20/0xb4)
[<c05803e0>] (device_is_dependent) from [<c0581bcc>] 
(device_link_add+0x70/0x2a8)
[<c0581bcc>] (device_link_add) from [<c04e4134>] (_regulator_get+0xbc/0x270)
[<c04e4134>] (_regulator_get) from [<c04e4350>] 
(regulator_bulk_get+0x60/0xcc)
[<c04e4350>] (regulator_bulk_get) from [<c05b4e40>] 
(wm8994_i2c_probe+0x334/0x8ec)
[<c05b4e40>] (wm8994_i2c_probe) from [<c06d8d98>] 
(i2c_device_probe+0x240/0x2b8)
[<c06d8d98>] (i2c_device_probe) from [<c05857d8>] 
(driver_probe_device+0x2dc/0x4ac)
[<c05857d8>] (driver_probe_device) from [<c0585ad0>] 
(__driver_attach+0x128/0x144)
[<c0585ad0>] (__driver_attach) from [<c0583668>] 
(bus_for_each_dev+0x68/0xb4)
[<c0583668>] (bus_for_each_dev) from [<c0584968>] 
(bus_add_driver+0x1a8/0x268)
[<c0584968>] (bus_add_driver) from [<c0586c48>] (driver_register+0x78/0x10c)
[<c0586c48>] (driver_register) from [<c06d9ca4>] 
(i2c_register_driver+0x3c/0xa8)
[<c06d9ca4>] (i2c_register_driver) from [<c0103188>] 
(do_one_initcall+0x8c/0x4b0)
[<c0103188>] (do_one_initcall) from [<c0f01268>] 
(kernel_init_freeable+0x3e4/0x570)
[<c0f01268>] (kernel_init_freeable) from [<c0a6503c>] 
(kernel_init+0x8/0x10c)
[<c0a6503c>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xee8c9fb0 to 0xee8c9ff8)
...
---[ end trace 4ed89cca1d70ea82 ]---

The above stack comes from Exynos5250 Arndale board
(arch/arm/boot/dts/exynos5250-arndale.dts), where wm8994 codec tries to get
regulator, which is provided by its parent mfd device. Similar issue can be
observed on Exynos4412-based Trats2 and Exynos5433 TM2(e) boards.

It looks that some more checks have to be done before adding a link between
regulator consumer and regulator driver, because it is not that uncommon 
that
regulator consumer shares the parent device with regulator provider.

> ---
>   drivers/regulator/core.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index da9b0fed8330..bb1324f93143 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1740,6 +1740,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
>   			rdev->use_count = 0;
>   	}
>   
> +	device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
> +
>   	return regulator;
>   }
>   
> @@ -1829,9 +1831,21 @@ static void _regulator_put(struct regulator *regulator)
>   
>   	debugfs_remove_recursive(regulator->debugfs);
>   
> -	/* remove any sysfs entries */
> -	if (regulator->dev)
> +	if (regulator->dev) {
> +		int count = 0;
> +		struct regulator *r;
> +
> +		list_for_each_entry(r, &rdev->consumer_list, list)
> +			if (r->dev == regulator->dev)
> +				count++;
> +
> +		if (count == 1)
> +			device_link_remove(regulator->dev, &rdev->dev);
> +
> +		/* remove any sysfs entries */
>   		sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
> +	}
> +
>   	regulator_lock(rdev);
>   	list_del(&regulator->list);
>   

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

Re: Applied "regulator: core: Link consumer with regulator driver" to the regulator tree

[Benjamin Gaignard]

+ Rafael

2018-07-09 13:17 GMT+02:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Dear All,
>
> On 2018-07-05 19:55, Mark Brown wrote:
>> The patch
>>
>>     regulator: core: Link consumer with regulator driver
>>
>> has been applied to the regulator tree at
>>
>>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
>>
>> All being well this means that it will be integrated into the linux-next
>> tree (usually sometime in the next 24 hours) and sent to Linus during
>> the next merge window (or sooner if it is a bug fix), however if
>> problems are discovered then the patch may be dropped or reverted.
>>
>> You may get further e-mails resulting from automated or manual testing
>> and review of the tree, please engage with people reporting problems and
>> send followup patches addressing any issues that are reported if needed.
>>
>> If any updates are required or you are submitting further changes they
>> should be sent as incremental updates against current git, existing
>> patches will not be replaced.
>>
>> Please add any relevant lists and maintainers to the CCs when replying
>> to this mail.
>>
>> Thanks,
>> Mark
>>
>> >From ed1ae2dd9f242c7a36e8e39100f6a7f6bcdfdd89 Mon Sep 17 00:00:00 2001
>> From: pascal paillet <p.paillet@st.com>
>> Date: Thu, 5 Jul 2018 14:25:56 +0000
>> Subject: [PATCH] regulator: core: Link consumer with regulator driver
>>
>> Add a device link between the consumer and the driver so that
>> the consumer is not suspended before the driver. The goal is to avoid
>> implementing suspend_late ops in regulator drivers.
>>
>> Signed-off-by: pascal paillet <p.paillet@st.com>
>> Signed-off-by: Mark Brown <broonie@kernel.org>
>
> This patch triggers the following warning on several Exynos SoC based
> boards:
>
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1 at drivers/base/core.c:108
> device_is_dependent+0xa4/0xb4
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180706 #112
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [<c0113200>] (unwind_backtrace) from [<c010edd0>] (show_stack+0x10/0x14)
> [<c010edd0>] (show_stack) from [<c0a4b6e8>] (dump_stack+0x98/0xc4)
> [<c0a4b6e8>] (dump_stack) from [<c0127b90>] (__warn+0x10c/0x124)
> [<c0127b90>] (__warn) from [<c0127cbc>] (warn_slowpath_null+0x40/0x48)
> [<c0127cbc>] (warn_slowpath_null) from [<c0580464>]
> (device_is_dependent+0xa4/0xb4)
> [<c0580464>] (device_is_dependent) from [<c058037c>]
> (device_for_each_child+0x50/0x94)
> [<c058037c>] (device_for_each_child) from [<c05803e0>]
> (device_is_dependent+0x20/0xb4)
> [<c05803e0>] (device_is_dependent) from [<c0581bcc>]
> (device_link_add+0x70/0x2a8)
> [<c0581bcc>] (device_link_add) from [<c04e4134>] (_regulator_get+0xbc/0x270)
> [<c04e4134>] (_regulator_get) from [<c04e4350>]
> (regulator_bulk_get+0x60/0xcc)
> [<c04e4350>] (regulator_bulk_get) from [<c05b4e40>]
> (wm8994_i2c_probe+0x334/0x8ec)
> [<c05b4e40>] (wm8994_i2c_probe) from [<c06d8d98>]
> (i2c_device_probe+0x240/0x2b8)
> [<c06d8d98>] (i2c_device_probe) from [<c05857d8>]
> (driver_probe_device+0x2dc/0x4ac)
> [<c05857d8>] (driver_probe_device) from [<c0585ad0>]
> (__driver_attach+0x128/0x144)
> [<c0585ad0>] (__driver_attach) from [<c0583668>]
> (bus_for_each_dev+0x68/0xb4)
> [<c0583668>] (bus_for_each_dev) from [<c0584968>]
> (bus_add_driver+0x1a8/0x268)
> [<c0584968>] (bus_add_driver) from [<c0586c48>] (driver_register+0x78/0x10c)
> [<c0586c48>] (driver_register) from [<c06d9ca4>]
> (i2c_register_driver+0x3c/0xa8)
> [<c06d9ca4>] (i2c_register_driver) from [<c0103188>]
> (do_one_initcall+0x8c/0x4b0)
> [<c0103188>] (do_one_initcall) from [<c0f01268>]
> (kernel_init_freeable+0x3e4/0x570)
> [<c0f01268>] (kernel_init_freeable) from [<c0a6503c>]
> (kernel_init+0x8/0x10c)
> [<c0a6503c>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xee8c9fb0 to 0xee8c9ff8)
> ...
> ---[ end trace 4ed89cca1d70ea82 ]---
>
> The above stack comes from Exynos5250 Arndale board
> (arch/arm/boot/dts/exynos5250-arndale.dts), where wm8994 codec tries to get
> regulator, which is provided by its parent mfd device. Similar issue can be
> observed on Exynos4412-based Trats2 and Exynos5433 TM2(e) boards.
>
> It looks that some more checks have to be done before adding a link between
> regulator consumer and regulator driver, because it is not that uncommon
> that
> regulator consumer shares the parent device with regulator provider.
>

Each time device_is_dependent() will return 1 we have a warn_on.
It is strange since returning 1 is well documented on function description.
I don't know if we can safely remove the warn_on or if their is a good
reason to keep it.
Rafael what do you think about that ?

>> ---
>>   drivers/regulator/core.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index da9b0fed8330..bb1324f93143 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -1740,6 +1740,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
>>                       rdev->use_count = 0;
>>       }
>>
>> +     device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
>> +
>>       return regulator;
>>   }
>>
>> @@ -1829,9 +1831,21 @@ static void _regulator_put(struct regulator *regulator)
>>
>>       debugfs_remove_recursive(regulator->debugfs);
>>
>> -     /* remove any sysfs entries */
>> -     if (regulator->dev)
>> +     if (regulator->dev) {
>> +             int count = 0;
>> +             struct regulator *r;
>> +
>> +             list_for_each_entry(r, &rdev->consumer_list, list)
>> +                     if (r->dev == regulator->dev)
>> +                             count++;
>> +
>> +             if (count == 1)
>> +                     device_link_remove(regulator->dev, &rdev->dev);
>> +
>> +             /* remove any sysfs entries */
>>               sysfs_remove_link(&rdev->dev.kobj, regulator->supply_name);
>> +     }
>> +
>>       regulator_lock(rdev);
>>       list_del(&regulator->list);
>>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: Applied "regulator: core: Link consumer with regulator driver" to the regulator tree

[Mark Brown]

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

On Mon, Jul 09, 2018 at 02:13:35PM +0200, Benjamin Gaignard wrote:
> 2018-07-09 13:17 GMT+02:00 Marek Szyprowski <m.szyprowski@samsung.com>:

> > ------------[ cut here ]------------
> > WARNING: CPU: 1 PID: 1 at drivers/base/core.c:108
> > device_is_dependent+0xa4/0xb4
> > Modules linked in:
> > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc3-next-20180706 #112

> > It looks that some more checks have to be done before adding a link between
> > regulator consumer and regulator driver, because it is not that uncommon
> > that
> > regulator consumer shares the parent device with regulator provider.

> Each time device_is_dependent() will return 1 we have a warn_on.
> It is strange since returning 1 is well documented on function description.
> I don't know if we can safely remove the warn_on or if their is a good
> reason to keep it.
> Rafael what do you think about that ?

It's trying to tell you that there's probably a higher level bug if
you're adding a link from a device to itself.  That's not super
unreasonable, though it is going to mean that every bit of generic code
like this adding links is going to have to add checks (or we have to
downgrade or make optional the WARN_ON).

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