On 1/17/24 14:44, Andrew Lunn wrote:
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:
- 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/per...
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.
- 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,
Hi Andrew,
Yes, I wasn't aware of the 100 lines limit, and yes, this is not a bug fix, but an improvement (optimisation).
I think by this I can join to consensus, this patch is not a candidate for backporting. :-/
However, I am concerned about the possibility of two kernel threads accessing the RTL NIC intermittently attempting to program the NIC over the RTL_(R|W)(8|16|32) instructions (which are expanded to readl/writel and assembly).
AFAICS, nothing prevents two (or more) threads to decide in unlikely but possible case to program the card at the same time. (Do we have a guard lock against this?)
mac_ocp_lock appears to be acquired and released for each RTL_(R|W)(8|16|32), with the exception of r8168_mac_ocp_modify().
To be true to the facts, each byte will go to the right port due to address/data pairs used in each call - however, I am worried whether there could be a scenario like this:
CPU 1 CPU 2
start programming NIC programming NIC (preempted in spin_lock_irqsave() start programming NIC programming NIC programming NIC programming NIC preempted in spin_lock_irqsave() (resume control in spin_unlock_irqrestore() programming NIC programming NIC (preempted in spin_lock_irqsave() continue programming NIC programming NIC programming NIC end programming NIC (resume control in spin_unlock_irqrestore() programming NIC end programming NIC
Now, every byte, word or longword will come to the right place, thanks to RTL_(R|W)(8|16|32) having the address/data pairs - but I worry that this jumping from sequence to sequence might confuse the NIC.
I mean, if those latches behind the addresses cause some physical effect, maybe the ORDER is also important, not just that every byte, word or longword comes to the right address?
r8168_mac_ocp_read()/r8168_mac_ocp_write() guarantee that every piece of data will end being read or written at the right address, OK. But this does not seem to guarantee the SEQUENTIAL ORDER of the programming.
I mean, if we are dealing with physical hardware like a NIC, the order of (especially writing) data might be crucial. 8-/
Am I making any sense?
Are we algorithmically secured that two threads will never attempt to write data to NIC hardware registers?
Thanks.
Best regards, Mirsad Todorovac
Andrew