Re: [RFC] power: supply: bq27xxx_battery: Fix polling interval after re-bind

[Krzysztof Kozlowski <krzk@kernel.org>]

On Fri, Jun 19, 2020 at 07:55:21PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, May 27, 2020 at 09:42:54AM +0200, Pali Rohár wrote:
> > On Tuesday 26 May 2020 21:16:28 Andrew F. Davis wrote:
> > > 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
> > 
> > I do not remember details too. It is long time ago.
> > 
> > CCing Ivaylo Dimitrov as he may remember something...
> 
> Applying this revert introduces at least a race condition when
> userspace reads sysfs files while kernel removes the driver.
> 
> So looking at the entrypoints for schedules:
> 
> bq27xxx_battery_i2c_probe:
>   Not relevant, probe is done when the battery is being removed.
> 
> poll_interval_param_set:
>   Can be avoided by unregistering from the list earlier. This
>   is the right thing to do considering the battery is added to
>   the list as last step in the probe routine, it should be removed
>   first during teardown.

Yes, good point.

> 
> bq27xxx_external_power_changed:
>   This can happen at any time while the power-supply device is
>   registered, because of the code in get_property.
> 
> bq27xxx_battery_poll:
>   This can happen at any time while the power-supply device is
>   registered.
> 
> As far as I can see the only thing in the delayed work needing
> the power-supply device is power_supply_changed(). If we add a
> check, that di->bat is not NULL, we should be able to reorder
> teardown like this:

Except power_supply structure there is the device state struct
bq27xxx_device_info 'di'. If bq27xxx_battery_poll() is called during the
unbind, it will access the 'di' which is being freed by devm-framework.
And just checking for di->bat is also not thread safe (can be
reordered).

I think there is no easy few-line fix for this.  Instead, the
workqueue scheduling should be guarded everywhere by device-instance
mutex (bq27xxx_device_info.lock).


> 
> 1. remove from list
> 2. unregister power-supply device and set to di->bat to NULL
> 3. cancel delayed work
> 4. destroy mutex
> 
> Also I agree with Andrew, that the locking looks fishy. I think
> the lock needs to be moved, so that the call to
> bq27xx_battery_update(di) in bq27xxx_battery_poll is protected.

Exactly.

Best regards,
Krzysztof