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

[Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>]

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
>