Re: [PATCH net-next] r8169: Use a raw_spinlock_t for the register locks.

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Mike Galbraith <efault@gmx.de>,
	Paolo Abeni <pabeni@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	nic_swsd@realtek.com
Subject: Re: [PATCH net-next] r8169: Use a raw_spinlock_t for the register locks.
Date: Mon, 22 May 2023 22:00:17 +0200	[thread overview]
Message-ID: <d48727e7-451a-39f4-5878-0ba957c92114@gmail.com> (raw)
In-Reply-To: <20230522134121.uxjax0F5@linutronix.de>

On 22.05.2023 15:41, Sebastian Andrzej Siewior wrote:
> The driver's interrupt service routine is requested with the
> IRQF_NO_THREAD if MSI is available. This means that the routine is
> invoked in hardirq context even on PREEMPT_RT. The routine itself is
> relatively short and schedules a worker, performs register access and
> schedules NAPI. On PREEMPT_RT, scheduling NAPI from hardirq results in
> waking ksoftirqd for further processing so using NAPI threads with this
> driver is highly recommended since it NULL routes the threaded-IRQ
> efforts.
> 
> Adding rtl_hw_aspm_clkreq_enable() to the ISR is problematic on
> PREEMPT_RT because the function uses spinlock_t locks which become
> sleeping locks on PREEMPT_RT. The locks are only used to protect
> register access and don't nest into other functions or locks. They are
> also not used for unbounded period of time. Therefore it looks okay to
> convert them to raw_spinlock_t.
> 
> Convert the three locks which are used from the interrupt service
> routine to raw_spinlock_t.
> 
> Fixes: e1ed3e4d91112 ("r8169: disable ASPM during NAPI poll")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> This is the only ethernet driver doing IRQF_NO_THREAD. It was recently
> enabled and based on thread it was argued to offer better performance
> with threaded interrupts and NAPI threads. So I'm trying this instead of
> reverting the whole thing since it does not seem necessary. Maybe the
> NAPI-threads could some special treats, I take a look.
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 44 +++++++++++------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>