| 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: Fri, 03 Jul 2015 09:03:53 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Anda-Maria Nicolae <anda-maria.nicolae@...el.com>
Cc: sre@...nel.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH] power_supply: rt9455_charger: Properly notify userspace
about charging events
On 02.07.2015 16:40, Anda-Maria Nicolae wrote:
> Hello Krzysztof,
>
> The information provided to userspace is not accurate in current driver
> implementation and it is accurate with this patch.
> Also, when the battery is reconnected to the charger (after it has been
> disconnected from the charger), the charger does not trigger any
> interrupt. Since power_supply_changed() is called only in interrupt
> handler, the userspace is never notified that the battery is reconnected.
>
> Let me explain to you why power_supply_changed() is called in
> rt9455_pwr_rdy_work_callback() and not in interrupt handler, when CHRVPI
> interrupt occurs.
>
> I created a userspace tool that waits for notifications from the driver.
> A notification is sent when the driver calls power_supply_changed().
> When my userspace tool receives a notification, it reads the files from
> /sys/class/power_supply/rt9455-charger/ folder. The tool reads each file
> this to figure it out what has been changed, since the driver notifies
> the tool only when a change has been made.
> These files include /sys/class/power_supply/rt9455-charger/online. This
> file displays 1 if the power source is connected to the charger and 0
> otherwise.
> When the userspace tool reads
> /sys/class/power_supply/rt9455-charger/online,
> rt9455_charger_get_online() function from the driver is called.
> rt9455_charger_get_online() returns the value of PWR_RDY bit.
> But whenever the power source is connected to / disconnected from the
> charger, RT9455 first sets CHRVPI interrupt bit, triggers an interrupt,
> and after almost 1-2 seconds, the charger also updates PWR_RDY bit.
> So, if we check PWR_RDY bit immediately after CHRVPI bit is set, we will
> not obtain accurate info about the change that has been made (i.e.: if
> the power source was connected or disconnected).
> This is why the driver has struct delayed_work pwr_rdy_work, which is
> scheduled whenever CHRVPI interrupt bit is set.
> This is why in this patch, in rt9455_pwr_rdy_work_callback(),
> power_supply_changed() is called, and not in interrupt handler.
That explains it, thank you. It would be nice if such answer to "why"
would be also in commit message (maybe a shorter one).
Beside of that the patch looks good:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
Best regards,
Krzysztof
>
> Let me explain to you why power_supply_changed() is called in
> rt9455_batt_presence_work_callback().
>
> As previously stated, the userspace tool is never notified that the
> battery is reconnected. But since the driver uses struct delayed_work
> batt_presence_work, to determine whether the battery has been
> reconnected, we can call power_supply_changed() whenever the driver
> determines the battery is reconnected (i.e. BATAB interrupt bit is
> cleared).
> This is why in this patch, in rt9455_batt_presence_work_callback(),
> power_supply_changed() is called.
>
> On 07/02/2015 03:49 AM, Krzysztof Kozlowski wrote:
>> 2015-06-18 1:28 GMT+09:00 Anda-Maria Nicolae
>> <anda-maria.nicolae@...el.com>:
>>> Do not call power_supply_changed() when CHRVPI interrupt has occurred.
>> What I cannot find here is the answer to: why? I am not familiar with
>> the driver so this is just guessing that you wanted to notify
>> userspace about connected/disconnected charger. In both cases (before
>> and after the patch) this is achieved, isn't it?
>>
>> Best regards,
>> Krzysztof
>>
>>
>>> CHRVPI interrupt occurs when the charger is connected to or disconnected
>>> from the power source. Call power_supply_changed() after PWR_RDY bit is
>>> read, to distinguish between connection to or disconnection from the
>>> power
>>> source.
>>> Also, call power_supply_changed() after the battery is reconnected to
>>> the
>>> charger, to notify userspace that the battery is no longer absent.
>>>
>>> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@...el.com>
>>> ---
>>> drivers/power/rt9455_charger.c | 16 ++++++++++++++--
>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/power/rt9455_charger.c
>>> b/drivers/power/rt9455_charger.c
>>> index 08baac6..a49a9d4 100644
>>> --- a/drivers/power/rt9455_charger.c
>>> +++ b/drivers/power/rt9455_charger.c
>>> @@ -973,7 +973,6 @@ static int
>>> rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>>>
>>> if (irq2 & GET_MASK(F_CHRVPI)) {
>>> dev_dbg(dev, "Charger fault occurred\n");
>>> - alert_userspace = true;
>>> /*
>>> * CHRVPI bit is set in 2 cases:
>>> * 1. when the power source is connected to the
>>> charger.
>>> @@ -981,6 +980,9 @@ static int
>>> rt9455_irq_handler_check_irq2_register(struct rt9455_info *info,
>>> * To identify the case, PWR_RDY bit is checked.
>>> Because
>>> * PWR_RDY bit is set / cleared after CHRVPI
>>> interrupt is
>>> * triggered, it is used delayed_work to later read
>>> PWR_RDY bit.
>>> + * Also, do not set to true alert_userspace, because
>>> there is no
>>> + * need to notify userspace when CHRVPI interrupt has
>>> occurred.
>>> + * Userspace will be notified after PWR_RDY bit is read.
>>> */
>>> queue_delayed_work(system_power_efficient_wq,
>>> &info->pwr_rdy_work,
>>> @@ -1178,7 +1180,7 @@ static irqreturn_t
>>> rt9455_irq_handler_thread(int irq, void *data)
>>> /*
>>> * Sometimes, an interrupt occurs while
>>> rt9455_probe() function
>>> * is executing and power_supply_register() is not
>>> yet called.
>>> - * Do not call power_supply_charged() in this case.
>>> + * Do not call power_supply_changed() in this case.
>>> */
>>> if (info->charger)
>>> power_supply_changed(info->charger);
>>> @@ -1478,6 +1480,11 @@ static void
>>> rt9455_pwr_rdy_work_callback(struct work_struct *work)
>>> RT9455_MAX_CHARGING_TIME * HZ);
>>> break;
>>> }
>>> + /*
>>> + * Notify userspace that the charger has been either
>>> connected to or
>>> + * disconnected from the power source.
>>> + */
>>> + power_supply_changed(info->charger);
>>> }
>>>
>>> static void rt9455_max_charging_time_work_callback(struct
>>> work_struct *work)
>>> @@ -1533,6 +1540,11 @@ static void
>>> rt9455_batt_presence_work_callback(struct work_struct *work)
>>> if (ret)
>>> dev_err(dev, "Failed to unmask BATAB
>>> interrupt\n");
>>> }
>>> + /*
>>> + * Notify userspace that the battery is now connected
>>> to the
>>> + * charger.
>>> + */
>>> + power_supply_changed(info->charger);
>>> }
>>> }
>>>
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists