[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bd1d5ece-7665-a116-f37d-c21061030997@gmail.com>
Date: Thu, 10 Nov 2022 20:13:12 +0200
From: Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
tony@...mide.com, philipp@...s.xyz
Subject: Re: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too
frequently
Hi,
On 10.11.22 г. 17:55 ч., Sebastian Reichel wrote:
> Hi,
>
> On Sat, Nov 05, 2022 at 01:25:42PM +0200, Ivaylo Dimitrov wrote:
>> It seems that low battery irq may be generated tens of times per second,
>> leading to userspace being flooded with unnecessary events.
>>
>> Fix that by preventing such events being generated more than once every 30
>> seconds.
>>
>> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
>> ---
>
> Concept looks ok to me, but the code is slightly racy, since the
> thread is flushed before the IRQ is disabled in the remove routine.
>
I did that on purpose, to have matching disable_irq()/enable_irq()
calls. Maybe I over-engineered it, but my understanding is:
When remove() is called, if we have delayed work pending, that means
that lowbph IRQ was disabled in cpcap_battery_irq_thread() and we have
to re-enable it. If delayed_work is not pending, flush_delayed_work()
will do nothing, IIUC.
Maybe I shall protect schedule_delayed_work() and disable_irq_nosync()
calls with a mutex and wait for it before calling flush_delayed_work()
in remove? That way there will be guarantee that if delayed_work is
pending, IRQ is disabled too, while now maybe there is a small time
window remove() to be called between schedule_delayed_work() and
disable_irq_nosync().
>> drivers/power/supply/cpcap-battery.c | 27 ++++++++++++++++++++++++++-
>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
>> index 4676560..8869067 100644
>> --- a/drivers/power/supply/cpcap-battery.c
>> +++ b/drivers/power/supply/cpcap-battery.c
>> @@ -137,6 +137,7 @@ struct cpcap_battery_ddata {
>> struct power_supply *psy;
>> struct cpcap_battery_config config;
>> struct cpcap_battery_state_data state[CPCAP_BATTERY_STATE_NR];
>> + struct delayed_work low_irq_work;
>> u32 cc_lsb; /* μAms per LSB */
>> atomic_t active;
>> int charge_full;
>> @@ -914,9 +915,13 @@ static irqreturn_t cpcap_battery_irq_thread(int irq, void *data)
>> dev_info(ddata->dev, "Coulomb counter calibration done\n");
>> break;
>> case CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW:
>> - if (latest->current_ua >= 0)
>> + if (latest->current_ua >= 0 &&
>> + !delayed_work_pending((&ddata->low_irq_work))) {
>> dev_warn(ddata->dev, "Battery low at %imV!\n",
>> latest->voltage / 1000);
>> + schedule_delayed_work(&ddata->low_irq_work, 30 * HZ);
>> + disable_irq_nosync(d->irq);
>> + }
>> break;
>> case CPCAP_BATTERY_IRQ_ACTION_POWEROFF:
>> if (latest->current_ua >= 0 && latest->voltage <= 3200000) {
>> @@ -1087,6 +1092,21 @@ static int cpcap_battery_calibrate(struct cpcap_battery_ddata *ddata)
>> return error;
>> }
>>
>> +static void cpcap_battery_lowbph_enable(struct work_struct *work)
>> +{
>> + struct delayed_work *d_work = to_delayed_work(work);
>> + struct cpcap_battery_ddata *ddata = container_of(d_work,
>> + struct cpcap_battery_ddata, low_irq_work);
>> + struct cpcap_interrupt_desc *d;
>> +
>> + list_for_each_entry(d, &ddata->irq_list, node) {
>> + if (d->action == CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW)
>> + break;
>> + }
>> +
>> + enable_irq(d->irq);
>> +}
>> +
>> #ifdef CONFIG_OF
>> static const struct of_device_id cpcap_battery_id_table[] = {
>> {
>> @@ -1118,6 +1138,8 @@ static int cpcap_battery_probe(struct platform_device *pdev)
>> if (!ddata)
>> return -ENOMEM;
>>
>> + INIT_DELAYED_WORK(&ddata->low_irq_work, cpcap_battery_lowbph_enable);
>
> use devm_delayed_work_autocancel() and put it directly before
> cpcap_battery_init_interrupts().
>
>> cpcap_battery_detect_battery_type(ddata);
>>
>> INIT_LIST_HEAD(&ddata->irq_list);
>> @@ -1185,6 +1207,9 @@ static int cpcap_battery_remove(struct platform_device *pdev)
>> if (error)
>> dev_err(&pdev->dev, "could not disable: %i\n", error);
>>
>> + /* make sure to call enable_irq() if needed */
>> + flush_delayed_work(&ddata->low_irq_work);
>
> and this can be dropped afterwards.
>
Ok, but what will happen if we have lowbph IRQ already disabled?
Wouldn't that cause issues, because of unbalanced
disable_irq()/enable_irq() calls?
Thanks,
Ivo
>> +
>> return 0;
>> }
>
> Thanks,
>
> -- Sebastian
>
Powered by blists - more mailing lists