On Fri, Nov 22, 2024 at 1:34 PM 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 per enable.
"...exactly one disable call per one 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.
...
> +static bool ad_sd_disable_irq(struct ad_sigma_delta *sigma_delta)
> +{
> + guard(spinlock_irqsave)(&sigma_delta->irq_lock);
> +
> + if (!sigma_delta->irq_dis) {
Why not positive conditional?
if (->irq_dis)
return false;
...
return true;
> + sigma_delta->irq_dis = true;
> + disable_irq_nosync(sigma_delta->irq_line);
> + return true;
> + } else {
> + return false;
> + }
> +}
...
> /* private: */
Consider at some point marking the below members with __private.
> struct completion completion;
> + spinlock_t irq_lock; /* protects .irq_dis and irq en/disable state */
> bool irq_dis;
--
With Best Regards,
Andy Shevchenko
