From: pascal paillet <[email protected]>
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
From: pascal paillet <[email protected]>
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 <[email protected]>
---
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(®ulator_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(®ulator_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
From: pascal paillet <[email protected]>
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 <[email protected]>
---
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(®ulator->list);
--
1.9.1
From: pascal paillet <[email protected]>
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 <[email protected]>
---
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
On Thu, Jul 05, 2018 at 02:25:56PM +0000, Pascal PAILLET-LME wrote:
> From: pascal paillet <[email protected]>
>
> 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?
On Thu, Jul 05, 2018 at 02:25:56PM +0000, Pascal PAILLET-LME wrote:
> From: pascal paillet <[email protected]>
>
> Device_link_remove uses the same arguments than device_link_add. The Goal
> is to avoid storing the link pointer.
Reviwed-by: Mark Brown <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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(®ulator_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(®ulator_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
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 <[email protected]>
> >
> > 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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(®ulator->list);
--
2.18.0.rc2
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Mark Brown <[email protected]>
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(®ulator->list);
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
+ Rafael
2018-07-09 13:17 GMT+02:00 Marek Szyprowski <[email protected]>:
> 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 <[email protected]>
>> 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 <[email protected]>
>> Signed-off-by: Mark Brown <[email protected]>
>
> 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(®ulator->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
On Mon, Jul 09, 2018 at 02:13:35PM +0200, Benjamin Gaignard wrote:
> 2018-07-09 13:17 GMT+02:00 Marek Szyprowski <[email protected]>:
> > ------------[ 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).