On Thu, Dec 09, 2021 at 10:46:12AM +0100, Horatiu Vultur wrote:
> This patch adds support for handling the interrupts generated by the
> analyzer. Currently, only the MAC table generates these interrupts.
> The MAC table will generate an interrupt whenever it learns or forgets
> an entry in the table. It is the SW responsibility figure out which
> entries were added/removed.
>
> Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> ---
> .../ethernet/microchip/lan966x/lan966x_mac.c | 244 ++++++++++++++++++
> .../ethernet/microchip/lan966x/lan966x_main.c | 23 ++
> .../ethernet/microchip/lan966x/lan966x_main.h | 6 +
> 3 files changed, 273 insertions(+)
>
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> index f6878b9f57ef..c01ab01bffbf 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_mac.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> +#include <net/switchdev.h>
> #include "lan966x_main.h"
>
> #define LAN966X_MAC_COLUMNS 4
> @@ -13,6 +14,23 @@
> #define MACACCESS_CMD_WRITE 7
> #define MACACCESS_CMD_SYNC_GET_NEXT 8
>
> +#define LAN966X_MAC_INVALID_ROW -1
> +
> +struct lan966x_mac_entry {
> + struct list_head list;
> + unsigned char mac[ETH_ALEN] __aligned(2);
> + u16 vid;
> + u16 port_index;
> + int row;
> +};
> +
> +struct lan966x_mac_raw_entry {
> + u32 mach;
> + u32 macl;
> + u32 maca;
> + bool process;
> +};
> +
> static int lan966x_mac_get_status(struct lan966x *lan966x)
> {
> return lan_rd(lan966x, ANA_MACACCESS);
> @@ -98,4 +116,230 @@ void lan966x_mac_init(struct lan966x *lan966x)
> /* Clear the MAC table */
> lan_wr(MACACCESS_CMD_INIT, lan966x, ANA_MACACCESS);
> lan966x_mac_wait_for_completion(lan966x);
> +
> + spin_lock_init(&lan966x->mac_lock);
> + INIT_LIST_HEAD(&lan966x->mac_entries);
> +}
> +
> +static struct lan966x_mac_entry *lan966x_mac_alloc_entry(const unsigned char *mac,
> + u16 vid, u16 port_index)
> +{
> + struct lan966x_mac_entry *mac_entry;
> +
> + mac_entry = kzalloc(sizeof(*mac_entry), GFP_KERNEL);
> + if (!mac_entry)
> + return NULL;
> +
> + memcpy(mac_entry->mac, mac, ETH_ALEN);
> + mac_entry->vid = vid;
> + mac_entry->port_index = port_index;
> + mac_entry->row = LAN966X_MAC_INVALID_ROW;
> + return mac_entry;
> +}
> +
> +static void lan966x_fdb_call_notifiers(enum switchdev_notifier_type type,
> + const char *mac, u16 vid,
> + struct net_device *dev)
> +{
> + struct switchdev_notifier_fdb_info info = { 0 };
> +
> + info.addr = mac;
> + info.vid = vid;
> + info.offloaded = true;
> + call_switchdev_notifiers(type, dev, &info.info, NULL);
> +}
> +
> +void lan966x_mac_purge_entries(struct lan966x *lan966x)
> +{
> + struct lan966x_mac_entry *mac_entry, *tmp;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&lan966x->mac_lock, flags);
I hope I'm not wrong, but you are only using this spinlock to serialize
access to the list, which isn't accessed from hardirq context anywhere
(the irq is threaded). So spin_lock_irqsave could simply be spin_lock.
Unless...
> + list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries,
> + list) {
> + lan966x_mac_forget(lan966x, mac_entry->mac, mac_entry->vid,
> + ENTRYTYPE_LOCKED);
Does this generate a MAC table interrupt?
> +
> + list_del(&mac_entry->list);
> + kfree(mac_entry);
> + }
> + spin_unlock_irqrestore(&lan966x->mac_lock, flags);
> +}
> +
> +static void lan966x_mac_notifiers(struct lan966x *lan966x,
> + enum switchdev_notifier_type type,
> + unsigned char *mac, u32 vid,
> + struct net_device *dev)
> +{
> + rtnl_lock();
> + lan966x_fdb_call_notifiers(type, mac, vid, dev);
> + rtnl_unlock();
> +}
> +
> +static void lan966x_mac_process_raw_entry(struct lan966x_mac_raw_entry *raw_entry,
> + u8 *mac, u16 *vid, u32 *dest_idx)
> +{
> + mac[0] = (raw_entry->mach >> 8) & 0xff;
> + mac[1] = (raw_entry->mach >> 0) & 0xff;
> + mac[2] = (raw_entry->macl >> 24) & 0xff;
> + mac[3] = (raw_entry->macl >> 16) & 0xff;
> + mac[4] = (raw_entry->macl >> 8) & 0xff;
> + mac[5] = (raw_entry->macl >> 0) & 0xff;
> +
> + *vid = (raw_entry->mach >> 16) & 0xfff;
> + *dest_idx = ANA_MACACCESS_DEST_IDX_GET(raw_entry->maca);
> +}
> +
> +static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
> + struct lan966x_mac_raw_entry *raw_entries)
> +{
> + struct lan966x_mac_entry *mac_entry, *tmp;
> + char mac[ETH_ALEN] __aligned(2);
unsigned char
> + unsigned long flags;
> + u32 dest_idx;
> + u32 column;
> + u16 vid;
> +
> + spin_lock_irqsave(&lan966x->mac_lock, flags);
> + list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries, list) {
> + bool found = false;
> +
> + if (mac_entry->row != row)
> + continue;
When the MAC table gets large, you could consider keeping separate lists
per row. This way you can avoid traversing a list of elements you're
sure you don't care about.
> +
> + for (column = 0; column < LAN966X_MAC_COLUMNS; ++column) {
> + /* All the valid entries are at the start of the row,
> + * so when get one invalid entry it can just skip the
> + * rest of the columns
> + */
> + if (!ANA_MACACCESS_VALID_GET(raw_entries[column].maca))
> + break;
> +
> + lan966x_mac_process_raw_entry(&raw_entries[column],
> + mac, &vid, &dest_idx);
> + WARN_ON(dest_idx > lan966x->num_phys_ports);
> +
> + /* If the entry in SW is found, then there is nothing
> + * to do
> + */
> + if (mac_entry->vid == vid &&
> + ether_addr_equal(mac_entry->mac, mac) &&
> + mac_entry->port_index == dest_idx) {
> + raw_entries[column].process = true;
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + /* Notify the bridge that the entry doesn't exist
> + * anymore in the HW and remmove the entry from the SW
s/remmove/remove/
> + * list
> + */
> + lan966x_mac_notifiers(lan966x, SWITCHDEV_FDB_DEL_TO_BRIDGE,
> + mac_entry->mac, mac_entry->vid,
> + lan966x->ports[mac_entry->port_index]->dev);
> +
> + list_del(&mac_entry->list);
> + kfree(mac_entry);
> + }
> + }
> + spin_unlock_irqrestore(&lan966x->mac_lock, flags);
> +
> + /* Now go to the list of columns and see if any entry was not in the SW
> + * list, then that means that the entry is new so it needs to notify the
> + * bridge.
> + */
> + for (column = 0; column < LAN966X_MAC_COLUMNS; ++column) {
> + /* All the valid entries are at the start of the row, so when
> + * get one invalid entry it can just skip the rest of the columns
> + */
> + if (!ANA_MACACCESS_VALID_GET(raw_entries[column].maca))
> + break;
> +
> + /* If the entry already exists then don't do anything */
> + if (raw_entries[column].process)
s/process/processed/
> + continue;
> +
> + lan966x_mac_process_raw_entry(&raw_entries[column],
> + mac, &vid, &dest_idx);
> + WARN_ON(dest_idx > lan966x->num_phys_ports);
> +
> + mac_entry = lan966x_mac_alloc_entry(mac, vid, dest_idx);
> + if (!mac_entry)
> + return;
> +
> + mac_entry->row = row;
> +
> + spin_lock_irqsave(&lan966x->mac_lock, flags);
> + list_add_tail(&mac_entry->list, &lan966x->mac_entries);
> + spin_unlock_irqrestore(&lan966x->mac_lock, flags);
spin_lock_irqsave shouldn't be necessary from an irq handler.
> +
> + lan966x_mac_notifiers(lan966x, SWITCHDEV_FDB_ADD_TO_BRIDGE,
> + mac, vid, lan966x->ports[dest_idx]->dev);
> + }
> +}
> +
> +irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x)
> +{
> + struct lan966x_mac_raw_entry entry[LAN966X_MAC_COLUMNS] = { 0 };
> + u32 index, column;
> + bool stop = true;
> + u32 val;
> +
> + /* Check if the mac table triggered this, if not just bail out */
> + if (!(ANA_ANAINTR_INTR_GET(lan_rd(lan966x, ANA_ANAINTR))))
> + return IRQ_NONE;
The interrupt isn't shared, so if we enter this condition, it means the
analyzer block generated it, just not the MAC table portion of it.
If we return IRQ_NONE there will be an IRQ storm because that condition
will never go away. Could we ack the interrupt and return IRQ_HANDLED?
> +
> + /* Start the scan from 0, 0 */
> + lan_wr(ANA_MACTINDX_M_INDEX_SET(0) |
> + ANA_MACTINDX_BUCKET_SET(0),
> + lan966x, ANA_MACTINDX);
> +
> + while (1) {
> + lan_rmw(ANA_MACACCESS_MAC_TABLE_CMD_SET(MACACCESS_CMD_SYNC_GET_NEXT),
> + ANA_MACACCESS_MAC_TABLE_CMD,
> + lan966x, ANA_MACACCESS);
> + lan966x_mac_wait_for_completion(lan966x);
> +
> + val = lan_rd(lan966x, ANA_MACTINDX);
> + index = ANA_MACTINDX_M_INDEX_GET(val);
> + column = ANA_MACTINDX_BUCKET_GET(val);
> +
> + /* The SYNC-GET-NEXT returns all the entries(4) in a row in
> + * which is suffered a change. By change it means that new entry
> + * was added or an entry was removed because of ageing.
> + * It would return all the columns for that row. And after that
> + * it would return the next row The stop conditions of the
> + * SYNC-GET-NEXT is when it reaches 'directly' to row 0
> + * column 3. So if SYNC-GET-NEXT returns row 0 and column 0
> + * then it is required to continue to read more even if it
> + * reaches row 0 and column 3.
> + */
> + if (index == 0 && column == 0)
> + stop = false;
> +
> + if (column == LAN966X_MAC_COLUMNS - 1 &&
> + index == 0 && stop)
> + break;
> +
> + entry[column].mach = lan_rd(lan966x, ANA_MACHDATA);
> + entry[column].macl = lan_rd(lan966x, ANA_MACLDATA);
> + entry[column].maca = lan_rd(lan966x, ANA_MACACCESS);
> +
> + /* Once all the columns are read process them */
> + if (column == LAN966X_MAC_COLUMNS - 1) {
> + lan966x_mac_irq_process(lan966x, index, entry);
> + /* A row was processed so it is safe to assume that the
> + * next row/column can be the stop condition
> + */
> + stop = true;
> + }
> + }
> +
> + lan_rmw(ANA_ANAINTR_INTR_SET(0),
> + ANA_ANAINTR_INTR,
> + lan966x, ANA_ANAINTR);
> +
> + return IRQ_HANDLED;
> }
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 101c1f005baf..7c6d6293611a 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -527,6 +527,13 @@ static irqreturn_t lan966x_xtr_irq_handler(int irq, void *args)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t lan966x_ana_irq_handler(int irq, void *args)
> +{
> + struct lan966x *lan966x = args;
> +
> + return lan966x_mac_irq_handler(lan966x);
> +}
> +
> static void lan966x_cleanup_ports(struct lan966x *lan966x)
> {
> struct lan966x_port *port;
> @@ -554,6 +561,11 @@ static void lan966x_cleanup_ports(struct lan966x *lan966x)
>
> disable_irq(lan966x->xtr_irq);
> lan966x->xtr_irq = -ENXIO;
> +
> + if (lan966x->ana_irq) {
> + disable_irq(lan966x->ana_irq);
> + lan966x->ana_irq = -ENXIO;
> + }
> }
>
> static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> @@ -870,6 +882,15 @@ static int lan966x_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> + lan966x->ana_irq = platform_get_irq_byname(pdev, "ana");
> + if (lan966x->ana_irq) {
> + err = devm_request_threaded_irq(&pdev->dev, lan966x->ana_irq, NULL,
> + lan966x_ana_irq_handler, IRQF_ONESHOT,
> + "ana irq", lan966x);
> + if (err)
> + return dev_err_probe(&pdev->dev, err, "Unable to use ana irq");
> + }
> +
> /* init switch */
> lan966x_init(lan966x);
> lan966x_stats_init(lan966x);
> @@ -923,6 +944,8 @@ static int lan966x_remove(struct platform_device *pdev)
> destroy_workqueue(lan966x->stats_queue);
> mutex_destroy(&lan966x->stats_lock);
>
> + lan966x_mac_purge_entries(lan966x);
> +
> return 0;
> }
>
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index 7e5a3b6f168d..ba548d65b58a 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -75,6 +75,9 @@ struct lan966x {
>
> u8 base_mac[ETH_ALEN];
>
> + struct list_head mac_entries;
> + spinlock_t mac_lock; /* lock for mac_entries list */
> +
> /* stats */
> const struct lan966x_stat_layout *stats_layout;
> u32 num_stats;
> @@ -87,6 +90,7 @@ struct lan966x {
>
> /* interrupts */
> int xtr_irq;
> + int ana_irq;
> };
>
> struct lan966x_port_config {
> @@ -141,6 +145,8 @@ int lan966x_mac_forget(struct lan966x *lan966x,
> int lan966x_mac_cpu_learn(struct lan966x *lan966x, const char *addr, u16 vid);
> int lan966x_mac_cpu_forget(struct lan966x *lan966x, const char *addr, u16 vid);
> void lan966x_mac_init(struct lan966x *lan966x);
> +void lan966x_mac_purge_entries(struct lan966x *lan966x);
> +irqreturn_t lan966x_mac_irq_handler(struct lan966x *lan966x);
>
> static inline void __iomem *lan_addr(void __iomem *base[],
> int id, int tinst, int tcnt,
> --
> 2.33.0
>
