Received-SPF: pass (google.com: domain of linux-kernel+bounces-29034-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 17 Jan 2024 14:44:38 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Mirsad Todorovac <mirsad.todorovac@alu.hr>
Cc: Jakub Kicinski <kuba@kernel.org>, Sasha Levin <sashal@kernel.org>,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>,
	Simon Horman <horms@kernel.org>,
	"David S . Miller" <davem@davemloft.net>, nic_swsd@realtek.com,
	edumazet@google.com, pabeni@redhat.com, netdev@vger.kernel.org
Subject: Re: [PATCH AUTOSEL 6.7 021/108] r8169: improve RTL8411b phy-down
 fixup
Message-ID: <2c590117-96fa-4e0b-84bd-9e3ea138d93b@lunn.ch>
References: <20240116194225.250921-1-sashal@kernel.org>
 <20240116194225.250921-21-sashal@kernel.org>
 <20240116174315.2629f21c@kernel.org>
 <4523ad21-d06a-4ba2-9b46-974a6093b189@alu.unizg.hr>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4523ad21-d06a-4ba2-9b46-974a6093b189@alu.unizg.hr>

On Wed, Jan 17, 2024 at 11:30:53AM +0100, Mirsad Todorovac wrote:
> On 1/17/24 02:43, Jakub Kicinski wrote:
> > On Tue, 16 Jan 2024 14:38:47 -0500 Sasha Levin wrote:
> > > Mirsad proposed a patch to reduce the number of spinlock lock/unlock
> > > operations and the function code size. This can be further improved
> > > because the function sets a consecutive register block.
> > 
> > Clearly a noop and a lot of LoC changed. I vote to drop this from
> > the backport.
> 
> Dear Jakub,
> 
> I will not argue with a senior developer, but please let me plead for the
> cause.
> 
> There are a couple of issues here:
> 
> 1. Heiner's patch generates smaller and faster code, with 100+
> spin_lock_irqsave()/spin_unlock_restore() pairs less.
> 
> According to this table:
> 
> [1] https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/perfbook/perfbook-1c.2023.06.11a.pdf#table.3.1
> 
> The cost of single lock can be 15.4 - 101.9 ns (for the example CPU),
> so total savings would be 1709 - 11310 ns. But as the event of PHY power
> down is not frequent, this might be a insignificant saving indeed.
> 
> 2. Why I had advertised atomic programming of RTL registers in the first
> place?
> 
> The mac_ocp_lock was introduced recently:
> 
> commit 91c8643578a21e435c412ffbe902bb4b4773e262
> Author: Heiner Kallweit <hkallweit1@gmail.com>
> Date:   Mon Mar 6 22:23:15 2023 +0100
> 
>     r8169: use spinlock to protect mac ocp register access
> 
>     For disabling ASPM during NAPI poll we'll have to access mac ocp
>     registers in atomic context. This could result in races because
>     a mac ocp read consists of a write to register OCPDR, followed
>     by a read from the same register. Therefore add a spinlock to
>     protect access to mac ocp registers.
> 
>     Reviewed-by: Simon Horman <simon.horman@corigine.com>
>     Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>     Tested-by: Holger Hoffst?tte <holger@applied-asynchrony.com>
>     Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Well, the answer is in the question - the very need for protecting the access
> to RTL_W(8|16|32) with locks comes from the fact that something was accessing
> the RTL card asynchronously.
> 
> Forgive me if this is a stupid question ...
> 
> Now - do we have a guarantee that the card will not be used asynchronously
> half-programmed from something else in that case, leading to another spurious
> lockup?
> 
> IMHO, shouldn't the entire reprogramming of PHY down recovery of the RTL 8411b
> be done atomically, under a single spin_lock_irqsave()/spin_unlock_irqrestore()
> pair?

Hi Mirsad

Please take a read of:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Do you think this patch fulfils these criteria? In particularly, "It
must either fix a real bug that bothers people...".

I agree with Heiner, this appears to be just an optimisation,

     Andrew