* [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 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-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 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).