Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver

[Krzysztof Kozlowski]

On 05/27/2016 10:37 AM, Krzysztof Kozlowski wrote:
>> And you might be completely correct, that is something that can only
>> happen specifically with the bq27xxx driver. In which case, making the
>> fix there should be the fix. I just know from the commit log (and some
>> previous work with power supply drivers) that the case of get_property
>> being called during registration has caused problems before. That's why
>> I am trying to make sure we cover the generic case if it exists. Using
>> scheduled work is common for power_supplies to regularly update their
>> status.
>>
>> As for your proposed patches for bq27xxx, I think the latest one you
>> suggested (@12:36PM EST) with the change for
>> battery_update->battery_poll as well makes the most sense for bq27xxx. I
>> would like to point out though that if we patch this, the cache won't be
>> populated for the first TEMP request, which has the same end effect as
>> the patch I proposed to power_supply_read_temp. I believe both will
>> return 0 for the temp.
>>
>> I think that patch would work just fine in place of what I suggested for
>> this specific crash.
> 
> Hello all,
> 
> Indeed I was struggling with similar issue in bq27x00_battery. The issue
> was introduced by... me :(  when moving the ownership of power supply
> structure from driver to the core. However IMHO my change exposed the
> fundamental problem with power supply.
> 
> Anyway a fix for this issue was:
> 7f1a57fdd6cb6e7b (power_supply: Fix possible NULL pointer dereference on
> early uevent)
> AFAIU, this fix no longer fixes all the issues, right?
> 
> As for the fundamental problem, the power supply core should not call
> back the driver (get_property()) until the probe ends. Even if the
> di->bat was initialized, some other fields of driver could not be set
> yet. In general, the probe did not end so we should avoid calling driver
> internal functions.
> 
> In this particular problem:
> 1. Fix for the driver (!di->bat) is okay... but it won't solve the
> problem in general.
> 2. I think the core should handle it somehow...

I was thinking about some more generic solutions for that. Few ideas:
1. Split the power_supply_register() into register + manual call to
power_supply_changed(). Each driver will have to call the
power_supply_changed() when it is ready to do it. After that call, it is
expected that driver provides everything for power supply (it can
receive callbacks).

2. Since 7f1a57fdd6cb ("power_supply: Fix possible NULL pointer
dereference on early uevent") the power_supply_changed() is called from
a deferred work. Separate thread. We can introduce (in the core only) a
mutex:
	power_supply_deferred_register_work()
	{
		psy->mutex_lock();
		power_supply_changed(psy);
		psy->mutex_unlock();
	}
and add it also to all of API:
	power_supply_get_property() {
		psy->mutex_lock();
		psy->get_property();
		psy->mutex_unlock();
	}
The changes would be limited only to the core but we will introduce
strict locking over all of the psy callbacks.

3. We can go back to previous API, leaving the allocation done by the core:
	some_drv_probe() {
		err = power_supply_register(&some_drv->psy...);
	}


I think the second solution seems to be the most self-contained and robust.

Best regards,
Krzysztof