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

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

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.

>> 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.

Cheers
Jon

-- 
nvpublic