Re: [PATCH v3 11/11] hwmon: chipcap2: synchronize ready IRQ state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwmon: chipcap2: spurious completion due to threaded IRQ scheduling delay
- [Medium] hwmon: chipcap2: spurious alarm IRQs after regulator enable
--

commit 14087cc4e23152ff414e4188a7fe5a671a791a15
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: synchronize ready IRQ state

The ready IRQ handler can race with reinit_completion() in the enable
path. In particular, a handler from an earlier operation can complete
the completion after it is reset for a new operation.

Use irq_lock to serialize access to the completion and process_irqs.
Disable IRQ processing and synchronize any running handler before resetting
the completion for a new operation.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index b3b725cc7224f..00f62a33d4637 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -123,6 +123,16 @@ static int cc2_enable(struct cc2_data *data)
>  {
>  	int ret;
>  
> +	if (data->irq_ready > 0) {
> +		scoped_guard(mutex, &data->irq_lock)
> +			data->process_irqs = false;
> +
> +		synchronize_irq(data->irq_ready);
> +
> +		scoped_guard(mutex, &data->irq_lock)
> +			reinit_completion(&data->complete);
> +	}
> +
>  	/* exclusive regulator, check in case a disable failed */
>  	if (data->regulator_enabled) {
>  		scoped_guard(mutex, &data->irq_lock)
> @@ -130,9 +140,6 @@ static int cc2_enable(struct cc2_data *data)
>  		return 0;
>  	}
>  
> -	/* clear any pending completion */
> -	try_wait_for_completion(&data->complete);
> -
>  	ret = regulator_enable(data->regulator);
>  	if (ret < 0)
>  		return ret;

[Severity: Medium]
If a power-up glitch from regulator_enable() triggers an IRQ, could the
threaded IRQ handlers be delayed by scheduling and run after process_irqs is
set to true further down in this function?

Looking at the rest of cc2_enable():

	data->regulator_enabled = true;

	usleep_range(CC2_STARTUP_TIME_US, CC2_STARTUP_TIME_US + 125);

	scoped_guard(mutex, &data->irq_lock)
		data->process_irqs = true;

Since there is no primary hardirq handler, the IRQ subsystem immediately
queues the threaded handler. If it is delayed by more than ~1.25ms, wouldn't
it observe process_irqs as true and prematurely unblock the measurement wait
by calling complete(&cc2->complete) in cc2_ready_interrupt()?

[Severity: Medium]
Similarly, is it possible for the threaded alarm handlers (irq_low, irq_high)
triggered by regulator toggling to execute after process_irqs is set to true
here?

If they are delayed and observe process_irqs as true, wouldn't they
incorrectly fire an alarm by unconditionally calling hwmon_notify_event() in
cc2_low_interrupt() and cc2_high_interrupt()?

Should there be a synchronize_irq() for the alarm and ready IRQs before
enabling process_irqs?

[ ... ]

> @@ -510,8 +520,10 @@ static irqreturn_t cc2_ready_interrupt(int irq, void *data)
>  {
>  	struct cc2_data *cc2 = data;
>  
> -	if (cc2->process_irqs)
> -		complete(&cc2->complete);
> +	scoped_guard(mutex, &cc2->irq_lock) {
> +		if (cc2->process_irqs)
> +			complete(&cc2->complete);
> +	}
>  
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Since this check happens in thread context rather than hardirq context, could
a delayed thread incorrectly observe the true state here and prematurely
unblock the measurement wait?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=11