| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <587704fa-c93d-4f78-a1d2-c3020108e921@alu.unizg.hr>
Date: Mon, 30 Oct 2023 16:13:25 +0100
From: Mirsad Todorovac <mirsad.todorovac@....unizg.hr>
To: Marco Elver <elver@...gle.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, nic_swsd@...ltek.com,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH v3 1/1] r8169: Coalesce RTL8411b PHY power-down recovery
programming instructions to reduce spinlock stalls
On 10/30/23 15:05, Marco Elver wrote:
> On Mon, 30 Oct 2023 at 14:53, Mirsad Todorovac
> <mirsad.todorovac@....unizg.hr> wrote:
>>
>> On 10/30/23 14:30, Mirsad Todorovac wrote:
>>>
>>>
>>> On 10/30/23 14:17, Heiner Kallweit wrote:
>>>> On 29.10.2023 05:56, Mirsad Todorovac wrote:
>>>>>
>>>>>
>>>>> On 10/28/23 21:21, Heiner Kallweit wrote:
>>>>>> On 28.10.2023 13:05, Mirsad Goran Todorovac wrote:
>>>>>>> On RTL8411b the RX unit gets confused if the PHY is powered-down.
>>>>>>> This was reported in [0] and confirmed by Realtek. Realtek provided
>>>>>>> a sequence to fix the RX unit after PHY wakeup.
>>>>>>>
>>>>>>> A series of about 130 r8168_mac_ocp_write() calls is performed to
>>>>>>> program the RTL registers for recovery.
>>>>>>>
>>>>>>> r8168_mac_ocp_write() expands to this code:
>>>>>>>
>>>>>>> static void __r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
>>>>>>> {
>>>>>>> if (rtl_ocp_reg_failure(reg))
>>>>>>> return;
>>>>>>>
>>>>>>> RTL_W32(tp, OCPDR, OCPAR_FLAG | (reg << 15) | data);
>>>>>>> }
>>>>>>>
>>>>>>> static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
>>>>>>> {
>>>>>>> unsigned long flags;
>>>>>>>
>>>>>>> raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
>>>>>>> __r8168_mac_ocp_write(tp, reg, data);
>>>>>>> raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
>>>>>>> }
>>>>>>>
>>>>>>> Register programming is done through RTL_W32() macro which expands into
>>>>>>>
>>>>>>> #define RTL_W32(tp, reg, val32) writel((val32), tp->mmio_addr + (reg))
>>>>>>>
>>>>>>> which is further (on Alpha):
>>>>>>>
>>>>>>> extern inline void writel(u32 b, volatile void __iomem *addr)
>>>>>>> {
>>>>>>> mb();
>>>>>>> __raw_writel(b, addr);
>>>>>>> }
>>>>>>>
>>>>>>> or on i386/x86_64:
>>>>>>>
>>>>>>> #define build_mmio_write(name, size, type, reg, barrier) \
>>>>>>> static inline void name(type val, volatile void __iomem *addr) \
>>>>>>> { asm volatile("mov" size " %0,%1": :reg (val), \
>>>>>>> "m" (*(volatile type __force *)addr) barrier); }
>>>>>>>
>>>>>>> build_mmio_write(writel, "l", unsigned int, "r", :"memory")
>>>>>>>
>>>>>>> This obviously involves iat least a compiler barrier.
>>>>>>>
>>>>>>> mb() expands into something like this i.e. on x86_64:
>>>>>>>
>>>>>>> #define mb() asm volatile("lock; addl $0,0(%%esp)" ::: "memory")
>>>>>>>
>>>>>>> This means a whole lot of memory bus barriers: for spin_lock_irqsave(),
>>>>>>> memory barrier, writel(), and spin_unlock_irqrestore().
>>>>>>>
>>>>>>> With about 130 of these sequential calls to r8168_mac_ocp_write() this looks like
>>>>>>> a LOCK storm that will thunder all of the cores and CPUs on the same memory controller
>>>>>>> for certain time that locked memory read-modify-write cyclo or I/O takes to finish.
>>>>>>>
>>>>>>> In a sequential case of RTL register programming, the writes to RTL registers
>>>>>>> can be coalesced under a same raw spinlock. This can dramatically decrease the
>>>>>>> number of bus stalls in a multicore or multi-CPU system:
>>>>>>>
>>>>>>> static void __r8168_mac_ocp_write_seq(struct rtl8169_private *tp,
>>>>>>> const struct recover_8411b_info *array)
>>>>>>> {
>>>>>>> struct recover_8411b_info const *p = array;
>>>>>>>
>>>>>>> while (p->reg) {
>>>>>>> if (!rtl_ocp_reg_failure(p->reg))
>>>>>>> RTL_W32(tp, OCPDR, OCPAR_FLAG | (p->reg << 15) | p->data);
>>>>>>> p++;
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> static void r8168_mac_ocp_write_seq(struct rtl8169_private *tp,
>>>>>>> const struct recover_8411b_info *array)
>>>>>>> {
>>>>>>> unsigned long flags;
>>>>>>>
>>>>>>> raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
>>>>>>> __r8168_mac_ocp_write_seq(tp, array);
>>>>>>> raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
>>>>>>> }
>>>>>>>
>>>>>>> static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
>>>>>>> {
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>> /* The following Realtek-provided magic fixes an issue with the RX unit
>>>>>>> * getting confused after the PHY having been powered-down.
>>>>>>> */
>>>>>>>
>>>>>>> static const struct recover_8411b_info init_zero_seq[] = {
>>>>>>> { 0xFC28, 0x0000 }, { 0xFC2A, 0x0000 }, { 0xFC2C, 0x0000 }, { 0xFC2E, 0x0000 },
>>>>>>> ...
>>>>>>> };
>>>>>>>
>>>>>>> static const struct recover_8411b_info recover_seq[] = {
>>>>>>> { 0xF800, 0xE008 }, { 0xF802, 0xE00A }, { 0xF804, 0xE00C }, { 0xF806, 0xE00E },
>>>>>>> ...
>>>>>>> };
>>>>>>>
>>>>>>> static const struct recover_8411b_info final_seq[] = {
>>>>>>> { 0xFC2A, 0x0743 }, { 0xFC2C, 0x0801 }, { 0xFC2E, 0x0BE9 }, { 0xFC30, 0x02FD },
>>>>>>> ...
>>>>>>> };
>>>>>>>
>>>>>>> r8168_mac_ocp_write_seq(tp, init_zero_seq);
>>>>>>> mdelay(3);
>>>>>>> r8168_mac_ocp_write(tp, 0xFC26, 0x0000);
>>>>>>> r8168_mac_ocp_write_seq(tp, recover_seq);
>>>>>>> r8168_mac_ocp_write(tp, 0xFC26, 0x8000);
>>>>>>> r8168_mac_ocp_write_seq(tp, final_seq);
>>>>>>> }
>>>>>>>
>>>>>>> The hex data is preserved intact through s/r8168_mac_ocp_write[(]tp,/{ / and s/[)];/ },/
>>>>>>> functions that only changed the function names and the ending of the line, so the actual
>>>>>>> hex data is unchanged.
>>>>>>>
>>>>>>> Note that the original reason for the introduction of the commit fe4e8db0392a6
>>>>>>> was to enable recovery of the RX unit on the RTL8411b which was confused by the
>>>>>>> powered-down PHY. This sequence of r8168_mac_ocp_write() calls amplifies the problem
>>>>>>
>>>>>> I still have a problem with this statement as you're saying that the original
>>>>>> problem still exists. I don't think that's the case.
>>>>>
>>>>> I will not disagree about it.
>>>>>
>>>>> But we have only reduced the number of spin_lock_irqsave/spin_unlock_irqrestore()
>>>>> pairs.
>>>>>
>>>>> Maybe additionally, on the low level, memory barrier isn't required for each write to
>>>>> MMIO?
>>>>>
>>>> One could argue whether in several places writel_relaxed() could be used.
>>>> But it's not really worth it, because we're not in a hot path.
>>>
>>> I see. Thank you for your evaluation.
>>>
>>> Using writel_relaxed() sounds clever. It expands to:
>>>
>>> #define build_mmio_write(name, size, type, reg, barrier) \
>>> static inline void name(type val, volatile void __iomem *addr) \
>>> { asm volatile("mov" size " %0,%1": :reg (val), \
>>> "m" (*(volatile type __force *)addr) barrier); }
>>> build_mmio_write(__writel, "l", unsigned int, "r", )
>>> #define writel_relaxed(v, a) __writel(v, a)
>>>
>>> Here "barrier" is an empty string. Really clever. ;-)
>>>
>>> I will not contradict, but the cummulative amount of memory barriers on each MMIO read/write
>>> in each single one of the drivers could amount to some degrading of overall performance and
>>> latency in a multicore system.
>>>
>>> As I understood Mr. Jonathan Corbet on LWN, the initiative and trend is to reduce overall
>>> kernel latency.
>>
>> P.S.
>>
>> On the second thought, if barrier() is only the compiler optimisation barrier from memory
>> reordering, then we do not gain much disablin git as it doesn't affect the other cores, and
>> reordering MMIO writes can really confuse some NIC hardware.
>>
>> /* Optimization barrier */
>> #ifndef barrier
>> /* The "volatile" is due to gcc bugs */
>> # define barrier() __asm__ __volatile__("": : :"memory")
>> #endif
>>
>>>>> If it still uses a LOCK addl $0, m32/m64, then it still creates 130 instances of all core
>>>>> bus locks for this NIC reset after the lost PHY? I'm just thinking, this is nothing
>>>>> authoritative ...
>
> I would recommend looking at the pre-processed source code if you
> can't manually untangle the maze of macros. ;-)
>
> Look at the .${obj}.cmd file (e.g.
> drivers/net/ethernet/realtek/.r8169_main.o.cmd), copy the compiler
> command in it, and add the "-E" option and make the compiler write the
> result to some temporary file you can inspect.
Thanks for the tip. I tried to add
EXTRA_CFLAGS += -save-temps
to the driver/net/ethernet/realtek/Kbuild, but something went wrong. Now I see that it is deprecated.
Best regards,
Mirsad Todorovac
Powered by blists - more mailing lists