| Message ID | 1667647544-12945-2-git-send-email-ivo.g.dimitrov.75@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] power: cpcap-battery: Do not issue low signal too frequently | expand |
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@gmail.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 >
Hi Ivaylo,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ivaylo-Dimitrov/power-supply-cpcap-battery-improvements/20221105-192758
base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link: https://lore.kernel.org/r/1667647544-12945-2-git-send-email-ivo.g.dimitrov.75%40gmail.com
patch subject: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently
config: m68k-randconfig-m041-20221110
compiler: m68k-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
smatch warnings:
drivers/power/supply/cpcap-battery.c:1084 cpcap_battery_lowbph_enable() warn: iterator used outside loop: 'd'
vim +/d +1084 drivers/power/supply/cpcap-battery.c
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1072 static void cpcap_battery_lowbph_enable(struct work_struct *work)
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1073 {
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1074 struct delayed_work *d_work = to_delayed_work(work);
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1075 struct cpcap_battery_ddata *ddata = container_of(d_work,
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1076 struct cpcap_battery_ddata, low_irq_work);
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1077 struct cpcap_interrupt_desc *d;
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1078
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1079 list_for_each_entry(d, &ddata->irq_list, node) {
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1080 if (d->action == CPCAP_BATTERY_IRQ_ACTION_BATTERY_LOW)
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1081 break;
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1082 }
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1083
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 @1084 enable_irq(d->irq);
If we exit the loop without hitting a break then "d" is not a valid
pointer and "enable_irq(d->irq);" will do something bad.
ce48d112324af4 Ivaylo Dimitrov 2022-11-05 1085 }
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); + 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); + return 0; }
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@gmail.com> --- drivers/power/supply/cpcap-battery.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)