[RFC PATCH 0/3] NCSI VLAN Filtering Support

[RFC PATCH 0/3] NCSI VLAN Filtering Support

[Samuel Mendoza-Jonas]

This RFC series adds VLAN filtering support for NCSI-capable network devices.
This supports adding and removing VLAN tags up to the limit of the actual
NCSI channel. In it's present form the NCSI driver will default to the
"Accept specific VLAN-tagged traffic as well as non-tagged traffic" mode.

This is still a bit rough, but sending out to the OpenBMC list for some
eyeballing.

Samuel Mendoza-Jonas (3):
  ftgmac: Include NETIF_F_HW_VLAN_CTAG_FILTER in features
  net/ncsi: Fix several packet definitions
  net/ncsi: Configure VLAN tag filter

 drivers/net/ethernet/faraday/ftgmac100.c |   5 +
 include/net/ncsi.h                       |   2 +
 net/ncsi/internal.h                      |  11 ++
 net/ncsi/ncsi-cmd.c                      |  10 +-
 net/ncsi/ncsi-manage.c                   | 257 ++++++++++++++++++++++++++++++-
 net/ncsi/ncsi-pkt.h                      |   2 +-
 net/ncsi/ncsi-rsp.c                      |  13 +-
 7 files changed, 290 insertions(+), 10 deletions(-)

-- 
2.13.3

[RFC PATCH 1/3] ftgmac: Include NETIF_F_HW_VLAN_CTAG_FILTER in features

[Samuel Mendoza-Jonas]

This is required for the VLAN core to call the add/kill callback for
VLAN IDs.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 33b5c8eb9961..7c4d772287c7 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1837,6 +1837,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
 		NETIF_F_HW_VLAN_CTAG_TX;
 
+	if (priv->use_ncsi)
+		netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+
 	/* AST2400  doesn't have working HW checksum generation */
 	if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
 		netdev->hw_features &= ~NETIF_F_HW_CSUM;
-- 
2.13.3

[RFC PATCH 2/3] net/ncsi: Fix several packet definitions

[Samuel Mendoza-Jonas]

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 net/ncsi/ncsi-cmd.c | 10 +++++-----
 net/ncsi/ncsi-pkt.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index db7083bfd476..1fec9fda7f60 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -146,9 +146,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
 
 	cmd = (struct ncsi_cmd_svf_pkt *)skb_put(skb, sizeof(*cmd));
 	memset(cmd, 0, sizeof(*cmd));
-	cmd->vlan = htons(nca->words[0]);
-	cmd->index = nca->bytes[2];
-	cmd->enable = nca->bytes[3];
+	cmd->vlan = htons(nca->words[1]);
+	cmd->index = nca->bytes[6];
+	cmd->enable = nca->bytes[7];
 	ncsi_cmd_build_header(&cmd->cmd.common, nca);
 
 	return 0;
@@ -161,7 +161,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
 
 	cmd = (struct ncsi_cmd_ev_pkt *)skb_put(skb, sizeof(*cmd));
 	memset(cmd, 0, sizeof(*cmd));
-	cmd->mode = nca->bytes[0];
+	cmd->mode = nca->bytes[3];
 	ncsi_cmd_build_header(&cmd->cmd.common, nca);
 
 	return 0;
@@ -240,7 +240,7 @@ static struct ncsi_cmd_handler {
 	{ NCSI_PKT_CMD_AE,     8, ncsi_cmd_handler_ae      },
 	{ NCSI_PKT_CMD_SL,     8, ncsi_cmd_handler_sl      },
 	{ NCSI_PKT_CMD_GLS,    0, ncsi_cmd_handler_default },
-	{ NCSI_PKT_CMD_SVF,    4, ncsi_cmd_handler_svf     },
+	{ NCSI_PKT_CMD_SVF,    8, ncsi_cmd_handler_svf     },
 	{ NCSI_PKT_CMD_EV,     4, ncsi_cmd_handler_ev      },
 	{ NCSI_PKT_CMD_DV,     0, ncsi_cmd_handler_default },
 	{ NCSI_PKT_CMD_SMA,    8, ncsi_cmd_handler_sma     },
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 3ea49ed0a935..91b4b66438df 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -104,7 +104,7 @@ struct ncsi_cmd_svf_pkt {
 	unsigned char           index;     /* VLAN table index  */
 	unsigned char           enable;    /* Enable or disable */
 	__be32                  checksum;  /* Checksum          */
-	unsigned char           pad[14];
+	unsigned char           pad[18];
 };
 
 /* Enable VLAN */
-- 
2.13.3

[RFC PATCH 3/3] net/ncsi: Configure VLAN tag filter

[Samuel Mendoza-Jonas]

Make use of the ndo_vlan_rx_{add,kill}_vid callbacks to have the NCSI
stack process new VLAN tags and configure the channel VLAN filter
appropriately.
Several VLAN tags can be set and a "Set VLAN Filter" packet must be sent
for each one, meaning the ncsi_dev_state_config_svf state must be
repeated. An internal list of VLAN tags is maintained, and compared
against the current channel's ncsi_channel_filter in order to keep track
within the state. VLAN filters are removed in a similar manner, with the
introduction of the ncsi_dev_state_config_clear_vids state. The maximum
number of VLAN tag filters is determined by the "Get Capabilities"
response from the channel.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c |   2 +
 include/net/ncsi.h                       |   2 +
 net/ncsi/internal.h                      |  11 ++
 net/ncsi/ncsi-manage.c                   | 257 ++++++++++++++++++++++++++++++-
 net/ncsi/ncsi-rsp.c                      |  13 +-
 5 files changed, 281 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 7c4d772287c7..5a3661317026 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1623,6 +1623,8 @@ static const struct net_device_ops ftgmac100_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= ftgmac100_poll_controller,
 #endif
+	.ndo_vlan_rx_add_vid	= ncsi_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid	= ncsi_vlan_rx_kill_vid,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
diff --git a/include/net/ncsi.h b/include/net/ncsi.h
index 68680baac0fd..1f96af46df49 100644
--- a/include/net/ncsi.h
+++ b/include/net/ncsi.h
@@ -28,6 +28,8 @@ struct ncsi_dev {
 };
 
 #ifdef CONFIG_NET_NCSI
+int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid);
+int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid);
 struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 				   void (*notifier)(struct ncsi_dev *nd));
 int ncsi_start_dev(struct ncsi_dev *nd);
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 1308a56f2591..af3d636534ef 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -180,6 +180,7 @@ struct ncsi_channel {
 #define NCSI_CHANNEL_INACTIVE		1
 #define NCSI_CHANNEL_ACTIVE		2
 #define NCSI_CHANNEL_INVISIBLE		3
+	bool                        reconfigure_needed;
 	spinlock_t                  lock;	/* Protect filters etc */
 	struct ncsi_package         *package;
 	struct ncsi_channel_version version;
@@ -235,6 +236,9 @@ enum {
 	ncsi_dev_state_probe_dp,
 	ncsi_dev_state_config_sp	= 0x0301,
 	ncsi_dev_state_config_cis,
+	ncsi_dev_state_config_clear_vids,
+	ncsi_dev_state_config_svf,
+	ncsi_dev_state_config_ev,
 	ncsi_dev_state_config_sma,
 	ncsi_dev_state_config_ebf,
 #if IS_ENABLED(CONFIG_IPV6)
@@ -253,6 +257,12 @@ enum {
 	ncsi_dev_state_suspend_done
 };
 
+struct vlan_vid {
+	struct list_head list;
+	__be16 proto;
+	u16 vid;
+};
+
 struct ncsi_dev_priv {
 	struct ncsi_dev     ndev;            /* Associated NCSI device     */
 	unsigned int        flags;           /* NCSI device flags          */
@@ -276,6 +286,7 @@ struct ncsi_dev_priv {
 	struct work_struct  work;            /* For channel management     */
 	struct packet_type  ptype;           /* NCSI packet Rx handler     */
 	struct list_head    node;            /* Form NCSI device list      */
+	struct list_head    vlan_vids;       /* List of active VLAN IDs */
 };
 
 struct ncsi_cmd_arg {
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index a3bd5fa8ad09..f45f2a443e34 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -646,9 +646,13 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 	struct ncsi_package *np = ndp->active_package;
 	struct ncsi_channel *nc = ndp->active_channel;
 	struct ncsi_channel *hot_nc = NULL;
+	struct ncsi_channel_filter *ncf;
 	struct ncsi_cmd_arg nca;
+	struct vlan_vid *vlan;
 	unsigned char index;
 	unsigned long flags;
+	int vlan_index;
+	bool clear;
 	int ret;
 
 	nca.ndp = ndp;
@@ -683,8 +687,11 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 		if (ret)
 			goto error;
 
-		nd->state = ncsi_dev_state_config_sma;
+		nd->state = ncsi_dev_state_config_clear_vids;
 		break;
+	case ncsi_dev_state_config_clear_vids:
+	case ncsi_dev_state_config_svf:
+	case ncsi_dev_state_config_ev:
 	case ncsi_dev_state_config_sma:
 	case ncsi_dev_state_config_ebf:
 #if IS_ENABLED(CONFIG_IPV6)
@@ -699,11 +706,93 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 		nca.package = np->id;
 		nca.channel = nc->id;
 
+		/* Clear any active filters on the channel before setting */
+		if (nd->state == ncsi_dev_state_config_clear_vids) {
+			clear = false;
+			vlan_index = -1;
+			spin_lock_irqsave(&nc->lock, flags);
+			ncf = nc->filters[NCSI_FILTER_VLAN];
+			while ((vlan_index = find_next_bit((void *)&ncf->bitmap, ncf->total, vlan_index + 1))
+			       < ncf->total) {
+				clear = true;
+				// TODO retrieve the vid that was set
+				netdev_printk(KERN_DEBUG, dev, "ncsi: removed vlan tag at index %d\n",
+						vlan_index);
+				ncsi_remove_filter(nc, NCSI_FILTER_VLAN, vlan_index);
+				break;
+			}
+			spin_unlock_irqrestore(&nc->lock, flags);
+
+			if (clear) {
+				nca.type = NCSI_PKT_CMD_SVF;
+				nca.words[1] = 0; // TODO
+				/* HW filter index starts at 1 */
+				nca.bytes[6] = vlan_index + 1;
+				nca.bytes[7] = 0x00;
+				/* Repeat */
+				nd->state = ncsi_dev_state_config_clear_vids;
+			} else {
+				nd->state = ncsi_dev_state_config_svf;
+				schedule_work(&ndp->work);
+				break;
+			}
+		/* Add known VLAN tags to the filter */
+		} else if (nd->state == ncsi_dev_state_config_svf) {
+			vlan = NULL;
+			vlan_index = 0;
+			list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
+				vlan_index = ncsi_find_filter(nc, NCSI_FILTER_VLAN, &vlan->vid);
+				if (vlan_index < 0) {
+					/* New tag to add */
+					netdev_info(dev, "new vlan id to set: %u\n", vlan->vid);
+					break;
+				} else
+					netdev_printk(KERN_DEBUG, dev, "vid %u already at filter pos %d\n",
+							vlan->vid, vlan_index);
+			}
+
+			if (vlan && vlan_index == -ENOENT) {
+				vlan_index = ncsi_add_filter(nc, NCSI_FILTER_VLAN, &vlan->vid);
+				if (vlan_index < 0) {
+					netdev_err(dev, "Failed to add new VLAN tag, error %d%s\n",
+							vlan_index,
+							vlan_index == -ENOSPC ? " (vid limit)" : "");
+					nd->state = ncsi_dev_state_config_ev;
+					schedule_work(&ndp->work);
+					break;
+				}
+
+				netdev_info(dev, "set vid %u in packet, index %u\n",
+						vlan->vid, vlan_index + 1);
+				nca.type = NCSI_PKT_CMD_SVF;
+				nca.words[1] = vlan->vid;
+				/* HW filter index starts at 1 */
+				nca.bytes[6] = vlan_index + 1;
+				nca.bytes[7] = 0x01;
+				/* Repeat */
+				nd->state = ncsi_dev_state_config_clear_vids;
+				nd->state = ncsi_dev_state_config_svf;
+			} else {
+				netdev_printk(KERN_DEBUG, dev, "no vlan ids left to set\n");
+				nd->state = ncsi_dev_state_config_ev;
+				schedule_work(&ndp->work);
+				break;
+			}
+		/* Enable/Disable the VLAN filter */
+		} else if (nd->state == ncsi_dev_state_config_ev) {
+			if (list_empty(&ndp->vlan_vids)) {
+				nca.type = NCSI_PKT_CMD_DV;
+			} else {
+				nca.type = NCSI_PKT_CMD_EV;
+				// TODO Filtering Type - set and forget or make configurable?
+				nca.bytes[3] = NCSI_CAP_VLAN_NO;
+			}
+			nd->state = ncsi_dev_state_config_sma;
+		} else if (nd->state == ncsi_dev_state_config_sma) {
 		/* Use first entry in unicast filter table. Note that
 		 * the MAC filter table starts from entry 1 instead of
 		 * 0.
 		 */
-		if (nd->state == ncsi_dev_state_config_sma) {
 			nca.type = NCSI_PKT_CMD_SMA;
 			for (index = 0; index < 6; index++)
 				nca.bytes[index] = dev->dev_addr[index];
@@ -751,6 +840,26 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 		break;
 	case ncsi_dev_state_config_done:
 		spin_lock_irqsave(&nc->lock, flags);
+		if (nc->reconfigure_needed) {
+			/*
+			 * This channel's configuration has been updated
+			 * part-way during the config state - start the
+			 * channel configuration over
+			 */
+			nc->reconfigure_needed = false;
+			nc->state = NCSI_CHANNEL_INVISIBLE;
+			spin_unlock_irqrestore(&nc->lock, flags);
+
+			spin_lock_irqsave(&ndp->lock, flags);
+			nc->state = NCSI_CHANNEL_INACTIVE;
+			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+			spin_unlock_irqrestore(&ndp->lock, flags);
+
+			netdev_info(dev, "Dirty NCSI channel state reset\n");
+			ncsi_process_next_channel(ndp);
+			break;
+		}
+
 		if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1) {
 			hot_nc = nc;
 			nc->state = NCSI_CHANNEL_ACTIVE;
@@ -1191,6 +1300,149 @@ static struct notifier_block ncsi_inet6addr_notifier = {
 };
 #endif /* CONFIG_IPV6 */
 
+static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
+{
+	struct ncsi_dev *nd = &ndp->ndev;
+	struct ncsi_channel *nc;
+	struct ncsi_package *np;
+	unsigned long flags;
+	unsigned int n = 0;
+
+	NCSI_FOR_EACH_PACKAGE(ndp, np) {
+		NCSI_FOR_EACH_CHANNEL(np, nc) {
+			spin_lock_irqsave(&nc->lock, flags);
+
+			/*
+			 * Channels may be busy - mark dirty instead of kicking if
+			 * a) not ACTIVE (configured)
+			 * b) in the channel_queue (to be configured)
+			 * c) it's ndev is in the config state
+			 */
+			if (nc->state != NCSI_CHANNEL_ACTIVE) {
+				if ((ndp->ndev.state & 0xff00) == ncsi_dev_state_config ||
+						!list_empty(&nc->link)) {
+					netdev_info(nd->dev, "nc %p marked dirty\n", nc);
+					nc->reconfigure_needed = true;
+				}
+				spin_unlock_irqrestore(&nc->lock, flags);
+				continue;
+			}
+
+			spin_unlock_irqrestore(&nc->lock, flags);
+
+			ncsi_stop_channel_monitor(nc);
+			spin_lock_irqsave(&nc->lock, flags);
+			nc->state = NCSI_CHANNEL_INVISIBLE;
+			spin_unlock_irqrestore(&nc->lock, flags);
+
+			spin_lock_irqsave(&ndp->lock, flags);
+			nc->state = NCSI_CHANNEL_INACTIVE;
+			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
+			spin_unlock_irqrestore(&ndp->lock, flags);
+
+			spin_unlock_irqrestore(&nc->lock, flags);
+
+			netdev_info(nd->dev, "Kicked channel %p\n", nc);
+			n++;
+		}
+	}
+
+	return n;
+}
+
+int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
+{
+	struct ncsi_channel_filter *ncf;
+	struct ncsi_dev_priv *ndp;
+	unsigned int n_vids = 0;
+	struct vlan_vid *vlan;
+	struct ncsi_dev *nd;
+	bool found = false;
+
+	if (vid == 0) {
+		netdev_info(dev, "Ignoring default vid 0\n");
+		return 0;
+	}
+
+	nd = ncsi_find_dev(dev);
+	if (!nd) {
+		netdev_warn(dev, "No net_device?\n");
+		return 0;
+	}
+
+	ndp = TO_NCSI_DEV_PRIV(nd);
+	ncf = ndp->hot_channel->filters[NCSI_FILTER_VLAN];
+
+	/* Add the VLAN id to our internal list */
+	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
+		n_vids++;
+		if (vlan->vid == vid) {
+			netdev_info(dev, "vid %u already registered\n", vid);
+			return 0;
+		}
+	}
+
+	if (n_vids >= ncf->total) {
+		netdev_info(dev, "NCSI Channel supports up to %u VLAN tags but %u are already set\n",
+				ncf->total, n_vids);
+		return -EINVAL;
+	}
+
+	vlan = kzalloc(sizeof(struct vlan_vid), GFP_KERNEL);
+	if (!vlan) {
+		netdev_err(dev, "could not alloc vlan_vid struct\n");
+		return -ENOMEM;
+	}
+
+	vlan->proto = proto;
+	vlan->vid = vid;
+	list_add_rcu(&vlan->list, &ndp->vlan_vids);
+
+	netdev_info(dev, "Added new vid %u\n", vid);
+
+	found = ncsi_kick_channels(ndp) != 0;
+
+	return found ? ncsi_process_next_channel(ndp) : 0;
+}
+
+int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
+{
+	struct vlan_vid *vlan, *tmp;
+	struct ncsi_dev_priv *ndp;
+	struct ncsi_dev *nd;
+	bool found = false;
+
+	if (vid == 0) {
+		netdev_info(dev, "Ignoring default vid 0\n");
+		return 0;
+	}
+
+	nd = ncsi_find_dev(dev);
+	if (!nd) {
+		netdev_warn(dev, "No net_device?\n");
+		return 0;
+	}
+
+	ndp = TO_NCSI_DEV_PRIV(nd);
+
+	/* Remove the VLAN id from our internal list */
+	list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
+		if (vlan->vid == vid) {
+			netdev_info(dev, "vid %u found, removing\n", vid);
+			list_del_rcu(&vlan->list);
+			found = true;
+		}
+
+	if (!found) {
+		printk(KERN_ERR "vid %u wasn't registered!\n", vid);
+		return -EINVAL;
+	}
+
+	found = ncsi_kick_channels(ndp) != 0;
+
+	return found ? ncsi_process_next_channel(ndp) : 0;
+}
+
 struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 				   void (*handler)(struct ncsi_dev *ndev))
 {
@@ -1215,6 +1467,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 	nd->handler = handler;
 	ndp->pending_req_num = 0;
 	INIT_LIST_HEAD(&ndp->channel_queue);
+	INIT_LIST_HEAD(&ndp->vlan_vids);
 	INIT_WORK(&ndp->work, ncsi_dev_work);
 
 	/* Initialize private NCSI device */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 087db775b3dc..90bb7f57bd86 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -39,8 +39,12 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 
 	/* Check on code and reason */
 	if (ntohs(h->code) != NCSI_PKT_RSP_C_COMPLETED ||
-	    ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR)
+	    ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR) {
+		printk(KERN_ERR "ncsi rsp not valid!\n");
+		printk(KERN_ERR "\tCode: %x\n", ntohs(h->code));
+		printk(KERN_ERR "\tReason: %x\n", ntohs(h->reason));
 		return -EINVAL;
+	}
 
 	/* Validate checksum, which might be zeroes if the
 	 * sender doesn't support checksum according to NCSI
@@ -693,7 +697,12 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
 
 		ncf->index = i;
 		ncf->total = cnt;
-		ncf->bitmap = 0x0ul;
+		if (i == NCSI_FILTER_VLAN) {
+			/* Set VLAN filters active so they are cleared in config */
+			ncf->bitmap = U64_MAX;
+		} else {
+			ncf->bitmap = 0x0ul;
+		}
 		nc->filters[i] = ncf;
 	}
 
-- 
2.13.3

Re: [RFC PATCH 2/3] net/ncsi: Fix several packet definitions

[Joel Stanley]

On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> ---
>  net/ncsi/ncsi-cmd.c | 10 +++++-----
>  net/ncsi/ncsi-pkt.h |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index db7083bfd476..1fec9fda7f60 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -146,9 +146,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
>
>         cmd = (struct ncsi_cmd_svf_pkt *)skb_put(skb, sizeof(*cmd));
>         memset(cmd, 0, sizeof(*cmd));
> -       cmd->vlan = htons(nca->words[0]);
> -       cmd->index = nca->bytes[2];
> -       cmd->enable = nca->bytes[3];
> +       cmd->vlan = htons(nca->words[1]);
> +       cmd->index = nca->bytes[6];
> +       cmd->enable = nca->bytes[7];

These look like straight up bugs. Should we send them off as fixes?

>         ncsi_cmd_build_header(&cmd->cmd.common, nca);
>
>         return 0;
> @@ -161,7 +161,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
>
>         cmd = (struct ncsi_cmd_ev_pkt *)skb_put(skb, sizeof(*cmd));
>         memset(cmd, 0, sizeof(*cmd));
> -       cmd->mode = nca->bytes[0];
> +       cmd->mode = nca->bytes[3];
>         ncsi_cmd_build_header(&cmd->cmd.common, nca);
>
>         return 0;
> @@ -240,7 +240,7 @@ static struct ncsi_cmd_handler {
>         { NCSI_PKT_CMD_AE,     8, ncsi_cmd_handler_ae      },
>         { NCSI_PKT_CMD_SL,     8, ncsi_cmd_handler_sl      },
>         { NCSI_PKT_CMD_GLS,    0, ncsi_cmd_handler_default },
> -       { NCSI_PKT_CMD_SVF,    4, ncsi_cmd_handler_svf     },
> +       { NCSI_PKT_CMD_SVF,    8, ncsi_cmd_handler_svf     },
>         { NCSI_PKT_CMD_EV,     4, ncsi_cmd_handler_ev      },
>         { NCSI_PKT_CMD_DV,     0, ncsi_cmd_handler_default },
>         { NCSI_PKT_CMD_SMA,    8, ncsi_cmd_handler_sma     },
> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> index 3ea49ed0a935..91b4b66438df 100644
> --- a/net/ncsi/ncsi-pkt.h
> +++ b/net/ncsi/ncsi-pkt.h
> @@ -104,7 +104,7 @@ struct ncsi_cmd_svf_pkt {
>         unsigned char           index;     /* VLAN table index  */
>         unsigned char           enable;    /* Enable or disable */
>         __be32                  checksum;  /* Checksum          */
> -       unsigned char           pad[14];
> +       unsigned char           pad[18];
>  };
>
>  /* Enable VLAN */
> --
> 2.13.3
>

Re: [RFC PATCH 3/3] net/ncsi: Configure VLAN tag filter

[Joel Stanley]

On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> Make use of the ndo_vlan_rx_{add,kill}_vid callbacks to have the NCSI
> stack process new VLAN tags and configure the channel VLAN filter
> appropriately.
> Several VLAN tags can be set and a "Set VLAN Filter" packet must be sent
> for each one, meaning the ncsi_dev_state_config_svf state must be
> repeated. An internal list of VLAN tags is maintained, and compared
> against the current channel's ncsi_channel_filter in order to keep track
> within the state. VLAN filters are removed in a similar manner, with the
> introduction of the ncsi_dev_state_config_clear_vids state. The maximum
> number of VLAN tag filters is determined by the "Get Capabilities"
> response from the channel.
>
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>

Thanks Sam. I've made some comments below.

> ---
>  drivers/net/ethernet/faraday/ftgmac100.c |   2 +
>  include/net/ncsi.h                       |   2 +
>  net/ncsi/internal.h                      |  11 ++
>  net/ncsi/ncsi-manage.c                   | 257 ++++++++++++++++++++++++++++++-
>  net/ncsi/ncsi-rsp.c                      |  13 +-
>  5 files changed, 281 insertions(+), 4 deletions(-)


> @@ -699,11 +706,93 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>                 nca.package = np->id;
>                 nca.channel = nc->id;
>
> +               /* Clear any active filters on the channel before setting */
> +               if (nd->state == ncsi_dev_state_config_clear_vids) {

This is epic. Could it go in a function?

> +                       clear = false;
> +                       vlan_index = -1;
> +                       spin_lock_irqsave(&nc->lock, flags);
> +                       ncf = nc->filters[NCSI_FILTER_VLAN];
> +                       while ((vlan_index = find_next_bit((void *)&ncf->bitmap, ncf->total, vlan_index + 1))

We should make ->bitmap an unsigned long, then we can pass it to
find_next_bit without casting. Other users of it do the same thing
with a temporary variable.

> +                              < ncf->total) {
> +                               clear = true;
> +                               // TODO retrieve the vid that was set

Still WIP?

> +                               netdev_printk(KERN_DEBUG, dev, "ncsi: removed vlan tag at index %d\n",

Do we get a ncsi prefix from the driver name?

> +                                               vlan_index);
> +                               ncsi_remove_filter(nc, NCSI_FILTER_VLAN, vlan_index);
> +                               break;
> +                       }
> +                       spin_unlock_irqrestore(&nc->lock, flags);
> +

> +int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> +{
> +       struct ncsi_channel_filter *ncf;
> +       struct ncsi_dev_priv *ndp;
> +       unsigned int n_vids = 0;
> +       struct vlan_vid *vlan;
> +       struct ncsi_dev *nd;
> +       bool found = false;

> +
> +       if (n_vids >= ncf->total) {
> +               netdev_info(dev, "NCSI Channel supports up to %u VLAN tags but %u are already set\n",
> +                               ncf->total, n_vids);
> +               return -EINVAL;
> +       }
> +
> +       vlan = kzalloc(sizeof(struct vlan_vid), GFP_KERNEL);

Does this get freed anywhere?

> +       if (!vlan) {
> +               netdev_err(dev, "could not alloc vlan_vid struct\n");
> +               return -ENOMEM;
> +       }
>

Re: [RFC PATCH 1/3] ftgmac: Include NETIF_F_HW_VLAN_CTAG_FILTER in features

[Joel Stanley]

On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> This is required for the VLAN core to call the add/kill callback for
> VLAN IDs.
>
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 33b5c8eb9961..7c4d772287c7 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1837,6 +1837,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
>                 NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
>                 NETIF_F_HW_VLAN_CTAG_TX;
>
> +       if (priv->use_ncsi)
> +               netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;

I'm a VLAN noob. Why is this conditional on ncsi?

Can the ftgmac100 support vlan with a directly attached PHY?

Cheers,

Joel

Re: [RFC PATCH 3/3] net/ncsi: Configure VLAN tag filter

[Joel Stanley]

On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> +static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> +{
> +       struct ncsi_dev *nd = &ndp->ndev;
> +       struct ncsi_channel *nc;
> +       struct ncsi_package *np;
> +       unsigned long flags;
> +       unsigned int n = 0;
> +
> +       NCSI_FOR_EACH_PACKAGE(ndp, np) {
> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
> +                       spin_lock_irqsave(&nc->lock, flags);
> +
> +                       /*
> +                        * Channels may be busy - mark dirty instead of kicking if
> +                        * a) not ACTIVE (configured)
> +                        * b) in the channel_queue (to be configured)
> +                        * c) it's ndev is in the config state
> +                        */
> +                       if (nc->state != NCSI_CHANNEL_ACTIVE) {
> +                               if ((ndp->ndev.state & 0xff00) == ncsi_dev_state_config ||
> +                                               !list_empty(&nc->link)) {
> +                                       netdev_info(nd->dev, "nc %p marked dirty\n", nc);
> +                                       nc->reconfigure_needed = true;
> +                               }
> +                               spin_unlock_irqrestore(&nc->lock, flags);
> +                               continue;
> +                       }
> +
> +                       spin_unlock_irqrestore(&nc->lock, flags);
> +
> +                       ncsi_stop_channel_monitor(nc);
> +                       spin_lock_irqsave(&nc->lock, flags);
> +                       nc->state = NCSI_CHANNEL_INVISIBLE;
> +                       spin_unlock_irqrestore(&nc->lock, flags);
> +
> +                       spin_lock_irqsave(&ndp->lock, flags);
> +                       nc->state = NCSI_CHANNEL_INACTIVE;
> +                       list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +                       spin_unlock_irqrestore(&ndp->lock, flags);
> +
> +                       spin_unlock_irqrestore(&nc->lock, flags);

Oops, this is unlocking nc->lock a second time.

> +
> +                       netdev_info(nd->dev, "Kicked channel %p\n", nc);
> +                       n++;
> +               }
> +       }

Re: [RFC PATCH 2/3] net/ncsi: Fix several packet definitions

[Benjamin Herrenschmidt]

On Thu, 2017-08-10 at 16:33 +0930, Joel Stanley wrote:
> On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
> <sam@mendozajonas.com> wrote:
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> > ---
> >  net/ncsi/ncsi-cmd.c | 10 +++++-----
> >  net/ncsi/ncsi-pkt.h |  2 +-
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index db7083bfd476..1fec9fda7f60 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -146,9 +146,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
> > 
> >         cmd = (struct ncsi_cmd_svf_pkt *)skb_put(skb, sizeof(*cmd));
> >         memset(cmd, 0, sizeof(*cmd));
> > -       cmd->vlan = htons(nca->words[0]);
> > -       cmd->index = nca->bytes[2];
> > -       cmd->enable = nca->bytes[3];
> > +       cmd->vlan = htons(nca->words[1]);
> > +       cmd->index = nca->bytes[6];
> > +       cmd->enable = nca->bytes[7];
> 
> These look like straight up bugs. Should we send them off as fixes?

This shows how completely wrong that whole "magic byte/words array
argumetns" thing is, especially when there *is* a proper struct
definitions for the packets :-( We definitely need that fixed too asap.

> >         ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > 
> >         return 0;
> > @@ -161,7 +161,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
> > 
> >         cmd = (struct ncsi_cmd_ev_pkt *)skb_put(skb, sizeof(*cmd));
> >         memset(cmd, 0, sizeof(*cmd));
> > -       cmd->mode = nca->bytes[0];
> > +       cmd->mode = nca->bytes[3];
> >         ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > 
> >         return 0;
> > @@ -240,7 +240,7 @@ static struct ncsi_cmd_handler {
> >         { NCSI_PKT_CMD_AE,     8, ncsi_cmd_handler_ae      },
> >         { NCSI_PKT_CMD_SL,     8, ncsi_cmd_handler_sl      },
> >         { NCSI_PKT_CMD_GLS,    0, ncsi_cmd_handler_default },
> > -       { NCSI_PKT_CMD_SVF,    4, ncsi_cmd_handler_svf     },
> > +       { NCSI_PKT_CMD_SVF,    8, ncsi_cmd_handler_svf     },
> >         { NCSI_PKT_CMD_EV,     4, ncsi_cmd_handler_ev      },
> >         { NCSI_PKT_CMD_DV,     0, ncsi_cmd_handler_default },
> >         { NCSI_PKT_CMD_SMA,    8, ncsi_cmd_handler_sma     },
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 3ea49ed0a935..91b4b66438df 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -104,7 +104,7 @@ struct ncsi_cmd_svf_pkt {
> >         unsigned char           index;     /* VLAN table index  */
> >         unsigned char           enable;    /* Enable or disable */
> >         __be32                  checksum;  /* Checksum          */
> > -       unsigned char           pad[14];
> > +       unsigned char           pad[18];
> >  };
> > 
> >  /* Enable VLAN */
> > --
> > 2.13.3
> > 

Re: [RFC PATCH 1/3] ftgmac: Include NETIF_F_HW_VLAN_CTAG_FILTER in features

[Benjamin Herrenschmidt]

On Thu, 2017-08-10 at 17:19 +0930, Joel Stanley wrote:
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1837,6 +1837,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
> >                  NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
> >                  NETIF_F_HW_VLAN_CTAG_TX;
> > 
> > +       if (priv->use_ncsi)
> > +               netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> 
> I'm a VLAN noob. Why is this conditional on ncsi?
> 
> Can the ftgmac100 support vlan with a directly attached PHY?

ftgmac100 supports vlans fine. This flag is telling the network stack
that we can offload the filtering so we need to get notified of vlans
being added or removed.

There is no point setting it when not doing ncsi. The HW doesn't have a
filter in that case (we do have HW vlan tag extraction and injection,
which my driver supports, but that's different flags).

Cheers,
Ben.

Re: [RFC PATCH 2/3] net/ncsi: Fix several packet definitions

[Samuel Mendoza-Jonas]

On Thu, 2017-08-10 at 16:33 +0930, Joel Stanley wrote:
> On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
> <sam@mendozajonas.com> wrote:
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> > ---
> >  net/ncsi/ncsi-cmd.c | 10 +++++-----
> >  net/ncsi/ncsi-pkt.h |  2 +-
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index db7083bfd476..1fec9fda7f60 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -146,9 +146,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
> > 
> >         cmd = (struct ncsi_cmd_svf_pkt *)skb_put(skb, sizeof(*cmd));
> >         memset(cmd, 0, sizeof(*cmd));
> > -       cmd->vlan = htons(nca->words[0]);
> > -       cmd->index = nca->bytes[2];
> > -       cmd->enable = nca->bytes[3];
> > +       cmd->vlan = htons(nca->words[1]);
> > +       cmd->index = nca->bytes[6];
> > +       cmd->enable = nca->bytes[7];
> 
> These look like straight up bugs. Should we send them off as fixes?

These are straight up bugs except... without my changes we never call
this code. As Ben says as time provides a lot of the current definitions
need to be gone over, there's a few command/response code paths that are
never triggered and could be broken in similar ways.

> 
> >         ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > 
> >         return 0;
> > @@ -161,7 +161,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
> > 
> >         cmd = (struct ncsi_cmd_ev_pkt *)skb_put(skb, sizeof(*cmd));
> >         memset(cmd, 0, sizeof(*cmd));
> > -       cmd->mode = nca->bytes[0];
> > +       cmd->mode = nca->bytes[3];
> >         ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > 
> >         return 0;
> > @@ -240,7 +240,7 @@ static struct ncsi_cmd_handler {
> >         { NCSI_PKT_CMD_AE,     8, ncsi_cmd_handler_ae      },
> >         { NCSI_PKT_CMD_SL,     8, ncsi_cmd_handler_sl      },
> >         { NCSI_PKT_CMD_GLS,    0, ncsi_cmd_handler_default },
> > -       { NCSI_PKT_CMD_SVF,    4, ncsi_cmd_handler_svf     },
> > +       { NCSI_PKT_CMD_SVF,    8, ncsi_cmd_handler_svf     },
> >         { NCSI_PKT_CMD_EV,     4, ncsi_cmd_handler_ev      },
> >         { NCSI_PKT_CMD_DV,     0, ncsi_cmd_handler_default },
> >         { NCSI_PKT_CMD_SMA,    8, ncsi_cmd_handler_sma     },
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 3ea49ed0a935..91b4b66438df 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -104,7 +104,7 @@ struct ncsi_cmd_svf_pkt {
> >         unsigned char           index;     /* VLAN table index  */
> >         unsigned char           enable;    /* Enable or disable */
> >         __be32                  checksum;  /* Checksum          */
> > -       unsigned char           pad[14];
> > +       unsigned char           pad[18];
> >  };
> > 
> >  /* Enable VLAN */
> > --
> > 2.13.3
> > 

Re: [RFC PATCH 3/3] net/ncsi: Configure VLAN tag filter

[Samuel Mendoza-Jonas]

On Thu, 2017-08-10 at 17:19 +0930, Joel Stanley wrote:
> On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
> <sam@mendozajonas.com> wrote:
> > Make use of the ndo_vlan_rx_{add,kill}_vid callbacks to have the NCSI
> > stack process new VLAN tags and configure the channel VLAN filter
> > appropriately.
> > Several VLAN tags can be set and a "Set VLAN Filter" packet must be sent
> > for each one, meaning the ncsi_dev_state_config_svf state must be
> > repeated. An internal list of VLAN tags is maintained, and compared
> > against the current channel's ncsi_channel_filter in order to keep track
> > within the state. VLAN filters are removed in a similar manner, with the
> > introduction of the ncsi_dev_state_config_clear_vids state. The maximum
> > number of VLAN tag filters is determined by the "Get Capabilities"
> > response from the channel.
> > 
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> 
> Thanks Sam. I've made some comments below.
> 
> > ---
> >  drivers/net/ethernet/faraday/ftgmac100.c |   2 +
> >  include/net/ncsi.h                       |   2 +
> >  net/ncsi/internal.h                      |  11 ++
> >  net/ncsi/ncsi-manage.c                   | 257 ++++++++++++++++++++++++++++++-
> >  net/ncsi/ncsi-rsp.c                      |  13 +-
> >  5 files changed, 281 insertions(+), 4 deletions(-)
> 
> 
> > @@ -699,11 +706,93 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> >                 nca.package = np->id;
> >                 nca.channel = nc->id;
> > 
> > +               /* Clear any active filters on the channel before setting */
> > +               if (nd->state == ncsi_dev_state_config_clear_vids) {
> 
> This is epic. Could it go in a function?

Yep, I'll clean up and split out what I can.

> 
> > +                       clear = false;
> > +                       vlan_index = -1;
> > +                       spin_lock_irqsave(&nc->lock, flags);
> > +                       ncf = nc->filters[NCSI_FILTER_VLAN];
> > +                       while ((vlan_index = find_next_bit((void *)&ncf->bitmap, ncf->total, vlan_index + 1))
> 
> We should make ->bitmap an unsigned long, then we can pass it to
> find_next_bit without casting. Other users of it do the same thing
> with a temporary variable.

Ack

> 
> > +                              < ncf->total) {
> > +                               clear = true;
> > +                               // TODO retrieve the vid that was set
> 
> Still WIP?

Definitely still 'rough' :)

> 
> > +                               netdev_printk(KERN_DEBUG, dev, "ncsi: removed vlan tag at index %d\n",
> 
> Do we get a ncsi prefix from the driver name?

No we get a prefix of the form
	"ftgmac100 1e660000.ethernet eth0:"
so yes adding a NCSI prefix would be good.

> 
> > +                                               vlan_index);
> > +                               ncsi_remove_filter(nc, NCSI_FILTER_VLAN, vlan_index);
> > +                               break;
> > +                       }
> > +                       spin_unlock_irqrestore(&nc->lock, flags);
> > +
> > +int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> > +{
> > +       struct ncsi_channel_filter *ncf;
> > +       struct ncsi_dev_priv *ndp;
> > +       unsigned int n_vids = 0;
> > +       struct vlan_vid *vlan;
> > +       struct ncsi_dev *nd;
> > +       bool found = false;
> > +
> > +       if (n_vids >= ncf->total) {
> > +               netdev_info(dev, "NCSI Channel supports up to %u VLAN tags but %u are already set\n",
> > +                               ncf->total, n_vids);
> > +               return -EINVAL;
> > +       }
> > +
> > +       vlan = kzalloc(sizeof(struct vlan_vid), GFP_KERNEL);
> 
> Does this get freed anywhere?

Whoops!

> 
> > +       if (!vlan) {
> > +               netdev_err(dev, "could not alloc vlan_vid struct\n");
> > +               return -ENOMEM;
> > +       }
> > 

Re: [RFC PATCH 3/3] net/ncsi: Configure VLAN tag filter

[Samuel Mendoza-Jonas]

On Thu, 2017-08-10 at 17:55 +0930, Joel Stanley wrote:
> On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
> <sam@mendozajonas.com> wrote:
> > +static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> > +{
> > +       struct ncsi_dev *nd = &ndp->ndev;
> > +       struct ncsi_channel *nc;
> > +       struct ncsi_package *np;
> > +       unsigned long flags;
> > +       unsigned int n = 0;
> > +
> > +       NCSI_FOR_EACH_PACKAGE(ndp, np) {
> > +               NCSI_FOR_EACH_CHANNEL(np, nc) {
> > +                       spin_lock_irqsave(&nc->lock, flags);
> > +
> > +                       /*
> > +                        * Channels may be busy - mark dirty instead of kicking if
> > +                        * a) not ACTIVE (configured)
> > +                        * b) in the channel_queue (to be configured)
> > +                        * c) it's ndev is in the config state
> > +                        */
> > +                       if (nc->state != NCSI_CHANNEL_ACTIVE) {
> > +                               if ((ndp->ndev.state & 0xff00) == ncsi_dev_state_config ||
> > +                                               !list_empty(&nc->link)) {
> > +                                       netdev_info(nd->dev, "nc %p marked dirty\n", nc);
> > +                                       nc->reconfigure_needed = true;
> > +                               }
> > +                               spin_unlock_irqrestore(&nc->lock, flags);
> > +                               continue;
> > +                       }
> > +
> > +                       spin_unlock_irqrestore(&nc->lock, flags);
> > +
> > +                       ncsi_stop_channel_monitor(nc);
> > +                       spin_lock_irqsave(&nc->lock, flags);
> > +                       nc->state = NCSI_CHANNEL_INVISIBLE;
> > +                       spin_unlock_irqrestore(&nc->lock, flags);
> > +
> > +                       spin_lock_irqsave(&ndp->lock, flags);
> > +                       nc->state = NCSI_CHANNEL_INACTIVE;
> > +                       list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> > +                       spin_unlock_irqrestore(&ndp->lock, flags);
> > +
> > +                       spin_unlock_irqrestore(&nc->lock, flags);
> 
> Oops, this is unlocking nc->lock a second time.

Oops indeed! Good spot.

> 
> > +
> > +                       netdev_info(nd->dev, "Kicked channel %p\n", nc);
> > +                       n++;
> > +               }
> > +       }

Re: [RFC PATCH 1/3] ftgmac: Include NETIF_F_HW_VLAN_CTAG_FILTER in features

[Joel Stanley]

On Thu, Aug 10, 2017 at 9:00 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2017-08-10 at 17:19 +0930, Joel Stanley wrote:
>> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
>> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
>> > @@ -1837,6 +1837,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
>> >                  NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
>> >                  NETIF_F_HW_VLAN_CTAG_TX;
>> >
>> > +       if (priv->use_ncsi)
>> > +               netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>>
>> I'm a VLAN noob. Why is this conditional on ncsi?
>>
>> Can the ftgmac100 support vlan with a directly attached PHY?
>
> ftgmac100 supports vlans fine. This flag is telling the network stack
> that we can offload the filtering so we need to get notified of vlans
> being added or removed.
>
> There is no point setting it when not doing ncsi. The HW doesn't have a
> filter in that case (we do have HW vlan tag extraction and injection,
> which my driver supports, but that's different flags).

Thanks for the explanation. Makes sense.

Sam, that might make a good addition to the commit message.

Cheers,

Joel

Re: [RFC PATCH 2/3] net/ncsi: Fix several packet definitions

[Joel Stanley]

On Fri, Aug 11, 2017 at 8:09 AM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> On Thu, 2017-08-10 at 16:33 +0930, Joel Stanley wrote:
>> On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
>> <sam@mendozajonas.com> wrote:
>> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
>> > ---
>> >  net/ncsi/ncsi-cmd.c | 10 +++++-----
>> >  net/ncsi/ncsi-pkt.h |  2 +-
>> >  2 files changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
>> > index db7083bfd476..1fec9fda7f60 100644
>> > --- a/net/ncsi/ncsi-cmd.c
>> > +++ b/net/ncsi/ncsi-cmd.c
>> > @@ -146,9 +146,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
>> >
>> >         cmd = (struct ncsi_cmd_svf_pkt *)skb_put(skb, sizeof(*cmd));
>> >         memset(cmd, 0, sizeof(*cmd));
>> > -       cmd->vlan = htons(nca->words[0]);
>> > -       cmd->index = nca->bytes[2];
>> > -       cmd->enable = nca->bytes[3];
>> > +       cmd->vlan = htons(nca->words[1]);
>> > +       cmd->index = nca->bytes[6];
>> > +       cmd->enable = nca->bytes[7];
>>
>> These look like straight up bugs. Should we send them off as fixes?
>
> These are straight up bugs except... without my changes we never call
> this code. As Ben says as time provides a lot of the current definitions
> need to be gone over, there's a few command/response code paths that are
> never triggered and could be broken in similar ways.

Okay. So we can do the cleanup once the vlan fire has been put out?

Makes sense to me. Unless we start hitting strange bugs, in which case
the cleanup might expose other issues.

Cheers,

Joel