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

[Jon Hunter]

On 27/05/16 13:55, Krzysztof Kozlowski wrote:
> 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.

In that case, shouldn't the driver should check that di->bat is valid
before anyone attempts to dereference it? However, if you and/or Rhyland
have a generic fix for preventing this, please go ahead and propose it.

Cheers
Jon

-- 
nvpublic