| 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
| ||
|
Date: Sun, 07 May 2017 19:37:41 +0200
From: Paul Kocialkowski <contact@...lk.fr>
To: Pali Rohár <pali.rohar@...il.com>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
"Andrew F . Davis" <afd@...com>,
Sebastian Reichel <sre@...nel.org>,
Chris Lapa <chris@...a.com.au>,
Matt Ranostay <mranostay@...il.com>
Subject: Re: [PATCH 4/5] power: supply: bq27xxx: Look for status change on
external power change
Hi,
Le vendredi 05 mai 2017 à 10:04 +0200, Pali Rohár a écrit :
> On Sunday 30 April 2017 20:27:26 Paul Kocialkowski wrote:
> > This introduces a dedicated status change work to look for power
> > status change. It is triggered by external power change notifications
> > and periodically retries detecting a power status change for 5 seconds.
> >
> > This is largely inspired by a similar mechanism from the sbs-battery
> > driver.
>
> What is reason/motivation for such change? It should be written in
> commit message (to help understand; not only for me), because really I
> do not know why such patch is needed.
Well, I really don't understand how things can work out without this patch.
How exactly are external power notifications supposed to be handled with the
current state of the driver?
From my tests, changes on external status change notifications were unreliable
as they did not look for a status change at all and instead scheduled a poll
work, which would not at any point call power_supply_changed, but instead update
the status for later reads.
This is *not* the expected behavior. When an external power supply is
connected/disconnected, the driver should detect the status change and report it
as soon as detected. This is precisely what this patch does (as is done already
with the sbs battery driver).
Note that it is up to *the driver* to report that status change, it must not
wait for userspace to query whether something changed.
Does that clarify why this is needed?
> > Signed-off-by: Paul Kocialkowski <contact@...lk.fr>
> > ---
> > drivers/power/supply/bq27xxx_battery.c | 38
> > ++++++++++++++++++++++++++++++++--
> > include/linux/power/bq27xxx_battery.h | 3 +++
> > 2 files changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/supply/bq27xxx_battery.c
> > b/drivers/power/supply/bq27xxx_battery.c
> > index 926bd58344d9..cade00df6162 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1190,6 +1190,11 @@ static int bq27xxx_battery_status(struct
> > bq27xxx_device_info *di,
> > status = POWER_SUPPLY_STATUS_CHARGING;
> > }
> >
> > + if (di->status_retry == 0 && di->status_change_reference != status)
> > {
> > + di->status_change_reference = status;
> > + power_supply_changed(di->bat);
> > + }
> > +
> > val->intval = status;
> >
> > return 0;
> > @@ -1340,12 +1345,38 @@ static int bq27xxx_battery_get_property(struct
> > power_supply *psy,
> > return ret;
> > }
> >
> > +static void bq27xxx_status_change(struct work_struct *work)
> > +{
> > + struct bq27xxx_device_info *di =
> > + container_of(work, struct bq27xxx_device_info,
> > + status_work.work);
> > + union power_supply_propval val;
> > + int ret;
> > +
> > + bq27xxx_battery_update(di);
> > +
> > + ret = bq27xxx_battery_status(di, &val);
> > + if (ret < 0)
> > + return;
> > +
> > + if (di->status_change_reference != val.intval) {
> > + di->status_change_reference = val.intval;
> > + power_supply_changed(di->bat);
> > + }
> > +
> > + if (di->status_retry > 0) {
> > + di->status_retry--;
> > + schedule_delayed_work(&di->status_work, HZ);
> > + }
> > +}
> > +
> > static void bq27xxx_external_power_changed(struct power_supply *psy)
> > {
> > struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> >
> > - cancel_delayed_work_sync(&di->poll_work);
> > - schedule_delayed_work(&di->poll_work, 0);
> > + cancel_delayed_work_sync(&di->status_work);
> > + schedule_delayed_work(&di->status_work, HZ);
> > + di->status_retry = 5;
> > }
> >
> > int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
> > @@ -1357,8 +1388,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info
> > *di)
> > psy_cfg.of_node = di->of_node;
> >
> > INIT_DELAYED_WORK(&di->poll_work, bq27xxx_battery_poll);
> > + INIT_DELAYED_WORK(&di->status_work, bq27xxx_status_change);
> > mutex_init(&di->lock);
> > di->regs = bq27xxx_regs[di->chip];
> > + di->status_change_reference = POWER_SUPPLY_STATUS_UNKNOWN;
> >
> > psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
> > if (!psy_desc)
> > @@ -1400,6 +1433,7 @@ void bq27xxx_battery_teardown(struct
> > bq27xxx_device_info *di)
> > poll_interval = 0;
> >
> > cancel_delayed_work_sync(&di->poll_work);
> > + cancel_delayed_work_sync(&di->status_work);
> >
> > power_supply_unregister(di->bat);
> >
> > diff --git a/include/linux/power/bq27xxx_battery.h
> > b/include/linux/power/bq27xxx_battery.h
> > index 0a9af513165a..16d604681ace 100644
> > --- a/include/linux/power/bq27xxx_battery.h
> > +++ b/include/linux/power/bq27xxx_battery.h
> > @@ -67,6 +67,9 @@ struct bq27xxx_device_info {
> > int charge_design_full;
> > unsigned long last_update;
> > struct delayed_work poll_work;
> > + struct delayed_work status_work;
> > + int status_retry;
> > + int status_change_reference;
> > struct power_supply *bat;
> > struct list_head list;
> > struct mutex lock;
>
>
--
Paul Kocialkowski, developer of free digital technology and hardware support
Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists