Received-SPF: pass (google.com: domain of linux-kernel+bounces-68891-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 16 Feb 2024 17:36:37 +0200
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Leonardo Bras <leobras@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Tony Lindgren <tony@atomide.com>,
	John Ogness <john.ogness@linutronix.de>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de>,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Shanker Donthineni <sdonthineni@nvidia.com>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [RFC PATCH v2 2/4] irq/spurious: Account for multiple handles in
 note_interrupt
Message-ID: <Zc-BBQGauwIEJJXy@smile.fi.intel.com>
References: <20240216075948.131372-2-leobras@redhat.com>
 <20240216075948.131372-4-leobras@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240216075948.131372-4-leobras@redhat.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Fri, Feb 16, 2024 at 04:59:44AM -0300, Leonardo Bras wrote:
> Currently note_interrupt() will check threads_handled for changes and use
> it to mark an IRQ as handled, in order to avoid having a threaded
> interrupt to falsely trigger unhandled IRQ detection.
> 
> This detection can still be falsely triggered if we have many IRQ handled
> accounted between each check of threads_handled, as those will be counted
> as a single one in the unhandled IRQ detection.
> 
> In order to fix this, subtract from irqs_unhandled the number of IRQs
> handled since the last check (threads_handled_last - threads_handled).

..

> +static inline int get_handled_diff(struct irq_desc *desc)
> +{
> +	unsigned int handled;
> +	int diff;
> +
> +	handled = (unsigned int)atomic_read(&desc->threads_handled);
> +	handled |= SPURIOUS_DEFERRED;
> +
> +	diff = handled - desc->threads_handled_last;
> +	diff >>= SPURIOUS_DEFERRED_SHIFT;
> +
> +	/*
> +	 * Note: We keep the SPURIOUS_DEFERRED bit set. We are handling the
> +	 * previous invocation right now. Keep it for the current one, so the
> +	 * next hardware interrupt will account for it.
> +	 */

> +	if (diff != 0)

	if (diff)

> +		desc->threads_handled_last = handled;
> +
> +	return diff;
> +}

..

> +			diff = get_handled_diff(desc);
> +			if (diff > 0) {

diff may not be negative as you always right shift by 1 (or more) bit. Hence

			if (diff)

will suffice (also be aligned with the similar check inside the helper) and
making the helper to return unsigned value will be clearer. Am I correct?

-- 
With Best Regards,
Andy Shevchenko