Received-SPF: pass (google.com: domain of linux-kernel+bounces-29220-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ccb8b694-4361-40bc-b7df-528a6616c15b@alu.unizg.hr>
Date: Wed, 17 Jan 2024 17:35:17 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH AUTOSEL 6.7 021/108] r8169: improve RTL8411b phy-down
 fixup
Content-Language: en-US
To: Andrew Lunn <andrew@lunn.ch>, 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
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>
 <2c590117-96fa-4e0b-84bd-9e3ea138d93b@lunn.ch>
From: Mirsad Todorovac <mirsad.todorovac@alu.hr>
In-Reply-To: <2c590117-96fa-4e0b-84bd-9e3ea138d93b@lunn.ch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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:
>>
>> 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,

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

-- 
CARNet system engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb

CARNet sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu