* [PATCH net-next v2 0/3] NCSI VLAN Filtering Support
@ 2017-08-14 1:29 Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions Samuel Mendoza-Jonas
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Samuel Mendoza-Jonas @ 2017-08-14 1:29 UTC (permalink / raw)
To: David S . Miller, netdev, linux-kernel, OpenBMC Maillist
Cc: Samuel Mendoza-Jonas, Joel Stanley, Benjamin Herrenschmidt,
Gavin Shan, ratagupt
This series (mainly patch 2) adds VLAN filtering to the NCSI implementation.
A fair amount of code already exists in the NCSI stack for VLAN filtering but
none of it is actually hooked up. This goes the final mile and fixes a few
bugs in the existing code found along the way (patch 1).
Patch 3 adds the appropriate flag and callbacks to the ftgmac100 driver to
enable filtering as it's a large consumer of NCSI (and what I've been
testing on).
Samuel Mendoza-Jonas (3):
net/ncsi: Fix several packet definitions
net/ncsi: Configure VLAN tag filter
ftgmac100: Support NCSI VLAN filtering when available
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 | 295 ++++++++++++++++++++++++++++++-
net/ncsi/ncsi-pkt.h | 2 +-
net/ncsi/ncsi-rsp.c | 12 +-
7 files changed, 326 insertions(+), 11 deletions(-)
--
2.14.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions
2017-08-14 1:29 [PATCH net-next v2 0/3] NCSI VLAN Filtering Support Samuel Mendoza-Jonas
@ 2017-08-14 1:29 ` Samuel Mendoza-Jonas
2017-08-14 2:10 ` Joel Stanley
2017-08-14 1:29 ` [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available Samuel Mendoza-Jonas
2 siblings, 1 reply; 11+ messages in thread
From: Samuel Mendoza-Jonas @ 2017-08-14 1:29 UTC (permalink / raw)
To: David S . Miller, netdev, linux-kernel, OpenBMC Maillist
Cc: Samuel Mendoza-Jonas, Joel Stanley, Benjamin Herrenschmidt,
Gavin Shan, ratagupt
Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
v2: Rebased on latest net-next
net/ncsi/ncsi-cmd.c | 10 +++++-----
net/ncsi/ncsi-pkt.h | 2 +-
net/ncsi/ncsi-rsp.c | 3 ++-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 5e03ed190e18..7567ca63aae2 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -139,9 +139,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
struct ncsi_cmd_svf_pkt *cmd;
cmd = skb_put_zero(skb, 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;
@@ -153,7 +153,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
struct ncsi_cmd_ev_pkt *cmd;
cmd = skb_put_zero(skb, sizeof(*cmd));
- cmd->mode = nca->bytes[0];
+ cmd->mode = nca->bytes[3];
ncsi_cmd_build_header(&cmd->cmd.common, nca);
return 0;
@@ -228,7 +228,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 */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 087db775b3dc..c1a191d790e2 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -354,7 +354,8 @@ static int ncsi_rsp_handler_svf(struct ncsi_request *nr)
/* Add or remove the VLAN filter */
if (!(cmd->enable & 0x1)) {
- ret = ncsi_remove_filter(nc, NCSI_FILTER_VLAN, cmd->index);
+ /* HW indexes from 1 */
+ ret = ncsi_remove_filter(nc, NCSI_FILTER_VLAN, cmd->index - 1);
} else {
vlan = ntohs(cmd->vlan);
ret = ncsi_add_filter(nc, NCSI_FILTER_VLAN, &vlan);
--
2.14.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter
2017-08-14 1:29 [PATCH net-next v2 0/3] NCSI VLAN Filtering Support Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions Samuel Mendoza-Jonas
@ 2017-08-14 1:29 ` Samuel Mendoza-Jonas
2017-08-14 2:09 ` Joel Stanley
2017-08-14 1:29 ` [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available Samuel Mendoza-Jonas
2 siblings, 1 reply; 11+ messages in thread
From: Samuel Mendoza-Jonas @ 2017-08-14 1:29 UTC (permalink / raw)
To: David S . Miller, netdev, linux-kernel, OpenBMC Maillist
Cc: Samuel Mendoza-Jonas, Joel Stanley, Benjamin Herrenschmidt,
Gavin Shan, ratagupt
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>
---
include/net/ncsi.h | 2 +
net/ncsi/internal.h | 11 ++
net/ncsi/ncsi-manage.c | 295 ++++++++++++++++++++++++++++++++++++++++++++++++-
net/ncsi/ncsi-rsp.c | 9 +-
4 files changed, 313 insertions(+), 4 deletions(-)
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..3cbd4328f142 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -38,6 +38,22 @@ static inline int ncsi_filter_size(int table)
return sizes[table];
}
+u32 *ncsi_get_filter(struct ncsi_channel *nc, int table, int index)
+{
+ struct ncsi_channel_filter *ncf;
+ int size;
+
+ ncf = nc->filters[table];
+ if (!ncf)
+ return NULL;
+
+ size = ncsi_filter_size(table);
+ if (size < 0)
+ return NULL;
+
+ return ncf->data + size * index;
+}
+
int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
{
struct ncsi_channel_filter *ncf;
@@ -58,7 +74,7 @@ int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
index = -1;
while ((index = find_next_bit(bitmap, ncf->total, index + 1))
< ncf->total) {
- if (!memcmp(ncf->data + size * index, data, size)) {
+ if (!data || !memcmp(ncf->data + size * index, data, size)) {
spin_unlock_irqrestore(&nc->lock, flags);
return index;
}
@@ -639,6 +655,83 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
nd->state = ncsi_dev_state_functional;
}
+/* Check the VLAN filter bitmap for a set filter, and construct a
+ * "Set VLAN Filter - Disable" packet if found.
+ */
+static int clear_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
+ struct ncsi_cmd_arg *nca)
+{
+ int index;
+ u16 vid;
+
+ index = ncsi_find_filter(nc, NCSI_FILTER_VLAN, NULL);
+ if (index < 0) {
+ /* Filter table empty */
+ return -1;
+ }
+
+ vid = *(u16 *)ncsi_get_filter(nc, NCSI_FILTER_VLAN, index);
+ netdev_printk(KERN_DEBUG, ndp->ndev.dev,
+ "ncsi: removed vlan tag %u at index %d\n",
+ vid, index + 1);
+ ncsi_remove_filter(nc, NCSI_FILTER_VLAN, index);
+
+ nca->type = NCSI_PKT_CMD_SVF;
+ nca->words[1] = vid;
+ /* HW filter index starts at 1 */
+ nca->bytes[6] = index + 1;
+ nca->bytes[7] = 0x00;
+ return 0;
+}
+
+/* Find an outstanding VLAN tag and constuct a "Set VLAN Filter - Enable"
+ * packet.
+ */
+static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
+ struct ncsi_cmd_arg *nca)
+{
+ struct vlan_vid *vlan = NULL;
+ int index = 0;
+
+ list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
+ index = ncsi_find_filter(nc, NCSI_FILTER_VLAN, &vlan->vid);
+ if (index < 0) {
+ /* New tag to add */
+ netdev_printk(KERN_DEBUG, ndp->ndev.dev,
+ "ncsi: new vlan id to set: %u\n",
+ vlan->vid);
+ break;
+ }
+ netdev_printk(KERN_DEBUG, ndp->ndev.dev,
+ "vid %u already at filter pos %d\n",
+ vlan->vid, index);
+ }
+
+ if (!vlan || index >= 0) {
+ netdev_printk(KERN_DEBUG, ndp->ndev.dev,
+ "no vlan ids left to set\n");
+ return -1;
+ }
+
+ index = ncsi_add_filter(nc, NCSI_FILTER_VLAN, &vlan->vid);
+ if (index < 0) {
+ netdev_err(ndp->ndev.dev,
+ "Failed to add new VLAN tag, error %d\n", index);
+ return -1;
+ }
+
+ netdev_printk(KERN_DEBUG, ndp->ndev.dev,
+ "ncsi: set vid %u in packet, index %u\n",
+ vlan->vid, index + 1);
+ nca->type = NCSI_PKT_CMD_SVF;
+ nca->words[1] = vlan->vid;
+ /* HW filter index starts at 1 */
+ nca->bytes[6] = index + 1;
+ nca->bytes[7] = 0x01;
+
+ return 0;
+}
+
static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
{
struct ncsi_dev *nd = &ndp->ndev;
@@ -683,8 +776,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 +795,40 @@ 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) {
+ ret = clear_one_vid(ndp, nc, &nca);
+ if (ret) {
+ nd->state = ncsi_dev_state_config_svf;
+ schedule_work(&ndp->work);
+ break;
+ }
+ /* Repeat */
+ nd->state = ncsi_dev_state_config_clear_vids;
+ /* Add known VLAN tags to the filter */
+ } else if (nd->state == ncsi_dev_state_config_svf) {
+ ret = set_one_vid(ndp, nc, &nca);
+ if (ret) {
+ nd->state = ncsi_dev_state_config_ev;
+ schedule_work(&ndp->work);
+ break;
+ }
+ /* Repeat */
+ nd->state = ncsi_dev_state_config_svf;
+ /* 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;
+ 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 +876,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_printk(KERN_DEBUG, 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 +1336,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_printk(KERN_DEBUG, nd->dev,
+ "ncsi: channel %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);
+
+ netdev_printk(KERN_DEBUG, nd->dev,
+ "ncsi: 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)
+ return 0;
+
+ nd = ncsi_find_dev(dev);
+ if (!nd) {
+ netdev_warn(dev, "ncsi: 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_printk(KERN_DEBUG, 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(*vlan), GFP_KERNEL);
+ if (!vlan)
+ return -ENOMEM;
+
+ vlan->proto = proto;
+ vlan->vid = vid;
+ list_add_rcu(&vlan->list, &ndp->vlan_vids);
+
+ netdev_printk(KERN_DEBUG, 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)
+ return 0;
+
+ nd = ncsi_find_dev(dev);
+ if (!nd) {
+ netdev_warn(dev, "ncsi: 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_printk(KERN_DEBUG, dev,
+ "vid %u found, removing\n", vid);
+ list_del_rcu(&vlan->list);
+ found = true;
+ kfree(vlan);
+ }
+
+ if (!found) {
+ netdev_err(dev, "ncsi: 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 +1503,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 c1a191d790e2..265b9a892d41 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -694,7 +694,14 @@ 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
+ * first configuration state
+ */
+ ncf->bitmap = U64_MAX;
+ } else {
+ ncf->bitmap = 0x0ul;
+ }
nc->filters[i] = ncf;
}
--
2.14.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available
2017-08-14 1:29 [PATCH net-next v2 0/3] NCSI VLAN Filtering Support Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter Samuel Mendoza-Jonas
@ 2017-08-14 1:29 ` Samuel Mendoza-Jonas
2017-08-14 2:16 ` Joel Stanley
2 siblings, 1 reply; 11+ messages in thread
From: Samuel Mendoza-Jonas @ 2017-08-14 1:29 UTC (permalink / raw)
To: David S . Miller, netdev, linux-kernel, OpenBMC Maillist
Cc: Samuel Mendoza-Jonas, Joel Stanley, Benjamin Herrenschmidt,
Gavin Shan, ratagupt
Register the ndo_vlan_rx_{add,kill}_vid callbacks and set the
NETIF_F_HW_VLAN_CTAG_FILTER if NCSI is available.
This allows the VLAN core to notify the NCSI driver when changes occur
so that the remote NCSI channel can be properly configured to filter on
the set VLAN tags.
Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
---
v2: Moved ftgmac100 change into same patch and reordered
drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 34dae51effd4..05fe7123d5ae 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)
@@ -1837,6 +1839,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.14.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter
2017-08-14 1:29 ` [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter Samuel Mendoza-Jonas
@ 2017-08-14 2:09 ` Joel Stanley
0 siblings, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2017-08-14 2:09 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, Aug 14, 2017 at 10:59 AM, 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>
I've given this some testing, but there are a few things I saw below
that we should sort out.
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -38,6 +38,22 @@ static inline int ncsi_filter_size(int table)
> return sizes[table];
> }
>
> +u32 *ncsi_get_filter(struct ncsi_channel *nc, int table, int index)
> +{
> + struct ncsi_channel_filter *ncf;
> + int size;
> +
> + ncf = nc->filters[table];
> + if (!ncf)
> + return NULL;
> +
> + size = ncsi_filter_size(table);
> + if (size < 0)
> + return NULL;
> +
> + return ncf->data + size * index;
> +}
> +
> int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
> {
> struct ncsi_channel_filter *ncf;
> @@ -58,7 +74,7 @@ int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
> index = -1;
> while ((index = find_next_bit(bitmap, ncf->total, index + 1))
> < ncf->total) {
> - if (!memcmp(ncf->data + size * index, data, size)) {
> + if (!data || !memcmp(ncf->data + size * index, data, size)) {
Not clear why this check is required?
> spin_unlock_irqrestore(&nc->lock, flags);
> return index;
> }
> @@ -639,6 +655,83 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
> nd->state = ncsi_dev_state_functional;
> }
>
> +/* Check the VLAN filter bitmap for a set filter, and construct a
> + * "Set VLAN Filter - Disable" packet if found.
> + */
> +static int clear_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
> + struct ncsi_cmd_arg *nca)
> +{
> + int index;
> + u16 vid;
> +
> + index = ncsi_find_filter(nc, NCSI_FILTER_VLAN, NULL);
> + if (index < 0) {
> + /* Filter table empty */
> + return -1;
> + }
> +
> + vid = *(u16 *)ncsi_get_filter(nc, NCSI_FILTER_VLAN, index);
You just added this function that returns a pointer to a u32. It's
strange to see the only call site then throw half of it away.
Also, ncsi_get_filter can return NULL.
> + netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> + "ncsi: removed vlan tag %u at index %d\n",
> + vid, index + 1);
> + ncsi_remove_filter(nc, NCSI_FILTER_VLAN, index);
> +
> + nca->type = NCSI_PKT_CMD_SVF;
> + nca->words[1] = vid;
> + /* HW filter index starts at 1 */
> + nca->bytes[6] = index + 1;
> + nca->bytes[7] = 0x00;
> + return 0;
> +}
> @@ -751,6 +876,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;
This looks strange. What's nc->state up to? Does setting it to
NCSI_CHANNEL_INVISIBLE have any affect?
The locking is confusing too.
> + list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> + spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_printk(KERN_DEBUG, 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 +1336,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_printk(KERN_DEBUG, nd->dev,
> + "ncsi: channel %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;
This is a similar pattern to ncsi_configure_channel. I suspect the
answer there will be the same as here.
> + list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> + spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_printk(KERN_DEBUG, nd->dev,
> + "ncsi: kicked channel %p\n", nc);
> + n++;
> + }
> + }
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter
@ 2017-08-14 2:09 ` Joel Stanley
0 siblings, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2017-08-14 2:09 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, Aug 14, 2017 at 10:59 AM, 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>
I've given this some testing, but there are a few things I saw below
that we should sort out.
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -38,6 +38,22 @@ static inline int ncsi_filter_size(int table)
> return sizes[table];
> }
>
> +u32 *ncsi_get_filter(struct ncsi_channel *nc, int table, int index)
> +{
> + struct ncsi_channel_filter *ncf;
> + int size;
> +
> + ncf = nc->filters[table];
> + if (!ncf)
> + return NULL;
> +
> + size = ncsi_filter_size(table);
> + if (size < 0)
> + return NULL;
> +
> + return ncf->data + size * index;
> +}
> +
> int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
> {
> struct ncsi_channel_filter *ncf;
> @@ -58,7 +74,7 @@ int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
> index = -1;
> while ((index = find_next_bit(bitmap, ncf->total, index + 1))
> < ncf->total) {
> - if (!memcmp(ncf->data + size * index, data, size)) {
> + if (!data || !memcmp(ncf->data + size * index, data, size)) {
Not clear why this check is required?
> spin_unlock_irqrestore(&nc->lock, flags);
> return index;
> }
> @@ -639,6 +655,83 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
> nd->state = ncsi_dev_state_functional;
> }
>
> +/* Check the VLAN filter bitmap for a set filter, and construct a
> + * "Set VLAN Filter - Disable" packet if found.
> + */
> +static int clear_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
> + struct ncsi_cmd_arg *nca)
> +{
> + int index;
> + u16 vid;
> +
> + index = ncsi_find_filter(nc, NCSI_FILTER_VLAN, NULL);
> + if (index < 0) {
> + /* Filter table empty */
> + return -1;
> + }
> +
> + vid = *(u16 *)ncsi_get_filter(nc, NCSI_FILTER_VLAN, index);
You just added this function that returns a pointer to a u32. It's
strange to see the only call site then throw half of it away.
Also, ncsi_get_filter can return NULL.
> + netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> + "ncsi: removed vlan tag %u at index %d\n",
> + vid, index + 1);
> + ncsi_remove_filter(nc, NCSI_FILTER_VLAN, index);
> +
> + nca->type = NCSI_PKT_CMD_SVF;
> + nca->words[1] = vid;
> + /* HW filter index starts at 1 */
> + nca->bytes[6] = index + 1;
> + nca->bytes[7] = 0x00;
> + return 0;
> +}
> @@ -751,6 +876,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;
This looks strange. What's nc->state up to? Does setting it to
NCSI_CHANNEL_INVISIBLE have any affect?
The locking is confusing too.
> + list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> + spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_printk(KERN_DEBUG, 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 +1336,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_printk(KERN_DEBUG, nd->dev,
> + "ncsi: channel %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;
This is a similar pattern to ncsi_configure_channel. I suspect the
answer there will be the same as here.
> + list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> + spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_printk(KERN_DEBUG, nd->dev,
> + "ncsi: kicked channel %p\n", nc);
> + n++;
> + }
> + }
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions
2017-08-14 1:29 ` [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions Samuel Mendoza-Jonas
@ 2017-08-14 2:10 ` Joel Stanley
0 siblings, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2017-08-14 2:10 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, Aug 14, 2017 at 10:59 AM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
I asked Sam if these should be backported to stable and he said:
> 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.
So we're okay here.
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
> ---
> v2: Rebased on latest net-next
>
> net/ncsi/ncsi-cmd.c | 10 +++++-----
> net/ncsi/ncsi-pkt.h | 2 +-
> net/ncsi/ncsi-rsp.c | 3 ++-
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 5e03ed190e18..7567ca63aae2 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -139,9 +139,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
> struct ncsi_cmd_svf_pkt *cmd;
>
> cmd = skb_put_zero(skb, 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;
> @@ -153,7 +153,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
> struct ncsi_cmd_ev_pkt *cmd;
>
> cmd = skb_put_zero(skb, sizeof(*cmd));
> - cmd->mode = nca->bytes[0];
> + cmd->mode = nca->bytes[3];
> ncsi_cmd_build_header(&cmd->cmd.common, nca);
>
> return 0;
> @@ -228,7 +228,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 */
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 087db775b3dc..c1a191d790e2 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -354,7 +354,8 @@ static int ncsi_rsp_handler_svf(struct ncsi_request *nr)
>
> /* Add or remove the VLAN filter */
> if (!(cmd->enable & 0x1)) {
> - ret = ncsi_remove_filter(nc, NCSI_FILTER_VLAN, cmd->index);
> + /* HW indexes from 1 */
> + ret = ncsi_remove_filter(nc, NCSI_FILTER_VLAN, cmd->index - 1);
> } else {
> vlan = ntohs(cmd->vlan);
> ret = ncsi_add_filter(nc, NCSI_FILTER_VLAN, &vlan);
> --
> 2.14.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions
@ 2017-08-14 2:10 ` Joel Stanley
0 siblings, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2017-08-14 2:10 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, Aug 14, 2017 at 10:59 AM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
I asked Sam if these should be backported to stable and he said:
> 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.
So we're okay here.
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
> ---
> v2: Rebased on latest net-next
>
> net/ncsi/ncsi-cmd.c | 10 +++++-----
> net/ncsi/ncsi-pkt.h | 2 +-
> net/ncsi/ncsi-rsp.c | 3 ++-
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 5e03ed190e18..7567ca63aae2 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -139,9 +139,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
> struct ncsi_cmd_svf_pkt *cmd;
>
> cmd = skb_put_zero(skb, 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;
> @@ -153,7 +153,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
> struct ncsi_cmd_ev_pkt *cmd;
>
> cmd = skb_put_zero(skb, sizeof(*cmd));
> - cmd->mode = nca->bytes[0];
> + cmd->mode = nca->bytes[3];
> ncsi_cmd_build_header(&cmd->cmd.common, nca);
>
> return 0;
> @@ -228,7 +228,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 */
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 087db775b3dc..c1a191d790e2 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -354,7 +354,8 @@ static int ncsi_rsp_handler_svf(struct ncsi_request *nr)
>
> /* Add or remove the VLAN filter */
> if (!(cmd->enable & 0x1)) {
> - ret = ncsi_remove_filter(nc, NCSI_FILTER_VLAN, cmd->index);
> + /* HW indexes from 1 */
> + ret = ncsi_remove_filter(nc, NCSI_FILTER_VLAN, cmd->index - 1);
> } else {
> vlan = ntohs(cmd->vlan);
> ret = ncsi_add_filter(nc, NCSI_FILTER_VLAN, &vlan);
> --
> 2.14.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available
2017-08-14 1:29 ` [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available Samuel Mendoza-Jonas
@ 2017-08-14 2:16 ` Joel Stanley
0 siblings, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2017-08-14 2:16 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, Aug 14, 2017 at 10:59 AM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> Register the ndo_vlan_rx_{add,kill}_vid callbacks and set the
> NETIF_F_HW_VLAN_CTAG_FILTER if NCSI is available.
> This allows the VLAN core to notify the NCSI driver when changes occur
> so that the remote NCSI channel can be properly configured to filter on
> the set VLAN tags.
>
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
I'm not a vlan expert, so I asked why this was only being set when
doing NCSI. The answer was:
> 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).
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> v2: Moved ftgmac100 change into same patch and reordered
>
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 34dae51effd4..05fe7123d5ae 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)
> @@ -1837,6 +1839,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.14.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available
@ 2017-08-14 2:16 ` Joel Stanley
0 siblings, 0 replies; 11+ messages in thread
From: Joel Stanley @ 2017-08-14 2:16 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, Aug 14, 2017 at 10:59 AM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> Register the ndo_vlan_rx_{add,kill}_vid callbacks and set the
> NETIF_F_HW_VLAN_CTAG_FILTER if NCSI is available.
> This allows the VLAN core to notify the NCSI driver when changes occur
> so that the remote NCSI channel can be properly configured to filter on
> the set VLAN tags.
>
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
I'm not a vlan expert, so I asked why this was only being set when
doing NCSI. The answer was:
> 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).
Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---
> v2: Moved ftgmac100 change into same patch and reordered
>
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 34dae51effd4..05fe7123d5ae 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)
> @@ -1837,6 +1839,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.14.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter
2017-08-14 2:09 ` Joel Stanley
(?)
@ 2017-08-14 4:23 ` Samuel Mendoza-Jonas
-1 siblings, 0 replies; 11+ messages in thread
From: Samuel Mendoza-Jonas @ 2017-08-14 4:23 UTC (permalink / raw)
To: Joel Stanley
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
OpenBMC Maillist, Benjamin Herrenschmidt, Gavin Shan,
Ratan K Gupta
On Mon, 2017-08-14 at 11:39 +0930, Joel Stanley wrote:
> On Mon, Aug 14, 2017 at 10:59 AM, 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>
>
> I've given this some testing, but there are a few things I saw below
> that we should sort out.
>
> > --- a/net/ncsi/ncsi-manage.c
> > +++ b/net/ncsi/ncsi-manage.c
> > @@ -38,6 +38,22 @@ static inline int ncsi_filter_size(int table)
> > return sizes[table];
> > }
> >
> > +u32 *ncsi_get_filter(struct ncsi_channel *nc, int table, int index)
> > +{
> > + struct ncsi_channel_filter *ncf;
> > + int size;
> > +
> > + ncf = nc->filters[table];
> > + if (!ncf)
> > + return NULL;
> > +
> > + size = ncsi_filter_size(table);
> > + if (size < 0)
> > + return NULL;
> > +
> > + return ncf->data + size * index;
> > +}
> > +
> > int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
> > {
> > struct ncsi_channel_filter *ncf;
> > @@ -58,7 +74,7 @@ int ncsi_find_filter(struct ncsi_channel *nc, int table, void *data)
> > index = -1;
> > while ((index = find_next_bit(bitmap, ncf->total, index + 1))
> > < ncf->total) {
> > - if (!memcmp(ncf->data + size * index, data, size)) {
> > + if (!data || !memcmp(ncf->data + size * index, data, size)) {
>
> Not clear why this check is required?
Right, this could use a comment. This is a small workaround to having a
way to finding an arbitrary active filter, below in clear_one_vid(). We
pass NULL as a way of saying "return any enabled filter".
>
> > spin_unlock_irqrestore(&nc->lock, flags);
> > return index;
> > }
> > @@ -639,6 +655,83 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
> > nd->state = ncsi_dev_state_functional;
> > }
> >
> > +/* Check the VLAN filter bitmap for a set filter, and construct a
> > + * "Set VLAN Filter - Disable" packet if found.
> > + */
> > +static int clear_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
> > + struct ncsi_cmd_arg *nca)
> > +{
> > + int index;
> > + u16 vid;
> > +
> > + index = ncsi_find_filter(nc, NCSI_FILTER_VLAN, NULL);
> > + if (index < 0) {
> > + /* Filter table empty */
> > + return -1;
> > + }
> > +
> > + vid = *(u16 *)ncsi_get_filter(nc, NCSI_FILTER_VLAN, index);
>
> You just added this function that returns a pointer to a u32. It's
> strange to see the only call site then throw half of it away.
The problem here is that the single ncsi_channel_filter struct is used to
represent several different filters, and the filter data is stored in a
u32 data[] type. We cast to u16 in clear_one_vid because we know it's a
VLAN tag (NCSI_FILTER_VLAN), although we probably should call
ncsi_filter_size() to be sure.
>
> Also, ncsi_get_filter can return NULL.
That is indeed a problem though!
>
> > + netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> > + "ncsi: removed vlan tag %u at index %d\n",
> > + vid, index + 1);
> > + ncsi_remove_filter(nc, NCSI_FILTER_VLAN, index);
> > +
> > + nca->type = NCSI_PKT_CMD_SVF;
> > + nca->words[1] = vid;
> > + /* HW filter index starts at 1 */
> > + nca->bytes[6] = index + 1;
> > + nca->bytes[7] = 0x00;
> > + return 0;
> > +}
> > @@ -751,6 +876,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;
>
> This looks strange. What's nc->state up to? Does setting it to
> NCSI_CHANNEL_INVISIBLE have any affect?
>
> The locking is confusing too.
The locking IS confusing!
Here I've followed existing convention from a similar case in ncsi-aen,
but I'm not convinced it's correct. Likely the following would still have
the desired effect:
nc->reconfigure_needed = false;
nc->state = NCSI_CHANNEL_INACTIVE;
spin_unlock_irqrestore(&nc->lock, flags);
spin_lock_irqsave(&ndp->lock, flags);
list_add_tail_rcu(&nc->link, &ndp->channel_queue);
spin_unlock_irqrestore(&ndp->lock, flags);
And something similar in ncsi_kick_channels().
Ideally I would ask the author Gavin, but in lieu of that I'll need to
pick it apart a bit to see if the ncsi-aen code is doing something
important or is just wrong.
>
> > + list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> > + spin_unlock_irqrestore(&ndp->lock, flags);
> > +
> > + netdev_printk(KERN_DEBUG, 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 +1336,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_printk(KERN_DEBUG, nd->dev,
> > + "ncsi: channel %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;
>
> This is a similar pattern to ncsi_configure_channel. I suspect the
> answer there will be the same as here.
>
> > + list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> > + spin_unlock_irqrestore(&ndp->lock, flags);
> > +
> > + netdev_printk(KERN_DEBUG, nd->dev,
> > + "ncsi: kicked channel %p\n", nc);
> > + n++;
> > + }
> > + }
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-08-14 4:23 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-14 1:29 [PATCH net-next v2 0/3] NCSI VLAN Filtering Support Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 1/3] net/ncsi: Fix several packet definitions Samuel Mendoza-Jonas
2017-08-14 2:10 ` Joel Stanley
2017-08-14 2:10 ` Joel Stanley
2017-08-14 1:29 ` [PATCH net-next v2 2/3] net/ncsi: Configure VLAN tag filter Samuel Mendoza-Jonas
2017-08-14 2:09 ` Joel Stanley
2017-08-14 2:09 ` Joel Stanley
2017-08-14 4:23 ` Samuel Mendoza-Jonas
2017-08-14 1:29 ` [PATCH net-next v2 3/3] ftgmac100: Support NCSI VLAN filtering when available Samuel Mendoza-Jonas
2017-08-14 2:16 ` Joel Stanley
2017-08-14 2:16 ` Joel Stanley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.