projects / openwrt / staging / ldir.git / blob
? search:


cb164d4ffb18f3d2b86c4c8e952595ab3c353f6f
[openwrt/staging/ldir.git] /
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 @@ -1076,8 +1076,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) },
Kevin D-Bs staging tree