From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932596AbcE0JT4 (ORCPT ); Fri, 27 May 2016 05:19:56 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:34922 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932443AbcE0JTy (ORCPT ); Fri, 27 May 2016 05:19:54 -0400 X-AuditID: cbfec7f4-f796c6d000001486-6f-57481136ab45 Subject: Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver To: Rhyland Klein , Jon Hunter , 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> Cc: Stephen Warren , Alexandre Courbot , linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <57481134.1030404@samsung.com> Date: Fri, 27 May 2016 11:19:48 +0200 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: <5748073F.1030704@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpgkeLIzCtJLcpLzFFi42I5/e/4NV0zQY9wgzlvlSwmPXnPbDFx5WRm i3OvHrFYtMxaxGLx+oWhxeVdc9gsOr/MYrNYdnw1q8Xp3SUWrw62sVjc/s3nwO2xc9Zddo/N K7Q8Nq3qZPPobX7H5tG3ZRWjx+dNch4b54YGsEdx2aSk5mSWpRbp2yVwZTy6+pWxYIpUxY/P 51gaGDtFuxg5OSQETCT2v37PAmGLSVy4t56ti5GLQ0hgKaPEuXunWUESQgLPGCUuf9AHsYUF PCSedbSwgBSJCLxjlHjRfxKqaAqLRMMqK5AEs0A/o8TLX+sYQRJsAsYSm5cvYYNYISfR2z0J bB2vgJbE7t1d7CA2i4CqxK6VbWBxUYEIiVnbfzBB1AhK/Jh8DyzOKaAt8efEOeYuRg6gBXoS 9y9qgYSZBeQlNq95yzyBUXAWko5ZCFWzkFQtYGRexSiaWppcUJyUnmuoV5yYW1yal66XnJ+7 iRESL192MC4+ZnWIUYCDUYmHl8HZPVyINbGsuDL3EKMEB7OSCG8vk0e4EG9KYmVValF+fFFp TmrxIUZpDhYlcd65u96HCAmkJ5akZqemFqQWwWSZODilGhjVG+4tdDefsebOTf8zkzq2Zt84 vcM7+PHb2pryladf/dx60GxOUTVr9THTjXPjp+c0sH5Rzr4TLWybxcwf7NldsviEtu10dbb/ HkwPFx67tCv3AU+k9iuf1A9N59r1StPfPryVbMDSm+zwI7Uz8ozDs7iDiSf0d0gt2ffhSbxI ZP8auTzptGAlluKMREMt5qLiRABG8oEYkwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/27/2016 10:37 AM, Krzysztof Kozlowski wrote: >> And you might be completely correct, that is something that can only >> happen specifically with the bq27xxx driver. In which case, making the >> fix there should be the fix. I just know from the commit log (and some >> previous work with power supply drivers) that the case of get_property >> being called during registration has caused problems before. That's why >> I am trying to make sure we cover the generic case if it exists. Using >> scheduled work is common for power_supplies to regularly update their >> status. >> >> As for your proposed patches for bq27xxx, I think the latest one you >> suggested (@12:36PM EST) with the change for >> battery_update->battery_poll as well makes the most sense for bq27xxx. I >> would like to point out though that if we patch this, the cache won't be >> populated for the first TEMP request, which has the same end effect as >> the patch I proposed to power_supply_read_temp. I believe both will >> return 0 for the temp. >> >> I think that patch would work just fine in place of what I suggested for >> this specific crash. > > Hello all, > > 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. > > In this particular problem: > 1. Fix for the driver (!di->bat) is okay... but it won't solve the > problem in general. > 2. I think the core should handle it somehow... I was thinking about some more generic solutions for that. Few ideas: 1. Split the power_supply_register() into register + manual call to power_supply_changed(). Each driver will have to call the power_supply_changed() when it is ready to do it. After that call, it is expected that driver provides everything for power supply (it can receive callbacks). 2. Since 7f1a57fdd6cb ("power_supply: Fix possible NULL pointer dereference on early uevent") the power_supply_changed() is called from a deferred work. Separate thread. We can introduce (in the core only) a mutex: power_supply_deferred_register_work() { psy->mutex_lock(); power_supply_changed(psy); psy->mutex_unlock(); } and add it also to all of API: power_supply_get_property() { psy->mutex_lock(); psy->get_property(); psy->mutex_unlock(); } The changes would be limited only to the core but we will introduce strict locking over all of the psy callbacks. 3. We can go back to previous API, leaving the allocation done by the core: some_drv_probe() { err = power_supply_register(&some_drv->psy...); } I think the second solution seems to be the most self-contained and robust. Best regards, Krzysztof