Re: [RESEND 1/2] power_supply: Register cooling device outside of probe

[Subbaraman Narayanamurthy <quic_subbaram@quicinc.com>]

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);