A pair of new helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq()
are introduced.
The motivation for these helpers was the locking overhead of 130 consecutive
r8168_mac_ocp_write() calls in the RTL8411b reset after the NIC gets confused
if the PHY is powered-down.
To quote Heiner:
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, each doing an expensive spin_lock_irqsave() and
spin_unlock_irqrestore().
Each mac ocp write is made of:
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 stalls: 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 stall all of the cores and CPUs on the same memory controller
for certain time 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.
Macro helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq() are
provided to reduce lock contention:
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 },
...
};
...
r8168_mac_ocp_write_seq(tp, init_zero_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.
To repeat, the reason for the introduction of the original commit
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
into a series of about 500+ memory bus locks, most waiting for the main memory read,
modify and write under a LOCK. The memory barrier in RTL_W32 should suffice for
the programming sequence to reach RTL NIC registers.
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1692075
Fixes: fe4e8db0392a6 ("r8169: fix issue with confused RX unit after PHY power-down on RTL8411b")
Fixes: 91c8643578a21 ("r8169: use spinlock to protect mac ocp register access")
Fixes: d6c36cbc5e533 ("r8169: Use a raw_spinlock_t for the register locks.")
Cc: Heiner Kallweit <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: [email protected]
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
v4:
fixed complaints as advised by Heiner and checkpatch.pl.
split the patch into five sections to be more easily manipulated and reviewed
introduced r8168_mac_ocp_write_seq()
applied coalescing of mac ocp writes/modifies for 8168H, 8125 and 8125B
v3:
removed register/mask pair array sentinels, so using ARRAY_SIZE().
avoided duplication of RTL_W32() call code as advised by Heiner.
drivers/net/ethernet/realtek/r8169_main.c | 57 +++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 361b90007148..df79fd95cf2d 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -939,6 +939,63 @@ static void r8168_mac_ocp_modify(struct rtl8169_private *tp, u32 reg, u16 mask,
raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
}
+struct e_info_regdata {
+ u32 reg;
+ u32 data;
+};
+
+struct e_info_regmaskset {
+ u32 reg;
+ u16 mask;
+ u16 set;
+};
+
+static void __r8168_mac_ocp_write_seqlen(struct rtl8169_private *tp,
+ const struct e_info_regdata *array, int len)
+{
+ struct e_info_regdata const *p;
+
+ for (p = array; len--; p++)
+ __r8168_mac_ocp_write(tp, p->reg, p->data);
+}
+
+static void r8168_mac_ocp_write_seqlen(struct rtl8169_private *tp,
+ const struct e_info_regdata *array, int len)
+{
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+ __r8168_mac_ocp_write_seqlen(tp, array, len);
+ raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+}
+
+static void __r8168_mac_ocp_modify_seqlen(struct rtl8169_private *tp,
+ const struct e_info_regmaskset *array, int len)
+{
+ struct e_info_regmaskset const *p;
+ u16 data;
+
+ for (p = array; len--; p++) {
+ data = __r8168_mac_ocp_read(tp, p->reg);
+ __r8168_mac_ocp_write(tp, p->reg, (data & ~p->mask) | p->set);
+ }
+}
+
+static void r8168_mac_ocp_modify_seqlen(struct rtl8169_private *tp,
+ const struct e_info_regmaskset *array, int len)
+{
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+ __r8168_mac_ocp_modify_seqlen(tp, array, len);
+ raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+}
+
+#define r8168_mac_ocp_write_seq(tp, a) r8168_mac_ocp_write_seqlen(tp, a, ARRAY_SIZE(a))
+#define r8168_mac_ocp_modify_seq(tp, a) r8168_mac_ocp_modify_seqlen(tp, a, ARRAY_SIZE(a))
+#define __r8168_mac_ocp_write_seq(tp, a) __r8168_mac_ocp_write_seqlen(tp, a, ARRAY_SIZE(a))
+#define __r8168_mac_ocp_modify_seq(tp, a) __r8168_mac_ocp_modify_seqlen(tp, a, ARRAY_SIZE(a))
+
/* Work around a hw issue with RTL8168g PHY, the quirk disables
* PHY MCU interrupts before PHY power-down.
*/
--
2.34.1
Repeated calls to r8168_mac_ocp_write() and r8168_mac_ocp_modify() in
the startup of 8168H involve implicit spin_lock_irqsave() and spin_unlock_irqrestore()
on each invocation.
Coalesced with the corresponding helpers, r8168_mac_ocp_write_seq() and
r8168_mac_ocp_modify_seq() with a sinqle lock/unlock, these calls reduce overall
lock contention.
Fixes: ef712ede3541d ("r8169: add helper r8168_mac_ocp_modify")
Fixes: 6e1d0b8988188 ("r8169:add support for RTL8168H and RTL8107E")
Cc: Heiner Kallweit <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: [email protected]
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
v4:
fixed complaints as advised by Heiner and checkpatch.pl.
split the patch into five sections to be more easily manipulated and reviewed
introduced r8168_mac_ocp_write_seq()
applied coalescing of mac ocp writes/modifies for 8168H, 8125 and 8125B
v3:
removed register/mask pair array sentinels, so using ARRAY_SIZE().
avoided duplication of RTL_W32() call code as advised by Heiner.
drivers/net/ethernet/realtek/r8169_main.c | 26 +++++++++++++++--------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 3dae924a6ca3..cbaac4675bd2 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -3227,6 +3227,21 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
{ 0x04, 0xffff, 0x854a },
{ 0x01, 0xffff, 0x068b }
};
+
+ static const struct e_info_regmaskset e_info_regmaskset_8168h_1[] = {
+ { 0xe056, 0x00f0, 0x0070 },
+ { 0xe052, 0x6000, 0x8008 },
+ { 0xe0d6, 0x01ff, 0x017f },
+ { 0xd420, 0x0fff, 0x047f },
+ };
+
+ static const struct e_info_regdata e_info_regdata_8168h_1[] = {
+ { 0xe63e, 0x0001 },
+ { 0xe63e, 0x0000 },
+ { 0xc094, 0x0000 },
+ { 0xc09e, 0x0000 },
+ };
+
int rg_saw_cnt;
rtl_ephy_init(tp, e_info_8168h_1);
@@ -3267,15 +3282,8 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
}
- r8168_mac_ocp_modify(tp, 0xe056, 0x00f0, 0x0070);
- r8168_mac_ocp_modify(tp, 0xe052, 0x6000, 0x8008);
- r8168_mac_ocp_modify(tp, 0xe0d6, 0x01ff, 0x017f);
- r8168_mac_ocp_modify(tp, 0xd420, 0x0fff, 0x047f);
-
- r8168_mac_ocp_write(tp, 0xe63e, 0x0001);
- r8168_mac_ocp_write(tp, 0xe63e, 0x0000);
- r8168_mac_ocp_write(tp, 0xc094, 0x0000);
- r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
+ r8168_mac_ocp_modify_seq(tp, e_info_regmaskset_8168h_1);
+ r8168_mac_ocp_write_seq(tp, e_info_regdata_8168h_1);
}
static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
--
2.34.1
Repeated calls to r8168_mac_ocp_write() and r8168_mac_ocp_modify() in
the startup of 8125 and 8125B involve implicit spin_lock_irqsave() and
spin_unlock_irqrestore() on each invocation.
Coalesced with the corresponding helpers r8168_mac_ocp_write_seq() and
r8168_mac_ocp_modify_seq() into sequential write or modidy with a sinqle lock/unlock,
these calls reduce overall lock contention.
Fixes: f1bce4ad2f1ce ("r8169: add support for RTL8125")
Fixes: 0439297be9511 ("r8169: add support for RTL8125B")
Cc: Heiner Kallweit <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: [email protected]
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
v4:
fixed complaints as advised by Heiner and checkpatch.pl.
split the patch into five sections to be more easily manipulated and reviewed
introduced r8168_mac_ocp_write_seq()
applied coalescing of mac ocp writes/modifies for 8168H, 8125 and 8125B
v3:
removed register/mask pair array sentinels, so using ARRAY_SIZE().
avoided duplication of RTL_W32() call code as advised by Heiner.
drivers/net/ethernet/realtek/r8169_main.c | 38 ++++++++++++++---------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index cbaac4675bd2..dd65e0384ab3 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -3539,6 +3539,27 @@ DECLARE_RTL_COND(rtl_mac_ocp_e00e_cond)
static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
{
+
+ static const struct e_info_regmaskset e_info_8125_common_1[] = {
+ { 0xd3e2, 0x0fff, 0x03a9 },
+ { 0xd3e4, 0x00ff, 0x0000 },
+ { 0xe860, 0x0000, 0x0080 },
+ };
+
+ static const struct e_info_regmaskset e_info_8125_common_2[] = {
+ { 0xc0b4, 0x0000, 0x000c },
+ { 0xeb6a, 0x00ff, 0x0033 },
+ { 0xeb50, 0x03e0, 0x0040 },
+ { 0xe056, 0x00f0, 0x0030 },
+ { 0xe040, 0x1000, 0x0000 },
+ { 0xea1c, 0x0003, 0x0001 },
+ { 0xe0c0, 0x4f0f, 0x4403 },
+ { 0xe052, 0x0080, 0x0068 },
+ { 0xd430, 0x0fff, 0x047f },
+ { 0xea1c, 0x0004, 0x0000 },
+ { 0xeb54, 0x0000, 0x0001 },
+ };
+
rtl_pcie_state_l2l3_disable(tp);
RTL_W16(tp, 0x382, 0x221b);
@@ -3553,9 +3574,7 @@ static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
r8168_mac_ocp_write(tp, 0xc140, 0xffff);
r8168_mac_ocp_write(tp, 0xc142, 0xffff);
- r8168_mac_ocp_modify(tp, 0xd3e2, 0x0fff, 0x03a9);
- r8168_mac_ocp_modify(tp, 0xd3e4, 0x00ff, 0x0000);
- r8168_mac_ocp_modify(tp, 0xe860, 0x0000, 0x0080);
+ r8168_mac_ocp_modify_seq(tp, e_info_8125_common_1);
/* disable new tx descriptor format */
r8168_mac_ocp_modify(tp, 0xeb58, 0x0001, 0x0000);
@@ -3570,18 +3589,7 @@ static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
else
r8168_mac_ocp_modify(tp, 0xe63e, 0x0c30, 0x0020);
- r8168_mac_ocp_modify(tp, 0xc0b4, 0x0000, 0x000c);
- r8168_mac_ocp_modify(tp, 0xeb6a, 0x00ff, 0x0033);
- r8168_mac_ocp_modify(tp, 0xeb50, 0x03e0, 0x0040);
- r8168_mac_ocp_modify(tp, 0xe056, 0x00f0, 0x0030);
- r8168_mac_ocp_modify(tp, 0xe040, 0x1000, 0x0000);
- r8168_mac_ocp_modify(tp, 0xea1c, 0x0003, 0x0001);
- r8168_mac_ocp_modify(tp, 0xe0c0, 0x4f0f, 0x4403);
- r8168_mac_ocp_modify(tp, 0xe052, 0x0080, 0x0068);
- r8168_mac_ocp_modify(tp, 0xd430, 0x0fff, 0x047f);
-
- r8168_mac_ocp_modify(tp, 0xea1c, 0x0004, 0x0000);
- r8168_mac_ocp_modify(tp, 0xeb54, 0x0000, 0x0001);
+ r8168_mac_ocp_modify_seq(tp, e_info_8125_common_2);
udelay(1);
r8168_mac_ocp_modify(tp, 0xeb54, 0x0001, 0x0000);
RTL_W16(tp, 0x1880, RTL_R16(tp, 0x1880) & ~0x0030);
--
2.34.1
Repeated calls to r8168_mac_ocp_write() and r8168_mac_ocp_modify() in
the init sequence of the 8125 involve implicit spin_lock_irqsave() and
spin_unlock_irqrestore() on each invocation.
Coalesced with the corresponding helpers r8168_mac_ocp_write_seq() and
r8168_mac_ocp_modify_seq() into sequential write or modidy with a sinqle lock/unlock,
these calls reduce overall lock contention.
Fixes: f1bce4ad2f1ce ("r8169: add support for RTL8125")
Cc: Heiner Kallweit <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: [email protected]
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Mirsad Goran Todorovac <[email protected]>
---
v4:
fixed complaints as advised by Heiner and checkpatch.pl.
split the patch into five sections to be more easily manipulated and reviewed
introduced r8168_mac_ocp_write_seq()
applied coalescing of mac ocp writes/modifies for 8168H, 8125 and 8125B
v3:
removed register/mask pair array sentinels, so using ARRAY_SIZE().
avoided duplication of RTL_W32() call code as advised by Heiner.
drivers/net/ethernet/realtek/r8169_main.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index dd65e0384ab3..8dce08d90367 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5070,6 +5070,12 @@ static void rtl_hw_init_8168g(struct rtl8169_private *tp)
static void rtl_hw_init_8125(struct rtl8169_private *tp)
{
+ static const struct e_info_regdata hw_init_8125_1[] = {
+ { 0xc0aa, 0x07d0 },
+ { 0xc0a6, 0x0150 },
+ { 0xc01e, 0x5555 },
+ };
+
rtl_enable_rxdvgate(tp);
RTL_W8(tp, ChipCmd, RTL_R8(tp, ChipCmd) & ~(CmdTxEnb | CmdRxEnb));
@@ -5079,9 +5085,7 @@ static void rtl_hw_init_8125(struct rtl8169_private *tp)
r8168_mac_ocp_modify(tp, 0xe8de, BIT(14), 0);
r8168g_wait_ll_share_fifo_ready(tp);
- r8168_mac_ocp_write(tp, 0xc0aa, 0x07d0);
- r8168_mac_ocp_write(tp, 0xc0a6, 0x0150);
- r8168_mac_ocp_write(tp, 0xc01e, 0x5555);
+ r8168_mac_ocp_write_seq(tp, hw_init_8125_1);
r8168g_wait_ll_share_fifo_ready(tp);
}
--
2.34.1
On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:> A pair of new
helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq()
> are introduced.
>
> The motivation for these helpers was the locking overhead of 130 consecutive
> r8168_mac_ocp_write() calls in the RTL8411b reset after the NIC gets confused
> if the PHY is powered-down.
>
> To quote Heiner:
>
> 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, each doing an expensive spin_lock_irqsave() and
> spin_unlock_irqrestore().
>
> Each mac ocp write is made of:
>
> 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 stalls: 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 stall all of the cores and CPUs on the same memory controller
> for certain time 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.
>
> Macro helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq() are
> provided to reduce lock contention:
>
> 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 },
> ...
> };
>
> ...
>
> r8168_mac_ocp_write_seq(tp, init_zero_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.
>
> To repeat, the reason for the introduction of the original commit
> 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
> into a series of about 500+ memory bus locks, most waiting for the main memory read,
> modify and write under a LOCK. The memory barrier in RTL_W32 should suffice for
> the programming sequence to reach RTL NIC registers.
>
> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1692075
>
I might have chosen to send some of this information as the cover letter
for the series instead of just as part of the commit message for [1/5],
but either way:
Reviewed-by: Jacob Keller <[email protected]>
On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:
> Repeated calls to r8168_mac_ocp_write() and r8168_mac_ocp_modify() in
> the startup of 8168H involve implicit spin_lock_irqsave() and spin_unlock_irqrestore()
> on each invocation.
>
> Coalesced with the corresponding helpers, r8168_mac_ocp_write_seq() and
> r8168_mac_ocp_modify_seq() with a sinqle lock/unlock, these calls reduce overall
> lock contention.
>
> Fixes: ef712ede3541d ("r8169: add helper r8168_mac_ocp_modify")
> Fixes: 6e1d0b8988188 ("r8169:add support for RTL8168H and RTL8107E")
> Cc: Heiner Kallweit <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: [email protected]
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Mirsad Goran Todorovac <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:
> Repeated calls to r8168_mac_ocp_write() and r8168_mac_ocp_modify() in
> the startup of 8125 and 8125B involve implicit spin_lock_irqsave() and
> spin_unlock_irqrestore() on each invocation.
>
> Coalesced with the corresponding helpers r8168_mac_ocp_write_seq() and
> r8168_mac_ocp_modify_seq() into sequential write or modidy with a sinqle lock/unlock,
> these calls reduce overall lock contention.
>
> Fixes: f1bce4ad2f1ce ("r8169: add support for RTL8125")
> Fixes: 0439297be9511 ("r8169: add support for RTL8125B")
> Cc: Heiner Kallweit <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: [email protected]
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Mirsad Goran Todorovac <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:
> Repeated calls to r8168_mac_ocp_write() and r8168_mac_ocp_modify() in
> the init sequence of the 8125 involve implicit spin_lock_irqsave() and
> spin_unlock_irqrestore() on each invocation.
>
> Coalesced with the corresponding helpers r8168_mac_ocp_write_seq() and
> r8168_mac_ocp_modify_seq() into sequential write or modidy with a sinqle lock/unlock,
> these calls reduce overall lock contention.
>
> Fixes: f1bce4ad2f1ce ("r8169: add support for RTL8125")
> Cc: Heiner Kallweit <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: [email protected]
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]/
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Mirsad Goran Todorovac <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
On 30.10.2023 22:50, Jacob Keller wrote:
>
>
> On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:> A pair of new
> helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq()
>> are introduced.
>>
>> The motivation for these helpers was the locking overhead of 130 consecutive
>> r8168_mac_ocp_write() calls in the RTL8411b reset after the NIC gets confused
>> if the PHY is powered-down.
>>
>> To quote Heiner:
>>
>> 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, each doing an expensive spin_lock_irqsave() and
>> spin_unlock_irqrestore().
>>
>> Each mac ocp write is made of:
>>
>> 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 stalls: 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 stall all of the cores and CPUs on the same memory controller
>> for certain time 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.
>>
>> Macro helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq() are
>> provided to reduce lock contention:
>>
>> 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 },
>> ...
>> };
>>
>> ...
>>
>> r8168_mac_ocp_write_seq(tp, init_zero_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.
>>
>> To repeat, the reason for the introduction of the original commit
>> 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
>> into a series of about 500+ memory bus locks, most waiting for the main memory read,
>> modify and write under a LOCK. The memory barrier in RTL_W32 should suffice for
>> the programming sequence to reach RTL NIC registers.
>>
>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1692075
>>
>
>
> I might have chosen to send some of this information as the cover letter
> for the series instead of just as part of the commit message for [1/5],
> but either way:
>
> Reviewed-by: Jacob Keller <[email protected]>
Cover letter is still missing, and there's a v5 already.
Good example why we have the "max one version per day" rule.
There's still some issues with the series, see my review comments
for v5. As-is I'd NAK the series.
On 10/30/2023 3:08 PM, Heiner Kallweit wrote:
> On 30.10.2023 22:50, Jacob Keller wrote:
>>
>>
>> On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:> A pair of new
>> helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq()
>>> are introduced.
>>>
>>> The motivation for these helpers was the locking overhead of 130 consecutive
>>> r8168_mac_ocp_write() calls in the RTL8411b reset after the NIC gets confused
>>> if the PHY is powered-down.
>>>
>>> To quote Heiner:
>>>
>>> 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, each doing an expensive spin_lock_irqsave() and
>>> spin_unlock_irqrestore().
>>>
>>> Each mac ocp write is made of:
>>>
>>> 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 stalls: 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 stall all of the cores and CPUs on the same memory controller
>>> for certain time 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.
>>>
>>> Macro helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq() are
>>> provided to reduce lock contention:
>>>
>>> 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 },
>>> ...
>>> };
>>>
>>> ...
>>>
>>> r8168_mac_ocp_write_seq(tp, init_zero_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.
>>>
>>> To repeat, the reason for the introduction of the original commit
>>> 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
>>> into a series of about 500+ memory bus locks, most waiting for the main memory read,
>>> modify and write under a LOCK. The memory barrier in RTL_W32 should suffice for
>>> the programming sequence to reach RTL NIC registers.
>>>
>>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1692075
>>>
>>
>>
>> I might have chosen to send some of this information as the cover letter
>> for the series instead of just as part of the commit message for [1/5],
>> but either way:
>>
>> Reviewed-by: Jacob Keller <[email protected]>
>
> Cover letter is still missing, and there's a v5 already.
> Good example why we have the "max one version per day" rule.
>
> There's still some issues with the series, see my review comments
> for v5. As-is I'd NAK the series.
>
Heh, ya. A v5 was sent without there being a single (public) comment on
the list prior to my reviewing. I didn't notice the v5, and my mail
scripts pointed out this series didn't have anyone who'd looked at it
yet.. I guess I could have searched for and noticed a newer version.
Thanks,
Jake
On 10/31/23 00:14, Jacob Keller wrote:
>
>
> On 10/30/2023 3:08 PM, Heiner Kallweit wrote:
>> On 30.10.2023 22:50, Jacob Keller wrote:
>>>
>>>
>>> On 10/29/2023 4:04 AM, Mirsad Goran Todorovac wrote:> A pair of new
>>> helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq()
>>>> are introduced.
>>>>
>>>> The motivation for these helpers was the locking overhead of 130 consecutive
>>>> r8168_mac_ocp_write() calls in the RTL8411b reset after the NIC gets confused
>>>> if the PHY is powered-down.
>>>>
>>>> To quote Heiner:
>>>>
>>>> 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, each doing an expensive spin_lock_irqsave() and
>>>> spin_unlock_irqrestore().
>>>>
>>>> Each mac ocp write is made of:
>>>>
>>>> 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 stalls: 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 stall all of the cores and CPUs on the same memory controller
>>>> for certain time 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.
>>>>
>>>> Macro helpers r8168_mac_ocp_write_seq() and r8168_mac_ocp_modify_seq() are
>>>> provided to reduce lock contention:
>>>>
>>>> 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 },
>>>> ...
>>>> };
>>>>
>>>> ...
>>>>
>>>> r8168_mac_ocp_write_seq(tp, init_zero_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.
>>>>
>>>> To repeat, the reason for the introduction of the original commit
>>>> 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
>>>> into a series of about 500+ memory bus locks, most waiting for the main memory read,
>>>> modify and write under a LOCK. The memory barrier in RTL_W32 should suffice for
>>>> the programming sequence to reach RTL NIC registers.
>>>>
>>>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1692075
>>>>
>>>
>>>
>>> I might have chosen to send some of this information as the cover letter
>>> for the series instead of just as part of the commit message for [1/5],
>>> but either way:
>>>
>>> Reviewed-by: Jacob Keller <[email protected]>
>>
>> Cover letter is still missing, and there's a v5 already.
>> Good example why we have the "max one version per day" rule.
>>
>> There's still some issues with the series, see my review comments
>> for v5. As-is I'd NAK the series.
I realise we need to keep the development process coherent. I am sorry that
my inexperience in the patch submission process made the whole series look bad.
As I previously stated to Mr. Kallweit, I will do the required number of iterations
to ensure the quality of the patches (I saw some go up to over 20 versions).
> Heh, ya. A v5 was sent without there being a single (public) comment on
> the list prior to my reviewing. I didn't notice the v5, and my mail
> scripts pointed out this series didn't have anyone who'd looked at it
> yet.. I guess I could have searched for and noticed a newer version.
Well, dear Sir,
I see I owe you an apology for I did not know about the "max one version per day"
rule. I was warned however not to overwhelm the maintainers by Guillaume Nault in
January and somehow I hypomanicaly OCD'd on this. My fault entirely.
I hope we can mend this.
I guess this is my time to take a break, do some homework and return to the drawing
board.
Besides, now we are in the merge window anyway, so I should thank Mr. Kallweit for
the special attention and for making an exception.
Am I allowed to keep Mr. Keller's Reviewed-by: tags on the reviewed diffs provided
that I fix the cover letter issue and objections?
Have a nice day.
Regards,
Mirsad
On 10/30/2023 8:51 PM, Mirsad Todorovac wrote:
> Am I allowed to keep Mr. Keller's Reviewed-by: tags on the reviewed diffs provided
> that I fix the cover letter issue and objections?
>
I have no objections as long as the content otherwise remains the same :)
Thanks,
Jake
On 10/31/23 20:46, Jacob Keller wrote:
>
>
> On 10/30/2023 8:51 PM, Mirsad Todorovac wrote:
>> Am I allowed to keep Mr. Keller's Reviewed-by: tags on the reviewed diffs provided
>> that I fix the cover letter issue and objections?
>>
>
> I have no objections as long as the content otherwise remains the same :)
>
> Thanks,
> Jake
Of course, one changed character would require another review.
Thank you.
Mirsad