Messages in this thread | | | Subject | Re: [RESEND 1/2] power_supply: Register cooling device outside of probe | From | Subbaraman Narayanamurthy <> | Date | Mon, 27 Feb 2023 13:46:52 -0800 |
| |
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);
| |