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

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On 05/27/2016 02:17 PM, Jon Hunter wrote:
> 
> On 27/05/16 12:46, Krzysztof Kozlowski wrote:
>> On 05/27/2016 12:28 PM, Jon Hunter wrote:
>>> Hi Krzysztof,
>>>
>>> On 27/05/16 09:37, Krzysztof Kozlowski wrote:
>>>
>>> ...
>>>
>>>> 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.
>>>
>>> For my understanding, can you elaborate why the power-supply core should
>>> not call back to the drivers ->get_property() before the probe ends? I
>>> assume that registering the power-supply should be the last thing done
>>> in the probe and so the power-supply should be configured at that point.
>>
>> It is not only about power supply but other resources allocated by the
>> driver. If the power_supply_register() is a last call, then no problem.
>> But if not, then these resources won't be available.
>>
>> Actually I exaggerated a little bit as a fundamental problem as this is
>> quite common pattern. When driver provides something (like power supply)
>> then after registration it should be ready for calls coming from the
>> core or user space. It does not have to be power supply. It might be
>> exposing sysfs entries or file operations (exposed before calling
>> power_supply_register()).
> 
> Right, exactly when you register with the power-supply core the device
> better be ready so that handle any incoming calls.

Yes, the unusual thing here is that the device is called back directly
from the power_supply_register() call.

> 
>>> The problems with the bq27xxx seem to stem from the periodic update of
>>> the bq27xxx status and so it is not clear to me that this is a generic
>>> problem for all power-supply devices.
>>
>> Initially, the generic problem was that the core would call back the
>> driver from power_supply_register() in a synchronous way through
>> power_supply_changed(). The commit 7f1a57fdd6c changed it to an
>> asynchronous call. Here it looks like the same problem - the
>> power_supply_register() calls thermal which calls
>> thermal_zone_device_update() and we are back at the driver... before
>> finishing power_supply_register() call.
> 
> So I am still not convinced this is a generic problem but a problem with
> the bq27xxx. In fact, I think that commit 7f1a57fdd6c could be avoided
> if we did something like ...
> 
> http://marc.info/?l=linux-kernel&m=146425896332433&w=2
> 
> AFAICT in most cases, in ->get_property() you should have no need to
> access a driver's equivalent of di->bat, because you have already been
> passed a pointer to this via the *psy argument.

I agree that get_property() shouldn't access di->bat. However if it is
not forbidden (at least by documentation) then someone might just do it
because he does not know about such requirement.

Best regards,
Krzysztof