Re: [PATCH net-next v3 3/6] net: lan966x: add support for interrupts from analyzer
From: Horatiu Vultur
Date: Thu Dec 09 2021 - 10:36:43 EST
The 12/09/2021 11:47, Vladimir Oltean wrote:
>
> 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...
It should be OK to use spin_lock. I have chosen spin_lock_irq because
that is the safest way in case it would be extended.
>
> > + 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?
It doesn't.
>
> > +
> > + 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.
Before I will change anything, I should run some measurements and see
some results.
>
> > +
> > + 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?
Actually, there is a wrong comment, it doesn't check if the MAC table
triggered it, but it checks if the analyzer generated it. So it can be
removed and then always return IRQ_HANDLER.
>
> > +
> > + /* 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
> >
--
/Horatiu