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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57483AC8.6010007@nvidia.com>
Date:	Fri, 27 May 2016 13:17:12 +0100
From:	Jon Hunter <jonathanh@...dia.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Rhyland Klein <rklein@...dia.com>,
	Thierry Reding <treding@...dia.com>,
	Sebastian Reichel <sre@...nel.org>,
	David Woodhouse <dwmw2@...radead.org>,
	"Dmitry Eremin-Solenikov" <dbaryshkov@...il.com>
CC:	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	<linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver


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