From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755246AbcEaRZP (ORCPT ); Tue, 31 May 2016 13:25:15 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:19003 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751484AbcEaRZG (ORCPT ); Tue, 31 May 2016 13:25:06 -0400 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Tue, 31 May 2016 10:23:10 -0700 Subject: Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver To: Krzysztof Kozlowski , Rhyland Klein , Thierry Reding , Sebastian Reichel , David Woodhouse , Dmitry Eremin-Solenikov References: <1462290318-9074-1-git-send-email-rklein@nvidia.com> <5744609A.1000008@nvidia.com> <324dfe74-4fc0-d500-91ac-2a802562e92f@nvidia.com> <5745853B.1040304@nvidia.com> <57458693.3050700@nvidia.com> <20160525154618.GD13765@ulmo.ba.sec> <9411ff33-e375-8286-8690-fe7fcac1c14b@nvidia.com> <5745CE75.7010603@nvidia.com> <5745D2DD.6080300@nvidia.com> <1c6df907-ea1f-201b-a36e-8311c5b2b3b1@nvidia.com> <5745E031.7010406@nvidia.com> <5c03a025-f31d-fa18-b973-0b026ede9c5c@nvidia.com> <5748073F.1030704@samsung.com> <57482162.20306@nvidia.com> <5748339E.9080504@samsung.com> <57483AC8.6010007@nvidia.com> <574843D4.4080303@samsung.com> CC: Stephen Warren , Alexandre Courbot , , From: Jon Hunter Message-ID: <574DC8E7.3040707@nvidia.com> Date: Tue, 31 May 2016 18:24:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <574843D4.4080303@samsung.com> X-Originating-IP: [10.21.132.110] X-ClientProxiedBy: UKMAIL101.nvidia.com (10.26.138.13) To UKMAIL102.nvidia.com (10.26.138.15) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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