Re: [PATCH 1/3] power: cpcap-battery: Do not issue low signal too frequently

[Ivaylo Dimitrov]

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@xxxxxxxxx>
> > ---
>
> 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