* [RESEND 1/2] power_supply: Register cooling device outside of probe
@ 2022-05-31 18:30 Manaf Meethalavalappu Pallikunhi
2022-05-31 18:30 ` [RESEND 2/2] power_supply: Use of-thermal cdev registration API Manaf Meethalavalappu Pallikunhi
2022-06-09 22:12 ` [RESEND 1/2] power_supply: Register cooling device outside of probe Sebastian Reichel
0 siblings, 2 replies; 6+ messages in thread
From: Manaf Meethalavalappu Pallikunhi @ 2022-05-31 18:30 UTC (permalink / raw)
To: Sebastian Reichel
Cc: linux-pm, linux-kernel, gregkh, Subbaraman Narayanamurthy,
Manaf Meethalavalappu Pallikunhi
Registering the cooling device from the probe can result in the
execution of get_property() function before it gets initialized.
To avoid this, register the cooling device from a workqueue
instead of registering in the probe.
Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
---
drivers/power/supply/power_supply_core.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 385814a14a0a..74623c4977db 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -132,6 +132,7 @@ void power_supply_changed(struct power_supply *psy)
}
EXPORT_SYMBOL_GPL(power_supply_changed);
+static int psy_register_cooler(struct power_supply *psy);
/*
* Notify that power supply was registered after parent finished the probing.
*
@@ -139,6 +140,8 @@ EXPORT_SYMBOL_GPL(power_supply_changed);
* calling power_supply_changed() directly from power_supply_register()
* would lead to execution of get_property() function provided by the driver
* too early - before the probe ends.
+ * Also, registering cooling device from the probe will execute the
+ * get_property() function. So register the cooling device after the probe.
*
* Avoid that by waiting on parent's mutex.
*/
@@ -156,6 +159,7 @@ static void power_supply_deferred_register_work(struct work_struct *work)
}
power_supply_changed(psy);
+ psy_register_cooler(psy);
if (psy->dev.parent)
mutex_unlock(&psy->dev.parent->mutex);
@@ -1261,10 +1265,6 @@ __power_supply_register(struct device *parent,
if (rc)
goto register_thermal_failed;
- rc = psy_register_cooler(psy);
- if (rc)
- goto register_cooler_failed;
-
rc = power_supply_create_triggers(psy);
if (rc)
goto create_triggers_failed;
@@ -1294,8 +1294,6 @@ __power_supply_register(struct device *parent,
add_hwmon_sysfs_failed:
power_supply_remove_triggers(psy);
create_triggers_failed:
- psy_unregister_cooler(psy);
-register_cooler_failed:
psy_unregister_thermal(psy);
register_thermal_failed:
device_del(dev);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RESEND 2/2] power_supply: Use of-thermal cdev registration API
2022-05-31 18:30 [RESEND 1/2] power_supply: Register cooling device outside of probe Manaf Meethalavalappu Pallikunhi
@ 2022-05-31 18:30 ` Manaf Meethalavalappu Pallikunhi
2022-06-09 22:17 ` Sebastian Reichel
2022-06-09 22:12 ` [RESEND 1/2] power_supply: Register cooling device outside of probe Sebastian Reichel
1 sibling, 1 reply; 6+ messages in thread
From: Manaf Meethalavalappu Pallikunhi @ 2022-05-31 18:30 UTC (permalink / raw)
To: Sebastian Reichel
Cc: linux-pm, linux-kernel, gregkh, Subbaraman Narayanamurthy,
Manaf Meethalavalappu Pallikunhi
With thermal frameworks of-thermal interface, thermal zone parameters can
be defined in devicetree. This includes cooling device mitigation levels
for a thermal zone. To take advantage of this, cooling device should use
the thermal_of_cooling_device_register API to register a cooling device.
Use thermal_of_cooling_device_register API to register the power supply
cooling device. This enables power supply cooling device be included in the
thermal zone parameter in devicetree.
Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
---
drivers/power/supply/power_supply_core.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 74623c4977db..4593450920a4 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1157,9 +1157,15 @@ static int psy_register_cooler(struct power_supply *psy)
for (i = 0; i < psy->desc->num_properties; i++) {
if (psy->desc->properties[i] ==
POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT) {
- psy->tcd = thermal_cooling_device_register(
- (char *)psy->desc->name,
- psy, &psy_tcd_ops);
+ if (psy->dev.parent)
+ psy->tcd = thermal_of_cooling_device_register(
+ dev_of_node(psy->dev.parent),
+ (char *)psy->desc->name,
+ psy, &psy_tcd_ops);
+ else
+ psy->tcd = thermal_cooling_device_register(
+ (char *)psy->desc->name,
+ psy, &psy_tcd_ops);
return PTR_ERR_OR_ZERO(psy->tcd);
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND 1/2] power_supply: Register cooling device outside of probe
2022-05-31 18:30 [RESEND 1/2] power_supply: Register cooling device outside of probe Manaf Meethalavalappu Pallikunhi
2022-05-31 18:30 ` [RESEND 2/2] power_supply: Use of-thermal cdev registration API Manaf Meethalavalappu Pallikunhi
@ 2022-06-09 22:12 ` Sebastian Reichel
2023-02-27 21:46 ` Subbaraman Narayanamurthy
1 sibling, 1 reply; 6+ messages in thread
From: Sebastian Reichel @ 2022-06-09 22:12 UTC (permalink / raw)
To: Manaf Meethalavalappu Pallikunhi
Cc: linux-pm, linux-kernel, gregkh, Subbaraman Narayanamurthy
[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]
Hi,
On Wed, Jun 01, 2022 at 12:00:53AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
> Registering the cooling device from the probe can result in the
> execution of get_property() function before it gets initialized.
>
> To avoid this, register the cooling device from a workqueue
> instead of registering in the probe.
>
> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
> ---
This removes error handling from the psy_register_cooler() call, so
it introduces a new potential problem. If power_supply_get_property()
is called to early -EAGAIN is returned. So can you elaborate the problem
that you are seeing with the current code?
-- Sebastian
> drivers/power/supply/power_supply_core.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 385814a14a0a..74623c4977db 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -132,6 +132,7 @@ void power_supply_changed(struct power_supply *psy)
> }
> EXPORT_SYMBOL_GPL(power_supply_changed);
>
> +static int psy_register_cooler(struct power_supply *psy);
> /*
> * Notify that power supply was registered after parent finished the probing.
> *
> @@ -139,6 +140,8 @@ EXPORT_SYMBOL_GPL(power_supply_changed);
> * calling power_supply_changed() directly from power_supply_register()
> * would lead to execution of get_property() function provided by the driver
> * too early - before the probe ends.
> + * Also, registering cooling device from the probe will execute the
> + * get_property() function. So register the cooling device after the probe.
> *
> * Avoid that by waiting on parent's mutex.
> */
> @@ -156,6 +159,7 @@ static void power_supply_deferred_register_work(struct work_struct *work)
> }
>
> power_supply_changed(psy);
> + psy_register_cooler(psy);
>
> if (psy->dev.parent)
> mutex_unlock(&psy->dev.parent->mutex);
> @@ -1261,10 +1265,6 @@ __power_supply_register(struct device *parent,
> if (rc)
> goto register_thermal_failed;
>
> - rc = psy_register_cooler(psy);
> - if (rc)
> - goto register_cooler_failed;
> -
> rc = power_supply_create_triggers(psy);
> if (rc)
> goto create_triggers_failed;
> @@ -1294,8 +1294,6 @@ __power_supply_register(struct device *parent,
> add_hwmon_sysfs_failed:
> power_supply_remove_triggers(psy);
> create_triggers_failed:
> - psy_unregister_cooler(psy);
> -register_cooler_failed:
> psy_unregister_thermal(psy);
> register_thermal_failed:
> device_del(dev);
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND 2/2] power_supply: Use of-thermal cdev registration API
2022-05-31 18:30 ` [RESEND 2/2] power_supply: Use of-thermal cdev registration API Manaf Meethalavalappu Pallikunhi
@ 2022-06-09 22:17 ` Sebastian Reichel
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2022-06-09 22:17 UTC (permalink / raw)
To: Manaf Meethalavalappu Pallikunhi
Cc: linux-pm, linux-kernel, gregkh, Subbaraman Narayanamurthy
[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]
Hi,
On Wed, Jun 01, 2022 at 12:00:54AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
> With thermal frameworks of-thermal interface, thermal zone parameters can
> be defined in devicetree. This includes cooling device mitigation levels
> for a thermal zone. To take advantage of this, cooling device should use
> the thermal_of_cooling_device_register API to register a cooling device.
>
> Use thermal_of_cooling_device_register API to register the power supply
> cooling device. This enables power supply cooling device be included in the
> thermal zone parameter in devicetree.
>
> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
> ---
>
> drivers/power/supply/power_supply_core.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 74623c4977db..4593450920a4 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1157,9 +1157,15 @@ static int psy_register_cooler(struct power_supply *psy)
> for (i = 0; i < psy->desc->num_properties; i++) {
> if (psy->desc->properties[i] ==
> POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT) {
> - psy->tcd = thermal_cooling_device_register(
> - (char *)psy->desc->name,
> - psy, &psy_tcd_ops);
> + if (psy->dev.parent)
> + psy->tcd = thermal_of_cooling_device_register(
> + dev_of_node(psy->dev.parent),
> + (char *)psy->desc->name,
> + psy, &psy_tcd_ops);
if (psy->of_node)
psy->tcd = thermal_of_cooling_device_register(psy->of_node, ...);
else
psy->tcd = thermal_cooling_device_register(...);
--- Sebastian
> + else
> + psy->tcd = thermal_cooling_device_register(
> + (char *)psy->desc->name,
> + psy, &psy_tcd_ops);
> return PTR_ERR_OR_ZERO(psy->tcd);
> }
> }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND 1/2] power_supply: Register cooling device outside of probe
2022-06-09 22:12 ` [RESEND 1/2] power_supply: Register cooling device outside of probe Sebastian Reichel
@ 2023-02-27 21:46 ` Subbaraman Narayanamurthy
2023-02-28 0:23 ` Sebastian Reichel
0 siblings, 1 reply; 6+ messages in thread
From: Subbaraman Narayanamurthy @ 2023-02-27 21:46 UTC (permalink / raw)
To: Sebastian Reichel, Manaf Meethalavalappu Pallikunhi
Cc: linux-pm, linux-kernel, gregkh
On 6/9/22 3:12 PM, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Jun 01, 2022 at 12:00:53AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
>> Registering the cooling device from the probe can result in the
>> execution of get_property() function before it gets initialized.
>>
>> To avoid this, register the cooling device from a workqueue
>> instead of registering in the probe.
>>
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
>> ---
> This removes error handling from the psy_register_cooler() call, so
> it introduces a new potential problem. If power_supply_get_property()
> is called to early -EAGAIN is returned. So can you elaborate the problem
> that you are seeing with the current code?
>
> -- Sebastian
When the device boots up with all the vendor modules getting loaded,
here is what we're seeing when booting up with 6.1.11 recently. First
log is printed with adding a pr_err() in __power_supply_register().
[ 7.008938][ T682] power_supply battery: psy_register_cooler failed, rc=-11
[ 7.030941][ T682] qti_battery_charger: probe of qcom,battery_charger failed with error -11
Here, our downstream qti_battery_charger driver exposes the following
power supply properties POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT and
POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX under a power supply device.
This is happening because of the following call sequence,
battery_chg_probe() ->
power_supply_register() ->
psy_register_cooler() ->
thermal_cooling_device_register() ->
cdev->ops->get_max_state() ->
ps_get_max_charge_cntl_limit() ->
power_supply_get_property()
ends up calling power_supply_get_property() to read CHARGE_CONTROL_LIMIT
property.
However, it returns -EAGAIN because psy->initialized is set to true
later after psy_register_cooler() succeeds. So, this ends up in a
driver probe failure forever.
-Subbaraman
>
>> drivers/power/supply/power_supply_core.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 385814a14a0a..74623c4977db 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -132,6 +132,7 @@ void power_supply_changed(struct power_supply *psy)
>> }
>> EXPORT_SYMBOL_GPL(power_supply_changed);
>>
>> +static int psy_register_cooler(struct power_supply *psy);
>> /*
>> * Notify that power supply was registered after parent finished the probing.
>> *
>> @@ -139,6 +140,8 @@ EXPORT_SYMBOL_GPL(power_supply_changed);
>> * calling power_supply_changed() directly from power_supply_register()
>> * would lead to execution of get_property() function provided by the driver
>> * too early - before the probe ends.
>> + * Also, registering cooling device from the probe will execute the
>> + * get_property() function. So register the cooling device after the probe.
>> *
>> * Avoid that by waiting on parent's mutex.
>> */
>> @@ -156,6 +159,7 @@ static void power_supply_deferred_register_work(struct work_struct *work)
>> }
>>
>> power_supply_changed(psy);
>> + psy_register_cooler(psy);
>>
>> if (psy->dev.parent)
>> mutex_unlock(&psy->dev.parent->mutex);
>> @@ -1261,10 +1265,6 @@ __power_supply_register(struct device *parent,
>> if (rc)
>> goto register_thermal_failed;
>>
>> - rc = psy_register_cooler(psy);
>> - if (rc)
>> - goto register_cooler_failed;
>> -
>> rc = power_supply_create_triggers(psy);
>> if (rc)
>> goto create_triggers_failed;
>> @@ -1294,8 +1294,6 @@ __power_supply_register(struct device *parent,
>> add_hwmon_sysfs_failed:
>> power_supply_remove_triggers(psy);
>> create_triggers_failed:
>> - psy_unregister_cooler(psy);
>> -register_cooler_failed:
>> psy_unregister_thermal(psy);
>> register_thermal_failed:
>> device_del(dev);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND 1/2] power_supply: Register cooling device outside of probe
2023-02-27 21:46 ` Subbaraman Narayanamurthy
@ 2023-02-28 0:23 ` Sebastian Reichel
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2023-02-28 0:23 UTC (permalink / raw)
To: Subbaraman Narayanamurthy
Cc: Manaf Meethalavalappu Pallikunhi, linux-pm, linux-kernel, gregkh
[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]
Hi,
On Mon, Feb 27, 2023 at 01:46:52PM -0800, Subbaraman Narayanamurthy wrote:
> On 6/9/22 3:12 PM, Sebastian Reichel wrote:
> > Hi,
> >
> > On Wed, Jun 01, 2022 at 12:00:53AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
> >> Registering the cooling device from the probe can result in the
> >> execution of get_property() function before it gets initialized.
> >>
> >> To avoid this, register the cooling device from a workqueue
> >> instead of registering in the probe.
> >>
> >> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
> >> ---
> > This removes error handling from the psy_register_cooler() call, so
> > it introduces a new potential problem. If power_supply_get_property()
> > is called to early -EAGAIN is returned. So can you elaborate the problem
> > that you are seeing with the current code?
> >
> > -- Sebastian
>
> When the device boots up with all the vendor modules getting loaded,
> here is what we're seeing when booting up with 6.1.11 recently. First
> log is printed with adding a pr_err() in __power_supply_register().
>
> [ 7.008938][ T682] power_supply battery: psy_register_cooler failed, rc=-11
> [ 7.030941][ T682] qti_battery_charger: probe of qcom,battery_charger failed with error -11
>
> Here, our downstream qti_battery_charger driver exposes the following
> power supply properties POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT and
> POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX under a power supply device.
>
> This is happening because of the following call sequence,
>
> battery_chg_probe() ->
> power_supply_register() ->
> psy_register_cooler() ->
> thermal_cooling_device_register() ->
> cdev->ops->get_max_state() ->
> ps_get_max_charge_cntl_limit() ->
> power_supply_get_property()
>
> ends up calling power_supply_get_property() to read CHARGE_CONTROL_LIMIT
> property.
>
> However, it returns -EAGAIN because psy->initialized is set to true
> later after psy_register_cooler() succeeds. So, this ends up in a
> driver probe failure forever.
This should be solved in 6.3:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply/power_supply_core.c?id=c85c191694cb1cf290b11059b3d2de8a2732ffd0
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-28 0:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 18:30 [RESEND 1/2] power_supply: Register cooling device outside of probe Manaf Meethalavalappu Pallikunhi
2022-05-31 18:30 ` [RESEND 2/2] power_supply: Use of-thermal cdev registration API Manaf Meethalavalappu Pallikunhi
2022-06-09 22:17 ` Sebastian Reichel
2022-06-09 22:12 ` [RESEND 1/2] power_supply: Register cooling device outside of probe Sebastian Reichel
2023-02-27 21:46 ` Subbaraman Narayanamurthy
2023-02-28 0:23 ` Sebastian Reichel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).