History

Merge branch 'net-lan966x-fix-issues-with-mac-table'

Horatiu Vultur says: ==================== net: lan966x: Fix issues with MAC table The patch series fixes 2 issues: - when an entry was forgotten the irq thread was holding a spin lock and then was talking also rtnl_lock. - the access to the HW MAC table is indirect, so the access to the HW MAC table was not synchronized, which means that there could be race conditions. ====================

Jakub Kicinski committed 4 years ago

Commit b6224a32 parents c32349f 675c8071 file · +80 −32Browse files

Unified · Split

drivers/net/ethernet/microchip/lan966x/lan966x_mac.c+80−32
@@ -75,6 +75,9 @@ static int __lan966x_mac_learn(struct lan966x *lan966x, int pgid, · 74 unmodified lines
7575 unsigned int vid,
7676 enum macaccess_entry_type type)
7777 {
78+ int ret;
79+
80+ spin_lock(&lan966x->mac_lock);
7881 lan966x_mac_select(lan966x, mac, vid);
7982
8083 /* Issue a write command */
@@ -86,7 +89,10 @@ static int __lan966x_mac_learn(struct lan966x *lan966x, int pgid, · 5 unmodified lines
8689 ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_LEARN),
8790 lan966x, ANA_MACACCESS);
8891
89− return lan966x_mac_wait_for_completion(lan966x);
92+ ret = lan966x_mac_wait_for_completion(lan966x);
93+ spin_unlock(&lan966x->mac_lock);
94+
95+ return ret;
9096 }
9197
9298 /* The mask of the front ports is encoded inside the mac parameter via a call
@@ -113,11 +119,13 @@ int lan966x_mac_learn(struct lan966x *lan966x, int port, · 20 unmodified lines
113119 return __lan966x_mac_learn(lan966x, port, false, mac, vid, type);
114120 }
115121
116−int lan966x_mac_forget(struct lan966x *lan966x,
117− const unsigned char mac[ETH_ALEN],
118− unsigned int vid,
119− enum macaccess_entry_type type)
122+static int lan966x_mac_forget_locked(struct lan966x *lan966x,
123+ const unsigned char mac[ETH_ALEN],
124+ unsigned int vid,
125+ enum macaccess_entry_type type)
120126 {
127+ lockdep_assert_held(&lan966x->mac_lock);
128+
121129 lan966x_mac_select(lan966x, mac, vid);
122130
123131 /* Issue a forget command */
@@ -128,6 +136,20 @@ int lan966x_mac_forget(struct lan966x *lan966x, · 4 unmodified lines
128136 return lan966x_mac_wait_for_completion(lan966x);
129137 }
130138
139+int lan966x_mac_forget(struct lan966x *lan966x,
140+ const unsigned char mac[ETH_ALEN],
141+ unsigned int vid,
142+ enum macaccess_entry_type type)
143+{
144+ int ret;
145+
146+ spin_lock(&lan966x->mac_lock);
147+ ret = lan966x_mac_forget_locked(lan966x, mac, vid, type);
148+ spin_unlock(&lan966x->mac_lock);
149+
150+ return ret;
151+}
152+
131153 int lan966x_mac_cpu_learn(struct lan966x *lan966x, const char *addr, u16 vid)
132154 {
133155 return lan966x_mac_learn(lan966x, PGID_CPU, addr, vid, ENTRYTYPE_LOCKED);
@@ -161,7 +183,7 @@ static struct lan966x_mac_entry *lan966x_mac_alloc_entry(const unsigned char *ma · 27 unmodified lines
161183 {
162184 struct lan966x_mac_entry *mac_entry;
163185
164− mac_entry = kzalloc(sizeof(*mac_entry), GFP_KERNEL);
186+ mac_entry = kzalloc(sizeof(*mac_entry), GFP_ATOMIC);
165187 if (!mac_entry)
166188 return NULL;
167189
@@ -179,7 +201,6 @@ static struct lan966x_mac_entry *lan966x_mac_find_entry(struct lan966x *lan966x, · 11 unmodified lines
179201 struct lan966x_mac_entry *res = NULL;
180202 struct lan966x_mac_entry *mac_entry;
181203
182− spin_lock(&lan966x->mac_lock);
183204 list_for_each_entry(mac_entry, &lan966x->mac_entries, list) {
184205 if (mac_entry->vid == vid &&
185206 ether_addr_equal(mac, mac_entry->mac) &&
@@ -188,7 +209,6 @@ static struct lan966x_mac_entry *lan966x_mac_find_entry(struct lan966x *lan966x, · 2 unmodified lines
188209 break;
189210 }
190211 }
191− spin_unlock(&lan966x->mac_lock);
192212
193213 return res;
194214 }
@@ -231,8 +251,11 @@ int lan966x_mac_add_entry(struct lan966x *lan966x, struct lan966x_port *port, · 36 unmodified lines
231251 {
232252 struct lan966x_mac_entry *mac_entry;
233253
234− if (lan966x_mac_lookup(lan966x, addr, vid, ENTRYTYPE_NORMAL))
254+ spin_lock(&lan966x->mac_lock);
255+ if (lan966x_mac_lookup(lan966x, addr, vid, ENTRYTYPE_NORMAL)) {
256+ spin_unlock(&lan966x->mac_lock);
235257 return 0;
258+ }
236259
237260 /* In case the entry already exists, don't add it again to SW,
238261 * just update HW, but we need to look in the actual HW because
@@ -241,21 +264,25 @@ int lan966x_mac_add_entry(struct lan966x *lan966x, struct lan966x_port *port, · 2 unmodified lines
241264 * add the entry but without the extern_learn flag.
242265 */
243266 mac_entry = lan966x_mac_find_entry(lan966x, addr, vid, port->chip_port);
244− if (mac_entry)
245− return lan966x_mac_learn(lan966x, port->chip_port,
246− addr, vid, ENTRYTYPE_LOCKED);
267+ if (mac_entry) {
268+ spin_unlock(&lan966x->mac_lock);
269+ goto mac_learn;
270+ }
247271
248272 mac_entry = lan966x_mac_alloc_entry(addr, vid, port->chip_port);
249− if (!mac_entry)
273+ if (!mac_entry) {
274+ spin_unlock(&lan966x->mac_lock);
250275 return -ENOMEM;
276+ }
251277
252− spin_lock(&lan966x->mac_lock);
253278 list_add_tail(&mac_entry->list, &lan966x->mac_entries);
254279 spin_unlock(&lan966x->mac_lock);
255280
256− lan966x_mac_learn(lan966x, port->chip_port, addr, vid, ENTRYTYPE_LOCKED);
257281 lan966x_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED, addr, vid, port->dev);
258282
283+mac_learn:
284+ lan966x_mac_learn(lan966x, port->chip_port, addr, vid, ENTRYTYPE_LOCKED);
285+
259286 return 0;
260287 }
261288
@@ -269,8 +296,9 @@ int lan966x_mac_del_entry(struct lan966x *lan966x, const unsigned char *addr, · 7 unmodified lines
269296 list) {
270297 if (mac_entry->vid == vid &&
271298 ether_addr_equal(addr, mac_entry->mac)) {
272− lan966x_mac_forget(lan966x, mac_entry->mac, mac_entry->vid,
273− ENTRYTYPE_LOCKED);
299+ lan966x_mac_forget_locked(lan966x, mac_entry->mac,
300+ mac_entry->vid,
301+ ENTRYTYPE_LOCKED);
274302
275303 list_del(&mac_entry->list);
276304 kfree(mac_entry);
@@ -288,8 +316,8 @@ void lan966x_mac_purge_entries(struct lan966x *lan966x) · 11 unmodified lines
288316 spin_lock(&lan966x->mac_lock);
289317 list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries,
290318 list) {
291− lan966x_mac_forget(lan966x, mac_entry->mac, mac_entry->vid,
292− ENTRYTYPE_LOCKED);
319+ lan966x_mac_forget_locked(lan966x, mac_entry->mac,
320+ mac_entry->vid, ENTRYTYPE_LOCKED);
293321
294322 list_del(&mac_entry->list);
295323 kfree(mac_entry);
@@ -325,10 +353,13 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row, · 29 unmodified lines
325353 {
326354 struct lan966x_mac_entry *mac_entry, *tmp;
327355 unsigned char mac[ETH_ALEN] __aligned(2);
356+ struct list_head mac_deleted_entries;
328357 u32 dest_idx;
329358 u32 column;
330359 u16 vid;
331360
361+ INIT_LIST_HEAD(&mac_deleted_entries);
362+
332363 spin_lock(&lan966x->mac_lock);
333364 list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries, list) {
334365 bool found = false;
@@ -362,20 +393,26 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row, · 27 unmodified lines
362393 }
363394
364395 if (!found) {
365− /* Notify the bridge that the entry doesn't exist
366− * anymore in the HW and remove the entry from the SW
367− * list
368− */
369− lan966x_mac_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
370− mac_entry->mac, mac_entry->vid,
371− lan966x->ports[mac_entry->port_index]->dev);
372−
373396 list_del(&mac_entry->list);
374− kfree(mac_entry);
397+ /* Move the entry from SW list to a tmp list such that
398+ * it would be deleted later
399+ */
400+ list_add_tail(&mac_entry->list, &mac_deleted_entries);
375401 }
376402 }
377403 spin_unlock(&lan966x->mac_lock);
378404
405+ list_for_each_entry_safe(mac_entry, tmp, &mac_deleted_entries, list) {
406+ /* Notify the bridge that the entry doesn't exist
407+ * anymore in the HW
408+ */
409+ lan966x_mac_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
410+ mac_entry->mac, mac_entry->vid,
411+ lan966x->ports[mac_entry->port_index]->dev);
412+ list_del(&mac_entry->list);
413+ kfree(mac_entry);
414+ }
415+
379416 /* Now go to the list of columns and see if any entry was not in the SW
380417 * list, then that means that the entry is new so it needs to notify the
381418 * bridge.
@@ -396,13 +433,20 @@ static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row, · 14 unmodified lines
396433 if (WARN_ON(dest_idx >= lan966x->num_phys_ports))
397434 continue;
398435
436+ spin_lock(&lan966x->mac_lock);
437+ mac_entry = lan966x_mac_find_entry(lan966x, mac, vid, dest_idx);
438+ if (mac_entry) {
439+ spin_unlock(&lan966x->mac_lock);
440+ continue;
441+ }
442+
399443 mac_entry = lan966x_mac_alloc_entry(mac, vid, dest_idx);
400− if (!mac_entry)
444+ if (!mac_entry) {
445+ spin_unlock(&lan966x->mac_lock);
401446 return;
447+ }
402448
403449 mac_entry->row = row;
404−
405− spin_lock(&lan966x->mac_lock);
406450 list_add_tail(&mac_entry->list, &lan966x->mac_entries);
407451 spin_unlock(&lan966x->mac_lock);
408452
@@ -424,6 +468,7 @@ irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x) · 15 unmodified lines
424468 lan966x, ANA_MACTINDX);
425469
426470 while (1) {
471+ spin_lock(&lan966x->mac_lock);
427472 lan_rmw(ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_SYNC_GET_NEXT),
428473 ANA_MACACCESS_MAC_TABLE_CMD,
429474 lan966x, ANA_MACACCESS);
@@ -447,12 +492,15 @@ irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x) · 17 unmodified lines
447492 stop = false;
448493
449494 if (column == LAN966X_MAC_COLUMNS - 1 &&
450− index == 0 && stop)
495+ index == 0 && stop) {
496+ spin_unlock(&lan966x->mac_lock);
451497 break;
498+ }
452499
453500 entry[column].mach = lan_rd(lan966x, ANA_MACHDATA);
454501 entry[column].macl = lan_rd(lan966x, ANA_MACLDATA);
455502 entry[column].maca = lan_rd(lan966x, ANA_MACACCESS);
503+ spin_unlock(&lan966x->mac_lock);
456504
457505 /* Once all the columns are read process them */
458506 if (column == LAN966X_MAC_COLUMNS - 1) {