target/linux/layerscape/patches-5.4/701-net-0236-enetc-Replace-enetc_gregs-with-a-readers-writer-lock.patch

   1 From 659f899773f9f3fdc8325f61acc1017dd838126c Mon Sep 17 00:00:00 2001
   2 From: Vladimir Oltean <vladimir.oltean@nxp.com>
   3 Date: Thu, 14 Nov 2019 17:30:50 +0200
   4 Subject: [PATCH] enetc: Replace enetc_gregs with a readers-writer lock
   5
   6 The LS1028A MDIO errata tells us that any MDIO register access must not
   7 be concurrent with any other ENETC register access.
   8
   9 That has been handled so far by a number of per-CPU spinlocks over the
  10 ENETC register map. This came as an optimization over a single spinlock,
  11 because the regular register accesses can still be concurrent with one
  12 another, as long as they aren't concurrent with MDIO.
  13
  14 But this logic is broken in RT, because the enetc_rd_reg_wa and
  15 enetc_wr_reg_wa functions can be preempted in any context, and when they
  16 resume they may not run on the same CPU.
  17
  18 This renders the logic to take the per-CPU spinlock pointless, since the
  19 spinlock may not be the correct one (corresponding to this CPU) after
  20 preemption has occurred.
  21
  22 The following splat is telling us the same thing:
  23
  24 [   19.073928] BUG: using smp_processor_id() in preemptible [00000000] code: systemd-network/3423
  25 [   19.073932] caller is debug_smp_processor_id+0x1c/0x30
  26 [   19.073935] CPU: 1 PID: 3423 Comm: systemd-network Not tainted 4.19.68-rt26 #1
  27 [   19.073936] Hardware name: LS1028A RDB Board (DT)
  28 [   19.073938] Call trace:
  29 [   19.073940]  dump_backtrace+0x0/0x1a0
  30 [   19.073942]  show_stack+0x24/0x30
  31 [   19.073945]  dump_stack+0x9c/0xdc
  32 [   19.073948]  check_preemption_disabled+0xe0/0x100
  33 [   19.073951]  debug_smp_processor_id+0x1c/0x30
  34 [   19.073954]  enetc_open+0x1b0/0xbc0
  35 [   19.073957]  __dev_open+0xdc/0x160
  36 [   19.073960]  __dev_change_flags+0x160/0x1d0
  37 [   19.073963]  dev_change_flags+0x34/0x70
  38 [   19.073966]  do_setlink+0x2a0/0xcd0
  39 [   19.073969]  rtnl_setlink+0xe4/0x140
  40 [   19.073972]  rtnetlink_rcv_msg+0x18c/0x500
  41 [   19.073975]  netlink_rcv_skb+0x60/0x120
  42 [   19.073978]  rtnetlink_rcv+0x28/0x40
  43 [   19.073982]  netlink_unicast+0x194/0x210
  44 [   19.073985]  netlink_sendmsg+0x194/0x330
  45 [   19.073987]  sock_sendmsg+0x34/0x50
  46 [   19.073990]  __sys_sendto+0xe4/0x150
  47 [   19.073992]  __arm64_sys_sendto+0x30/0x40
  48 [   19.073996]  el0_svc_common+0xa4/0x1a0
  49 [   19.073999]  el0_svc_handler+0x38/0x80
  50 [   19.074002]  el0_svc+0x8/0xc
  51
  52 But there already exists a spinlock optimized for the single writer,
  53 multiple readers case: the rwlock_t. The writer in this case is the MDIO
  54 access code (irrelevant whether that MDIO access is a register read or
  55 write), and the reader is everybody else.
  56
  57 This patch also fixes two more existing bugs in the errata workaround:
  58 - The MDIO access code was not unlocking the per-CPU spinlocks in the
  59   reverse order of their locking order.
  60 - The per-CPU spinlock array was not initialized.
  61
  62 Fixes: 5ec0d668d62e ("enetc: WA for MDIO register access issue")
  63 Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
  64 Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
  65 ---
  66  drivers/net/ethernet/freescale/enetc/enetc.c    | 59 ++++++----------------
  67  drivers/net/ethernet/freescale/enetc/enetc_hw.h | 67 ++++---------------------
  68  drivers/net/ethernet/freescale/enetc/enetc_pf.c |  5 +-
  69  3 files changed, 30 insertions(+), 101 deletions(-)
  70
  71 --- a/drivers/net/ethernet/freescale/enetc/enetc.c
  72 +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
  73 @@ -20,13 +20,8 @@ netdev_tx_t enetc_xmit(struct sk_buff *s
  74  {
  75         struct enetc_ndev_priv *priv = netdev_priv(ndev);
  76         struct enetc_bdr *tx_ring;
  77 -       unsigned long flags;
  78 -       /* pointer to per-cpu ENETC lock for register access issue WA */
  79 -       spinlock_t *lock;
  80         int count;
  81
  82 -       lock = this_cpu_ptr(&enetc_gregs);
  83 -
  84         tx_ring = priv->tx_ring[skb->queue_mapping];
  85
  86         if (unlikely(skb_shinfo(skb)->nr_frags > ENETC_MAX_SKB_FRAGS))
  87 @@ -39,11 +34,9 @@ netdev_tx_t enetc_xmit(struct sk_buff *s
  88                 return NETDEV_TX_BUSY;
  89         }
  90
  91 -       spin_lock_irqsave(lock, flags);
  92 -
  93 +       read_lock(&enetc_mdio_lock);
  94         count = enetc_map_tx_buffs(tx_ring, skb, priv->active_offloads);
  95 -
  96 -       spin_unlock_irqrestore(lock, flags);
  97 +       read_unlock(&enetc_mdio_lock);
  98
  99         if (unlikely(!count))
 100                 goto drop_packet_err;
 101 @@ -259,13 +252,9 @@ dma_err:
 102  static irqreturn_t enetc_msix(int irq, void *data)
 103  {
 104         struct enetc_int_vector *v = data;
 105 -       unsigned long flags;
 106 -       /* pointer to per-cpu ENETC lock for register access issue WA */
 107 -       spinlock_t *lock;
 108         int i;
 109
 110 -       lock = this_cpu_ptr(&enetc_gregs);
 111 -       spin_lock_irqsave(lock, flags);
 112 +       read_lock(&enetc_mdio_lock);
 113
 114         /* disable interrupts */
 115         enetc_wr_reg_hot(v->rbier, 0);
 116 @@ -273,7 +262,7 @@ static irqreturn_t enetc_msix(int irq, v
 117         for_each_set_bit(i, &v->tx_rings_map, ENETC_MAX_NUM_TXQS)
 118                 enetc_wr_reg_hot(v->tbier_base + ENETC_BDR_OFF(i), 0);
 119
 120 -       spin_unlock_irqrestore(lock, flags);
 121 +       read_unlock(&enetc_mdio_lock);
 122
 123         napi_schedule_irqoff(&v->napi);
 124
 125 @@ -289,9 +278,6 @@ static int enetc_poll(struct napi_struct
 126         struct enetc_int_vector
 127                 *v = container_of(napi, struct enetc_int_vector, napi);
 128         bool complete = true;
 129 -       unsigned long flags;
 130 -       /* pointer to per-cpu ENETC lock for register access issue WA */
 131 -       spinlock_t *lock;
 132         int work_done;
 133         int i;
 134
 135 @@ -308,8 +294,7 @@ static int enetc_poll(struct napi_struct
 136
 137         napi_complete_done(napi, work_done);
 138
 139 -       lock = this_cpu_ptr(&enetc_gregs);
 140 -       spin_lock_irqsave(lock, flags);
 141 +       read_lock(&enetc_mdio_lock);
 142
 143         /* enable interrupts */
 144         enetc_wr_reg_hot(v->rbier, ENETC_RBIER_RXTIE);
 145 @@ -318,7 +303,7 @@ static int enetc_poll(struct napi_struct
 146                 enetc_wr_reg_hot(v->tbier_base + ENETC_BDR_OFF(i),
 147                                  ENETC_TBIER_TXTIE);
 148
 149 -       spin_unlock_irqrestore(lock, flags);
 150 +       read_unlock(&enetc_mdio_lock);
 151
 152         return work_done;
 153  }
 154 @@ -363,18 +348,12 @@ static bool enetc_clean_tx_ring(struct e
 155         bool do_tstamp;
 156         u64 tstamp = 0;
 157
 158 -       unsigned long flags;
 159 -       /* pointer to per-cpu ENETC lock for register access issue WA */
 160 -       spinlock_t *lock;
 161 -
 162 -       lock = this_cpu_ptr(&enetc_gregs);
 163 -
 164         i = tx_ring->next_to_clean;
 165         tx_swbd = &tx_ring->tx_swbd[i];
 166
 167 -       spin_lock_irqsave(lock, flags);
 168 +       read_lock(&enetc_mdio_lock);
 169         bds_to_clean = enetc_bd_ready_count(tx_ring, i);
 170 -       spin_unlock_irqrestore(lock, flags);
 171 +       read_unlock(&enetc_mdio_lock);
 172
 173         do_tstamp = false;
 174
 175 @@ -417,7 +396,7 @@ static bool enetc_clean_tx_ring(struct e
 176                         tx_swbd = tx_ring->tx_swbd;
 177                 }
 178
 179 -               spin_lock_irqsave(lock, flags);
 180 +               read_lock(&enetc_mdio_lock);
 181
 182                 /* BD iteration loop end */
 183                 if (is_eof) {
 184 @@ -430,7 +409,7 @@ static bool enetc_clean_tx_ring(struct e
 185                 if (unlikely(!bds_to_clean))
 186                         bds_to_clean = enetc_bd_ready_count(tx_ring, i);
 187
 188 -               spin_unlock_irqrestore(lock, flags);
 189 +               read_unlock(&enetc_mdio_lock);
 190         }
 191
 192         tx_ring->next_to_clean = i;
 193 @@ -516,7 +495,7 @@ static int enetc_refill_rx_ring(struct e
 194  }
 195
 196  #ifdef CONFIG_FSL_ENETC_HW_TIMESTAMPING
 197 -/* Must be called with &enetc_gregs spinlock held */
 198 +/* Must be called with the read-side enetc_mdio_lock held */
 199  static void enetc_get_rx_tstamp(struct net_device *ndev,
 200                                 union enetc_rx_bd *rxbd,
 201                                 struct sk_buff *skb)
 202 @@ -667,12 +646,6 @@ static int enetc_clean_rx_ring(struct en
 203         int rx_frm_cnt = 0, rx_byte_cnt = 0;
 204         int cleaned_cnt, i;
 205
 206 -       unsigned long flags;
 207 -       /* pointer to per-cpu ENETC lock for register access issue WA */
 208 -       spinlock_t *lock;
 209 -
 210 -       lock = this_cpu_ptr(&enetc_gregs);
 211 -
 212         cleaned_cnt = enetc_bd_unused(rx_ring);
 213         /* next descriptor to process */
 214         i = rx_ring->next_to_clean;
 215 @@ -683,7 +656,7 @@ static int enetc_clean_rx_ring(struct en
 216                 u32 bd_status;
 217                 u16 size;
 218
 219 -               spin_lock_irqsave(lock, flags);
 220 +               read_lock(&enetc_mdio_lock);
 221
 222                 if (cleaned_cnt >= ENETC_RXBD_BUNDLE) {
 223                         int count = enetc_refill_rx_ring(rx_ring, cleaned_cnt);
 224 @@ -694,7 +667,7 @@ static int enetc_clean_rx_ring(struct en
 225                 rxbd = ENETC_RXBD(*rx_ring, i);
 226                 bd_status = le32_to_cpu(rxbd->r.lstatus);
 227                 if (!bd_status) {
 228 -                       spin_unlock_irqrestore(lock, flags);
 229 +                       read_unlock(&enetc_mdio_lock);
 230                         break;
 231                 }
 232
 233 @@ -703,7 +676,7 @@ static int enetc_clean_rx_ring(struct en
 234                 size = le16_to_cpu(rxbd->r.buf_len);
 235                 skb = enetc_map_rx_buff_to_skb(rx_ring, i, size);
 236                 if (!skb) {
 237 -                       spin_unlock_irqrestore(lock, flags);
 238 +                       read_unlock(&enetc_mdio_lock);
 239                         break;
 240                 }
 241
 242 @@ -719,7 +692,7 @@ static int enetc_clean_rx_ring(struct en
 243
 244                 if (unlikely(bd_status &
 245                              ENETC_RXBD_LSTATUS(ENETC_RXBD_ERR_MASK))) {
 246 -                       spin_unlock_irqrestore(lock, flags);
 247 +                       read_unlock(&enetc_mdio_lock);
 248                         dev_kfree_skb(skb);
 249                         while (!(bd_status & ENETC_RXBD_LSTATUS_F)) {
 250                                 dma_rmb();
 251 @@ -763,7 +736,7 @@ static int enetc_clean_rx_ring(struct en
 252
 253                 enetc_process_skb(rx_ring, skb);
 254
 255 -               spin_unlock_irqrestore(lock, flags);
 256 +               read_unlock(&enetc_mdio_lock);
 257
 258                 napi_gro_receive(napi, skb);
 259
 260 --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
 261 +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
 262 @@ -331,7 +331,7 @@ struct enetc_hw {
 263  #define enetc_wr_reg(reg, val) enetc_wr_reg_wa((reg), (val))
 264
 265  /* accessors for data-path, due to MDIO issue on LS1028 these should be called
 266 - * only under enetc_gregs per-cpu lock
 267 + * only under the rwlock_t enetc_mdio_lock
 268   */
 269  #define enetc_rd_reg_hot(reg)  ioread32((reg))
 270  #define enetc_wr_reg_hot(reg, val)     iowrite32((val), (reg))
 271 @@ -354,90 +354,45 @@ static inline u64 enetc_rd_reg64(void __
 272  }
 273  #endif
 274
 275 -extern DEFINE_PER_CPU(spinlock_t, enetc_gregs);
 276 +extern rwlock_t enetc_mdio_lock;
 277
 278  static inline u32 enetc_rd_reg_wa(void *reg)
 279  {
 280 -       unsigned long flags;
 281 -       /* pointer to per-cpu ENETC lock for register access issue WA */
 282 -       spinlock_t *lock;
 283         u32 val;
 284
 285 -       lock = this_cpu_ptr(&enetc_gregs);
 286 -       spin_lock_irqsave(lock, flags);
 287 +       read_lock(&enetc_mdio_lock);
 288         val = ioread32(reg);
 289 -       spin_unlock_irqrestore(lock, flags);
 290 +       read_unlock(&enetc_mdio_lock);
 291
 292         return val;
 293  }
 294
 295  static inline void enetc_wr_reg_wa(void *reg, u32 val)
 296  {
 297 -       unsigned long flags;
 298 -       /* pointer to per-cpu ENETC lock for register access issue WA */
 299 -       spinlock_t *lock;
 300 -
 301 -       lock = this_cpu_ptr(&enetc_gregs);
 302 -       spin_lock_irqsave(lock, flags);
 303 +       read_lock(&enetc_mdio_lock);
 304         iowrite32(val, reg);
 305 -       spin_unlock_irqrestore(lock, flags);
 306 +       read_unlock(&enetc_mdio_lock);
 307  }
 308
 309 -/* NR_CPUS=256 in ARM64 defconfig and using it as array size triggers stack
 310 - * frame warnings for the functions below.  Use a custom define of 2 for now,
 311 - * LS1028 has just two cores.
 312 - */
 313 -#define ENETC_NR_CPU_LOCKS     2
 314 -
 315  static inline u32 enetc_rd_reg_wa_single(void *reg)
 316  {
 317 -       u32 val;
 318 -       int cpu;
 319 -       /* per-cpu ENETC lock array for register access issue WA */
 320 -       spinlock_t *lock[ENETC_NR_CPU_LOCKS];
 321         unsigned long flags;
 322 +       u32 val;
 323
 324 -       local_irq_save(flags);
 325 -       preempt_disable();
 326 -
 327 -       for_each_online_cpu(cpu) {
 328 -               lock[cpu] = per_cpu_ptr(&enetc_gregs, cpu);
 329 -               spin_lock(lock[cpu]);
 330 -       }
 331 -
 332 +       write_lock_irqsave(&enetc_mdio_lock, flags);
 333         val = ioread32(reg);
 334 -
 335 -       for_each_online_cpu(cpu)
 336 -               spin_unlock(lock[cpu]);
 337 -       local_irq_restore(flags);
 338 -
 339 -       preempt_enable();
 340 +       write_unlock_irqrestore(&enetc_mdio_lock, flags);
 341
 342         return val;
 343  }
 344
 345  static inline void enetc_wr_reg_wa_single(void *reg, u32 val)
 346  {
 347 -       int cpu;
 348 -       /* per-cpu ENETC lock array for register access issue WA */
 349 -       spinlock_t *lock[ENETC_NR_CPU_LOCKS];
 350         unsigned long flags;
 351
 352 -       local_irq_save(flags);
 353 -       preempt_disable();
 354 -
 355 -       for_each_online_cpu(cpu) {
 356 -               lock[cpu] = per_cpu_ptr(&enetc_gregs, cpu);
 357 -               spin_lock(lock[cpu]);
 358 -       }
 359 -
 360 +       write_lock_irqsave(&enetc_mdio_lock, flags);
 361         iowrite32(val, reg);
 362 -
 363 -       for_each_online_cpu(cpu)
 364 -               spin_unlock(lock[cpu]);
 365 -       local_irq_restore(flags);
 366 -
 367 -       preempt_enable();
 368 +       write_unlock_irqrestore(&enetc_mdio_lock, flags);
 369  }
 370
 371  #define enetc_rd(hw, off)              enetc_rd_reg((hw)->reg + (off))
 372 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
 373 +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
 374 @@ -1046,8 +1046,9 @@ static void enetc_pf_remove(struct pci_d
 375         enetc_pci_remove(pdev);
 376  }
 377
 378 -DEFINE_PER_CPU(spinlock_t, enetc_gregs);
 379 -EXPORT_PER_CPU_SYMBOL(enetc_gregs);
 380 +/* Lock for MDIO access errata on LS1028A */
 381 +DEFINE_RWLOCK(enetc_mdio_lock);
 382 +EXPORT_SYMBOL_GPL(enetc_mdio_lock);
 383
 384  static const struct pci_device_id enetc_pf_id_table[] = {
 385         { PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, ENETC_DEV_ID_PF) },