Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC] power: supply: bq27xxx_battery: Fix polling interval after
 re-bind
To:     Krzysztof Kozlowski <krzk@kernel.org>,
        =?UTF-8?Q?Pali_Roh=c3=a1r?= <pali@kernel.org>,
        Sebastian Reichel <sre@kernel.org>,
        Anton Vorontsov <cbouatmailru@gmail.com>,
        <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC:     <stable@vger.kernel.org>
References: <20200525113220.369-1-krzk@kernel.org>
From:   "Andrew F. Davis" <afd@ti.com>
Message-ID: <65ccf383-85a3-3ccd-f38c-e92ddae8fe1e@ti.com>
Date:   Tue, 26 May 2020 21:16:28 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200525113220.369-1-krzk@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 5/25/20 7:32 AM, Krzysztof Kozlowski wrote:
> This reverts commit 8cfaaa811894a3ae2d7360a15a6cfccff3ebc7db.
> 
> If device was unbound and bound, the polling interval would be set to 0.
> This is both unexpected and messes up with other bq27xxx devices (if
> more than one battery device is used).
> 
> This reset of polling interval was added in commit 8cfaaa811894
> ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00 driver")
> stating that power_supply_unregister() calls get_property().  However in
> Linux kernel v3.1 and newer, such call trace does not exist.
> Unregistering power supply does not call get_property() on unregistered
> power supply.
> 
> Fixes: 8cfaaa811894 ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00 driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> ---
> 
> I really could not identify the issue being fixed in offending commit
> 8cfaaa811894 ("bq27x00_battery: Fix OOPS caused by unregistring bq27x00
> driver"), therefore maybe I missed here something important.
> 
> Please share your thoughts on this.


I'm having a hard time finding the OOPS also. Maybe there is a window
where the poll function is running or about to run where
cancel_delayed_work_sync() is called and cancels the work, only to have
an interrupt or late get_property call in to the poll function and
re-schedule it.

What we really need is to do is look at how we are handling the polling
function. It gets called from the workqueue, from a threaded interrupt
context, and from a power supply framework callback, possibly all at the
same time. Sometimes its protected by a lock, sometimes not. Updating
the device's cached data should always be locked.

What's more is the poll function is self-arming, so if we call
cancel_delayed_work_sync() (remove it from the work queue then then wait
for it to finish if running), are we sure it wont have just re-arm itself?

We should make the only way we call the poll function be through the
work queue, (plus make sure all accesses to the cache are locked).

Andrew


> ---
>  drivers/power/supply/bq27xxx_battery.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 942c92127b6d..4c94ee72de95 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1905,14 +1905,6 @@ EXPORT_SYMBOL_GPL(bq27xxx_battery_setup);
>  
>  void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
>  {
> -	/*
> -	 * power_supply_unregister call bq27xxx_battery_get_property which
> -	 * call bq27xxx_battery_poll.
> -	 * Make sure that bq27xxx_battery_poll will not call
> -	 * schedule_delayed_work again after unregister (which cause OOPS).
> -	 */
> -	poll_interval = 0;
> -
>  	cancel_delayed_work_sync(&di->work);
>  
>  	power_supply_unregister(di->bat);
>