Message-ID: <20211209114747.pg73qojm2gexlo33@skbuf>
Date:   Thu, 9 Dec 2021 11:47:47 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH net-next v3 3/6] net: lan966x: add support for interrupts
 from analyzer

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@...rochip.com>
> ---
>  .../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
>

Powered by blists - more mailing lists