Hello Jonathan,
On Sun, Dec 08, 2024 at 12:42:05PM +0000, Jonathan Cameron wrote:
> On Fri, 6 Dec 2024 18:28:38 +0100
> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> wrote:
>
> > The ad_sigma_delta driver helper uses irq_disable_nosync(). With that
> > one it is possible that the irq handler still runs after the
> > irq_disable_nosync() function call returns. Also to properly synchronize
> > irq disabling in the different threads proper locking is needed and
> > because it's unclear if the irq handler's irq_disable_nosync() call
> > comes first or the one in the enabler's error path, all code locations
> > that disable the irq must check for .irq_dis first to ensure there is
> > exactly one disable call per enable call.
> >
> > So add a spinlock to the struct ad_sigma_delta and use it to synchronize
> > irq enabling and disabling. Also only act in the irq handler if the irq
> > is still enabled.
> >
> > Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Delta devices")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
>
> From sparse.
>
> drivers/iio/adc/ad_sigma_delta.c:205:13: warning: context imbalance in 'ad_sd_disable_irq' - wrong count at exit
> drivers/iio/adc/ad_sigma_delta.c:218:13: warning: context imbalance in 'ad_sd_enable_irq' - wrong count at exit
>
> I saw your discussion with Linus on this...
>
> https://lore.kernel.org/all/CAHk-=wiVDZejo_1BhOaR33qb=pny7sWnYtP4JUbRTXkXCkW6jA@xxxxxxxxxxxxxx/
>
> So I guess we just treat that as a false positive and move on.
Yes, my discussion was about a different driver, but it's the same here.
sparse is happy when applying the following patch:
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index d32102b25530..dea4816793fa 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -206,23 +206,33 @@ EXPORT_SYMBOL_NS_GPL(ad_sd_reset, IIO_AD_SIGMA_DELTA);
static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
{
- guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(&sigma_delta->irq_lock, flags);
/* It's already off, return false to indicate nothing was changed */
- if (sigma_delta->irq_dis)
+ if (sigma_delta->irq_dis) {
+ spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
return false;
+ }
sigma_delta->irq_dis = true;
disable_irq_nosync(sigma_delta->irq_line);
+ spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
+
return true;
}
static void ad_sd_enable_irq(struct ad_sigma_delta *sigma_delta)
{
- guard(spinlock_irqsave)(&sigma_delta->irq_lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(&sigma_delta->irq_lock, flags);
sigma_delta->irq_dis = false;
enable_irq(sigma_delta->irq_line);
+
+ spin_unlock_irqrestore(&sigma_delta->irq_lock, flags);
}
#define AD_SD_CLEAR_DATA_BUFLEN 9
which results in equivalent code. Also TTBOMK this can only be fixed by
teaching sparse about the cleanup stuff and an annotation doesn't help.
So yes, please move on (or fix sparse :-)
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
