Messages in this thread Patch in this message | | | From | <> | Subject | RE: [RFC PATCH 2/2] net/ncsi: Configure multi-package, multi-channel modes with failover | Date | Thu, 11 Oct 2018 22:09:02 +0000 |
| |
Hi Sam,
Please see my comments below.
Thanks, Justin
> On Wed, 2018-10-10 at 22:36 +0000, Justin.Lee1@Dell.com wrote: > > Hi Samuel, > > > > I am still testing your change and have some comments below. > > > > Thanks, > > Justin > > > > > > > This patch extends the ncsi-netlink interface with two new commands and > > > three new attributes to configure multiple packages and/or channels at > > > once, and configure specific failover modes. > > > > > > NCSI_CMD_SET_PACKAGE mask and NCSI_CMD_SET_CHANNEL_MASK set a whitelist > > > of packages or channels allowed to be configured with the > > > NCSI_ATTR_PACKAGE_MASK and NCSI_ATTR_CHANNEL_MASK attributes > > > respectively. If one of these whitelists is set only packages or > > > channels matching the whitelist are considered for the channel queue in > > > ncsi_choose_active_channel(). > > > > > > These commands may also use the NCSI_ATTR_MULTI_FLAG to signal that > > > multiple packages or channels may be configured simultaneously. NCSI > > > hardware arbitration (HWA) must be available in order to enable > > > multi-package mode. Multi-channel mode is always available. > > > > > > If the NCSI_ATTR_CHANNEL_ID attribute is present in the > > > NCSI_CMD_SET_CHANNEL_MASK command the it sets the preferred channel as > > > with the NCSI_CMD_SET_INTERFACE command. The combination of preferred > > > channel and channel whitelist defines a primary channel and the allowed > > > failover channels. > > > If the NCSI_ATTR_MULTI_FLAG attribute is also present then the preferred > > > channel is configured for Tx/Rx and the other channels are enabled only > > > for Rx. > > > > > > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> > > > --- > > > include/uapi/linux/ncsi.h | 16 +++ > > > net/ncsi/internal.h | 11 +- > > > net/ncsi/ncsi-aen.c | 2 +- > > > net/ncsi/ncsi-manage.c | 138 ++++++++++++++++-------- > > > net/ncsi/ncsi-netlink.c | 217 +++++++++++++++++++++++++++++++++----- > > > net/ncsi/ncsi-rsp.c | 2 +- > > > 6 files changed, 312 insertions(+), 74 deletions(-) > > > > > > diff --git a/include/uapi/linux/ncsi.h b/include/uapi/linux/ncsi.h > > > index 4c292ecbb748..035fba1693f9 100644 > > > --- a/include/uapi/linux/ncsi.h > > > +++ b/include/uapi/linux/ncsi.h > > > @@ -23,6 +23,13 @@ > > > * optionally the preferred NCSI_ATTR_CHANNEL_ID. > > > * @NCSI_CMD_CLEAR_INTERFACE: clear any preferred package/channel combination. > > > * Requires NCSI_ATTR_IFINDEX. > > > + * @NCSI_CMD_SET_PACKAGE_MASK: set a whitelist of allowed packages. > > > + * @NCSI_CMD_SET_PACKAGE_MASK: set a whitelist of allowed channels. > > > + * Requires NCSI_ATTR_IFINDEX and NCSI_ATTR_PACKAGE_MASK. > > > + * @NCSI_CMD_SET_PACKAGE_MASK: set a whitelist of allowed channels. > > > + * Requires NCSI_ATTR_IFINDEX, NCSI_ATTR_PACKAGE_ID, and > > > + * NCSI_ATTR_CHANNEL_MASK. If NCSI_ATTR_CHANNEL_ID is present it sets > > > + * the primary channel. > > > * @NCSI_CMD_MAX: highest command number > > > */ > > > > There are some typo in the description. > > * @NCSI_CMD_SET_PACKAGE_MASK: set a whitelist of allowed packages. > > * Requires NCSI_ATTR_IFINDEX and NCSI_ATTR_PACKAGE_MASK. > > * @NCSI_CMD_SET_CHANNEL_MASK: set a whitelist of allowed channels. > > * Requires NCSI_ATTR_IFINDEX, NCSI_ATTR_PACKAGE_ID, and > > * NCSI_ATTR_CHANNEL_MASK. If NCSI_ATTR_CHANNEL_ID is present it sets > > * the primary channel. > > Haha, yes I threw that in at the end, thanks for catching it. > > > > > > enum ncsi_nl_commands { > > > @@ -30,6 +37,8 @@ enum ncsi_nl_commands { > > > NCSI_CMD_PKG_INFO, > > > NCSI_CMD_SET_INTERFACE, > > > NCSI_CMD_CLEAR_INTERFACE, > > > + NCSI_CMD_SET_PACKAGE_MASK, > > > + NCSI_CMD_SET_CHANNEL_MASK, > > > > > > __NCSI_CMD_AFTER_LAST, > > > NCSI_CMD_MAX = __NCSI_CMD_AFTER_LAST - 1 > > > @@ -43,6 +52,10 @@ enum ncsi_nl_commands { > > > * @NCSI_ATTR_PACKAGE_LIST: nested array of NCSI_PKG_ATTR attributes > > > * @NCSI_ATTR_PACKAGE_ID: package ID > > > * @NCSI_ATTR_CHANNEL_ID: channel ID > > > + * @NCSI_ATTR_MULTI_FLAG: flag to signal that multi-mode should be enabled with > > > + * NCSI_CMD_SET_PACKAGE_MASK or NCSI_CMD_SET_CHANNEL_MASK. > > > + * @NCSI_ATTR_PACKAGE_MASK: 32-bit mask of allowed packages. > > > + * @NCSI_ATTR_CHANNEL_MASK: 32-bit mask of allowed channels. > > > * @NCSI_ATTR_MAX: highest attribute number > > > */ > > > enum ncsi_nl_attrs { > > > @@ -51,6 +64,9 @@ enum ncsi_nl_attrs { > > > NCSI_ATTR_PACKAGE_LIST, > > > NCSI_ATTR_PACKAGE_ID, > > > NCSI_ATTR_CHANNEL_ID, > > > + NCSI_ATTR_MULTI_FLAG, > > > + NCSI_ATTR_PACKAGE_MASK, > > > + NCSI_ATTR_CHANNEL_MASK, > > > > Is there a case that we might set these two masks at the same time? > > If not, maybe we can just have one generic MASK attribute. > > > > I thought of this too: not yet, but I wonder if we might in the future. > I'll have a think about it. > > > > > > > __NCSI_ATTR_AFTER_LAST, > > > NCSI_ATTR_MAX = __NCSI_ATTR_AFTER_LAST - 1 > > > diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h > > > index 3d0a33b874f5..8437474d0a78 100644 > > > --- a/net/ncsi/internal.h > > > +++ b/net/ncsi/internal.h > > > @@ -213,6 +213,10 @@ struct ncsi_package { > > > unsigned int channel_num; /* Number of channels */ > > > struct list_head channels; /* List of chanels */ > > > struct list_head node; /* Form list of packages */ > > > + > > > + bool multi_channel; /* Enable multiple channels */ > > > + u32 channel_whitelist; /* Channels to configure */ > > > + struct ncsi_channel *preferred_channel; /* Primary channel */ > > > }; > > > > > > struct ncsi_request { > > > @@ -280,8 +284,6 @@ struct ncsi_dev_priv { > > > unsigned int package_num; /* Number of packages */ > > > struct list_head packages; /* List of packages */ > > > struct ncsi_channel *hot_channel; /* Channel was ever active */ > > > - struct ncsi_package *force_package; /* Force a specific package */ > > > - struct ncsi_channel *force_channel; /* Force a specific channel */ > > > struct ncsi_request requests[256]; /* Request table */ > > > unsigned int request_id; /* Last used request ID */ > > > #define NCSI_REQ_START_IDX 1 > > > @@ -294,6 +296,9 @@ struct ncsi_dev_priv { > > > struct list_head node; /* Form NCSI device list */ > > > #define NCSI_MAX_VLAN_VIDS 15 > > > struct list_head vlan_vids; /* List of active VLAN IDs */ > > > + > > > + bool multi_package; /* Enable multiple packages */ > > > + u32 package_whitelist; /* Packages to configure */ > > > }; > > > > > > struct ncsi_cmd_arg { > > > @@ -345,6 +350,8 @@ struct ncsi_request *ncsi_alloc_request(struct ncsi_dev_priv *ndp, > > > void ncsi_free_request(struct ncsi_request *nr); > > > struct ncsi_dev *ncsi_find_dev(struct net_device *dev); > > > int ncsi_process_next_channel(struct ncsi_dev_priv *ndp); > > > +bool ncsi_channel_is_last(struct ncsi_dev_priv *ndp, > > > + struct ncsi_channel *channel); > > > > > > /* Packet handlers */ > > > u32 ncsi_calculate_checksum(unsigned char *data, int len); > > > diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c > > > index 65f47a648be3..eac56aee30c4 100644 > > > --- a/net/ncsi/ncsi-aen.c > > > +++ b/net/ncsi/ncsi-aen.c > > > @@ -86,7 +86,7 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp, > > > !(state == NCSI_CHANNEL_ACTIVE && !(data & 0x1))) > > > return 0; > > > > > > - if (state == NCSI_CHANNEL_ACTIVE) > > > + if (state == NCSI_CHANNEL_ACTIVE && ncsi_channel_is_last(ndp, nc)) > > > ndp->flags |= NCSI_DEV_RESHUFFLE; > > > > > > ncsi_stop_channel_monitor(nc); > > > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c > > > index 665bee25ec44..6a55df700bcb 100644 > > > --- a/net/ncsi/ncsi-manage.c > > > +++ b/net/ncsi/ncsi-manage.c > > > @@ -27,6 +27,24 @@ > > > LIST_HEAD(ncsi_dev_list); > > > DEFINE_SPINLOCK(ncsi_dev_lock); > > > > > > +/* Returns true if the given channel is the last channel available */ > > > +bool ncsi_channel_is_last(struct ncsi_dev_priv *ndp, > > > + struct ncsi_channel *channel) > > > +{ > > > + struct ncsi_package *np; > > > + struct ncsi_channel *nc; > > > + > > > + NCSI_FOR_EACH_PACKAGE(ndp, np) > > > + NCSI_FOR_EACH_CHANNEL(np, nc) { > > > + if (nc == channel) > > > + continue; > > > + if (nc->state == NCSI_CHANNEL_ACTIVE) > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > static void ncsi_report_link(struct ncsi_dev_priv *ndp, bool force_down) > > > { > > > struct ncsi_dev *nd = &ndp->ndev; > > > @@ -266,6 +284,7 @@ struct ncsi_package *ncsi_add_package(struct ncsi_dev_priv *ndp, > > > np->ndp = ndp; > > > spin_lock_init(&np->lock); > > > INIT_LIST_HEAD(&np->channels); > > > + np->channel_whitelist = UINT_MAX; > > > > > > spin_lock_irqsave(&ndp->lock, flags); > > > tmp = ncsi_find_package(ndp, id); > > > @@ -633,6 +652,34 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc, > > > return 0; > > > } > > > > > > +/* Determine if a given channel should be the Tx channel */ > > > +bool ncsi_channel_is_tx(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc) > > > +{ > > > + struct ncsi_package *np = nc->package; > > > + struct ncsi_channel *channel; > > > + struct ncsi_channel_mode *ncm;
Learn from Dave. All local variable declarations from longest to shortest line. :)
> > > + > > > + NCSI_FOR_EACH_CHANNEL(np, channel) { > > > + ncm = &channel->modes[NCSI_MODE_TX_ENABLE]; > > > + /* Another channel is already Tx */ > > > + if (ncm->enable) > > > + return false; > > > + } > > > + > > > + if (!np->preferred_channel) > > > + return true; > > > + > > > + if (np->preferred_channel == nc) > > > + return true; > > > + > > > + /* The preferred channel is not in the queue and not active */ > > > + if (list_empty(&np->preferred_channel->link) && > > > + np->preferred_channel->state != NCSI_CHANNEL_ACTIVE) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > static void ncsi_configure_channel(struct ncsi_dev_priv *ndp) > > > { > > > struct ncsi_dev *nd = &ndp->ndev; > > > @@ -745,18 +792,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp) > > > } else if (nd->state == ncsi_dev_state_config_ebf) { > > > nca.type = NCSI_PKT_CMD_EBF; > > > nca.dwords[0] = nc->caps[NCSI_CAP_BC].cap; > > > - nd->state = ncsi_dev_state_config_ecnt; > > > + if (ncsi_channel_is_tx(ndp, nc)) > > > + nd->state = ncsi_dev_state_config_ecnt; > > > + else > > > + nd->state = ncsi_dev_state_config_ec; > > > #if IS_ENABLED(CONFIG_IPV6) > > > if (ndp->inet6_addr_num > 0 && > > > (nc->caps[NCSI_CAP_GENERIC].cap & > > > NCSI_CAP_GENERIC_MC)) > > > nd->state = ncsi_dev_state_config_egmf; > > > - else > > > - nd->state = ncsi_dev_state_config_ecnt; > > > } else if (nd->state == ncsi_dev_state_config_egmf) { > > > nca.type = NCSI_PKT_CMD_EGMF; > > > nca.dwords[0] = nc->caps[NCSI_CAP_MC].cap; > > > - nd->state = ncsi_dev_state_config_ecnt; > > > + if (ncsi_channel_is_tx(ndp, nc)) > > > + nd->state = ncsi_dev_state_config_ecnt; > > > + else > > > + nd->state = ncsi_dev_state_config_ec; > > > #endif /* CONFIG_IPV6 */ > > > } else if (nd->state == ncsi_dev_state_config_ecnt) { > > > nca.type = NCSI_PKT_CMD_ECNT; > > > @@ -840,43 +891,35 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp) > > > > > > static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp) > > > { > > > - struct ncsi_package *np, *force_package; > > > - struct ncsi_channel *nc, *found, *hot_nc, *force_channel; > > > + struct ncsi_package *np; > > > + struct ncsi_channel *nc, *found, *hot_nc; > > > struct ncsi_channel_mode *ncm; > > > - unsigned long flags; > > > + unsigned long flags, cflags; > > > + bool with_link;
All local variable declarations from longest to shortest line.
> > > > > > spin_lock_irqsave(&ndp->lock, flags); > > > hot_nc = ndp->hot_channel; > > > - force_channel = ndp->force_channel; > > > - force_package = ndp->force_package; > > > spin_unlock_irqrestore(&ndp->lock, flags); > > > > > > - /* Force a specific channel whether or not it has link if we have been > > > - * configured to do so > > > - */ > > > - if (force_package && force_channel) { > > > - found = force_channel; > > > - ncm = &found->modes[NCSI_MODE_LINK]; > > > - if (!(ncm->data[2] & 0x1)) > > > - netdev_info(ndp->ndev.dev, > > > - "NCSI: Channel %u forced, but it is link down\n", > > > - found->id); > > > - goto out; > > > - } > > > - > > > - /* The search is done once an inactive channel with up > > > - * link is found. > > > + /* By default the search is done once an inactive channel with up > > > + * link is found, unless a preferred channel is set. > > > + * If multi_package or multi_channel are configured all channels in the > > > + * whitelist with link are added to the channel queue. > > > */ > > > found = NULL; > > > + with_link = false; > > > NCSI_FOR_EACH_PACKAGE(ndp, np) { > > > - if (ndp->force_package && np != ndp->force_package) > > > + if (!(ndp->package_whitelist & (0x1 << np->id))) > > > continue; > > > NCSI_FOR_EACH_CHANNEL(np, nc) { > > > - spin_lock_irqsave(&nc->lock, flags); > > > + if (!(np->channel_whitelist & (0x1 << nc->id))) > > > + continue; > > > + > > > + spin_lock_irqsave(&nc->lock, cflags); > > > > > > if (!list_empty(&nc->link) || > > > nc->state != NCSI_CHANNEL_INACTIVE) { > > > - spin_unlock_irqrestore(&nc->lock, flags); > > > + spin_unlock_irqrestore(&nc->lock, cflags); > > > continue; > > > } > > > > > > @@ -888,32 +931,42 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp) > > > > > > ncm = &nc->modes[NCSI_MODE_LINK]; > > > if (ncm->data[2] & 0x1) { > > > > This data will not be updated if the channel monitor for it is not running. > > If I move the cable from the current configured channel to the other channel, > > NC-SI module will not detect the link status as the other channel is not configured > > and AEN will not happen. > > Is it per design that NC-SI module will always use the first interface with the link? > > We'll check the link state of every channel at init, and per-package on > suspend events, but you're right that we only get AENs right now from > configured channels. There's probably no reason why we could at least > enable AENs on all channels even if we don't use them for network, maybe > during the package probe step. >
To receive the AEN packet, the channel (RX) needs to be enabled. It seems that we need to send Get Link Status command to all channels before choosing The active channel.
> > > > > - spin_unlock_irqrestore(&nc->lock, flags); > > > found = nc; > > > - goto out; > > > + with_link = true; > > > + > > > + spin_lock_irqsave(&ndp->lock, flags); > > > + list_add_tail_rcu(&found->link, > > > + &ndp->channel_queue); > > > + spin_unlock_irqrestore(&ndp->lock, flags); > > > + > > > + netdev_dbg(ndp->ndev.dev, > > > + "NCSI: Channel %u added to queue (link %s)\n", > > > + found->id, > > > + ncm->data[2] & 0x1 ? "up" : "down"); > > > }
If multi_channel is enabled, the interface without link still needs to be processed. Something likes below? } else if (np->multi_channel) { found = nc;
spin_lock_irqsave(&ndp->lock, flags); list_add_tail_rcu(&found->link, &ndp->channel_queue); spin_unlock_irqrestore(&ndp->lock, flags);
netdev_dbg(ndp->ndev.dev, "NCSI: pkg %u ch %u added to queue (link %s)\n", found->package->id, found->id, ncm->data[2] & 0x1 ? "up" : "down"); }
> > > + spin_unlock_irqrestore(&nc->lock, cflags); > > > > > > - spin_unlock_irqrestore(&nc->lock, flags); > > > + if (with_link && !np->multi_channel) > > > + break; > > > } > > > + if (with_link && !ndp->multi_package) > > > + break; > > > } > > > > > > - if (!found) { > > > + if (!with_link && found) { > > > + netdev_info(ndp->ndev.dev, > > > + "NCSI: No channel with link found, configuring channel %u\n", > > > + found->id); > > > + spin_lock_irqsave(&ndp->lock, flags); > > > + list_add_tail_rcu(&found->link, &ndp->channel_queue); > > > + spin_unlock_irqrestore(&ndp->lock, flags); > > > + } else if (!found) { > > > netdev_warn(ndp->ndev.dev, > > > - "NCSI: No channel found with link\n"); > > > + "NCSI: No channel found to configure!\n"); > > > ncsi_report_link(ndp, true); > > > return -ENODEV; > > > } > > > > > > - ncm = &found->modes[NCSI_MODE_LINK]; > > > - netdev_dbg(ndp->ndev.dev, > > > - "NCSI: Channel %u added to queue (link %s)\n", > > > - found->id, ncm->data[2] & 0x1 ? "up" : "down"); > > > - > > > -out: > > > - spin_lock_irqsave(&ndp->lock, flags); > > > - list_add_tail_rcu(&found->link, &ndp->channel_queue); > > > - spin_unlock_irqrestore(&ndp->lock, flags); > > > - > > > return ncsi_process_next_channel(ndp); > > > } > > > > > > @@ -1428,6 +1481,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev, > > > INIT_LIST_HEAD(&ndp->channel_queue); > > > INIT_LIST_HEAD(&ndp->vlan_vids); > > > INIT_WORK(&ndp->work, ncsi_dev_work); > > > + ndp->package_whitelist = UINT_MAX; > > > > > > /* Initialize private NCSI device */ > > > spin_lock_init(&ndp->lock); > > > diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c > > > index 32cb7751d216..33a091e6f466 100644 > > > --- a/net/ncsi/ncsi-netlink.c > > > +++ b/net/ncsi/ncsi-netlink.c > > > > Is the following missed in the patch? > > static const struct nla_policy ncsi_genl_policy[NCSI_ATTR_MAX + 1] = { > > ... > > [NCSI_ATTR_MULTI_FLAG] = { .type = NLA_FLAG }, > > [NCSI_ATTR_PACKAGE_MASK] = { .type = NLA_U32 }, > > [NCSI_ATTR_CHANNEL_MASK] = { .type = NLA_U32 }, > > Oops, added. > > > > > > @@ -67,7 +67,7 @@ static int ncsi_write_channel_info(struct sk_buff *skb, > > > nla_put_u32(skb, NCSI_CHANNEL_ATTR_LINK_STATE, m->data[2]); > > > if (nc->state == NCSI_CHANNEL_ACTIVE) > > > nla_put_flag(skb, NCSI_CHANNEL_ATTR_ACTIVE); > > > - if (ndp->force_channel == nc) > > > + if (nc == nc->package->preferred_channel) > > > nla_put_flag(skb, NCSI_CHANNEL_ATTR_FORCED); > > > > > > nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MAJOR, nc->version.version); > > > @@ -112,7 +112,7 @@ static int ncsi_write_package_info(struct sk_buff *skb, > > > if (!pnest) > > > return -ENOMEM; > > > nla_put_u32(skb, NCSI_PKG_ATTR_ID, np->id); > > > - if (ndp->force_package == np) > > > + if ((0x1 << np->id) == ndp->package_whitelist) > > > nla_put_flag(skb, NCSI_PKG_ATTR_FORCED); > > > cnest = nla_nest_start(skb, NCSI_PKG_ATTR_CHANNEL_LIST); > > > if (!cnest) { > > > @@ -288,45 +288,54 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info) > > > package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]); > > > package = NULL; > > > > > > - spin_lock_irqsave(&ndp->lock, flags); > > > - > > > NCSI_FOR_EACH_PACKAGE(ndp, np) > > > if (np->id == package_id) > > > package = np; > > > if (!package) { > > > /* The user has set a package that does not exist */ > > > - spin_unlock_irqrestore(&ndp->lock, flags); > > > return -ERANGE; > > > } > > > > > > channel = NULL; > > > - if (!info->attrs[NCSI_ATTR_CHANNEL_ID]) { > > > - /* Allow any channel */ > > > - channel_id = NCSI_RESERVED_CHANNEL; > > > - } else { > > > + if (info->attrs[NCSI_ATTR_CHANNEL_ID]) { > > > channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]); > > > NCSI_FOR_EACH_CHANNEL(package, nc) > > > - if (nc->id == channel_id) > > > + if (nc->id == channel_id) { > > > channel = nc; > > > + break; > > > + } > > > + if (!channel) { > > > + netdev_info(ndp->ndev.dev, > > > + "NCSI: Channel %u does not exist!\n", > > > + channel_id); > > > + return -ERANGE; > > > + } > > > } > > > > > > - if (channel_id != NCSI_RESERVED_CHANNEL && !channel) { > > > - /* The user has set a channel that does not exist on this > > > - * package > > > - */ > > > - spin_unlock_irqrestore(&ndp->lock, flags); > > > - netdev_info(ndp->ndev.dev, "NCSI: Channel %u does not exist!\n", > > > - channel_id); > > > - return -ERANGE; > > > - } > > > - > > > - ndp->force_package = package; > > > - ndp->force_channel = channel; > > > + spin_lock_irqsave(&ndp->lock, flags); > > > + ndp->package_whitelist = 0x1 << package->id; > > > + ndp->multi_package = false; > > > spin_unlock_irqrestore(&ndp->lock, flags); > > > > > > - netdev_info(ndp->ndev.dev, "Set package 0x%x, channel 0x%x%s as preferred\n", > > > - package_id, channel_id, > > > - channel_id == NCSI_RESERVED_CHANNEL ? " (any)" : ""); > > > + spin_lock_irqsave(&package->lock, flags); > > > + package->multi_channel = false; > > > + if (channel) { > > > + package->channel_whitelist = 0x1 << channel->id; > > > + package->preferred_channel = channel; > > > + } else { > > > + /* Allow any channel */ > > > + package->channel_whitelist = UINT_MAX; > > > + package->preferred_channel = NULL; > > > + } > > > + spin_unlock_irqrestore(&package->lock, flags); > > > + > > > + if (channel) > > > + netdev_info(ndp->ndev.dev, > > > + "Set package 0x%x, channel 0x%x as preferred\n", > > > + package_id, channel_id); > > > + else > > > + netdev_info(ndp->ndev.dev, "Set package 0x%x as preferred\n", > > > + package_id); > > > > > > /* Bounce the NCSI channel to set changes */ > > > ncsi_stop_dev(&ndp->ndev); > > > @@ -338,6 +347,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info) > > > static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info) > > > { > > > struct ncsi_dev_priv *ndp; > > > + struct ncsi_package *np; > > > unsigned long flags; > > > > > > if (!info || !info->attrs) > > > @@ -351,11 +361,19 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info) > > > if (!ndp) > > > return -ENODEV; > > > > > > - /* Clear any override */ > > > + /* Reset any whitelists and disable multi mode */ > > > spin_lock_irqsave(&ndp->lock, flags); > > > - ndp->force_package = NULL; > > > - ndp->force_channel = NULL; > > > + ndp->package_whitelist = UINT_MAX; > > > + ndp->multi_package = false; > > > spin_unlock_irqrestore(&ndp->lock, flags); > > > + > > > + NCSI_FOR_EACH_PACKAGE(ndp, np) { > > > + spin_lock_irqsave(&np->lock, flags); > > > + np->multi_channel = false; > > > + np->channel_whitelist = UINT_MAX; > > > + np->preferred_channel = NULL; > > > + spin_unlock_irqrestore(&np->lock, flags); > > > + } > > > netdev_info(ndp->ndev.dev, "NCSI: Cleared preferred package/channel\n"); > > > > > > /* Bounce the NCSI channel to set changes */ > > > @@ -365,6 +383,137 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info) > > > return 0; > > > } > > > > > > +static int ncsi_set_package_mask_nl(struct sk_buff *msg, > > > + struct genl_info *info) > > > +{ > > > + struct ncsi_dev_priv *ndp; > > > + unsigned long flags; > > > + int rc; > > > + > > > + if (!info || !info->attrs) > > > + return -EINVAL; > > > + > > > + if (!info->attrs[NCSI_ATTR_IFINDEX]) > > > + return -EINVAL; > > > + > > > + if (!info->attrs[NCSI_ATTR_PACKAGE_MASK]) > > > + return -EINVAL; > > > + > > > + ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)), > > > + nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX])); > > > + if (!ndp) > > > + return -ENODEV; > > > + > > > + spin_lock_irqsave(&ndp->lock, flags); > > > + ndp->package_whitelist = > > > + nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_MASK]); > > > + > > > + if (nla_get_flag(info->attrs[NCSI_ATTR_MULTI_FLAG])) { > > > + if (ndp->flags & NCSI_DEV_HWA) { > > > + ndp->multi_package = true; > > > + rc = 0; > > > + } else { > > > + netdev_err(ndp->ndev.dev, > > > + "NCSI: Can't use multiple packages without HWA\n"); > > > + rc = -EPERM; > > > + } > > > + } else { > > > + rc = 0; > > > + }
If the flag is not set, do we need to clear the multi_package flag? It is possible that it is user's intension to disable the multi-mode. } else { ndp->multi_package = false; rc = 0; }
> > > + > > > + spin_unlock_irqrestore(&ndp->lock, flags); > > > + > > > + if (!rc) { > > > + /* Bounce the NCSI channel to set changes */ > > > + ncsi_stop_dev(&ndp->ndev); > > > + ncsi_start_dev(&ndp->ndev); > > > > Is it possible to delay the restart? If we have two packages, we might send > > set_package_mask command once and set_channel_mask command twice. > > We will see the unnecessary reconfigurations in a very short period time. > > We could probably do with a better way of bouncing the link here too. I > suppose we could schedule the actual link 'bounce' to occur a second or > two later, and delay if we receive new configuration in that time. > Perhaps something outside of the scope of this patch though. > > > > > > + } > > > + > > > + return rc; > > > +} > > > + > > > +static int ncsi_set_channel_mask_nl(struct sk_buff *msg, > > > + struct genl_info *info) > > > +{ > > > + struct ncsi_package *np, *package; > > > + struct ncsi_channel *nc, *channel; > > > + struct ncsi_dev_priv *ndp; > > > + unsigned long flags; > > > + u32 package_id, channel_id;
All local variable declarations from longest to shortest line.
> > > + > > > + if (!info || !info->attrs) > > > + return -EINVAL; > > > + > > > + if (!info->attrs[NCSI_ATTR_IFINDEX]) > > > + return -EINVAL; > > > + > > > + if (!info->attrs[NCSI_ATTR_PACKAGE_ID]) > > > + return -EINVAL; > > > + > > > + if (!info->attrs[NCSI_ATTR_CHANNEL_MASK]) > > > + return -EINVAL; > > > + > > > + ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)), > > > + nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX])); > > > + if (!ndp) > > > + return -ENODEV; > > > + > > > + package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]); > > > + package = NULL; > > > + NCSI_FOR_EACH_PACKAGE(ndp, np) > > > + if (np->id == package_id) { > > > + package = np; > > > + break; > > > + } > > > + if (!package) > > > + return -ERANGE; > > > + > > > + spin_lock_irqsave(&package->lock, flags); > > > + > > > + channel = NULL; > > > + if (info->attrs[NCSI_ATTR_CHANNEL_ID]) { > > > + channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]); > > > + NCSI_FOR_EACH_CHANNEL(np, nc) > > > + if (nc->id == channel_id) { > > > + channel = nc; > > > + break; > > > + } > > > + if (!channel) { > > > + spin_unlock_irqrestore(&package->lock, flags); > > > + return -ERANGE; > > > + } > > > + netdev_dbg(ndp->ndev.dev, > > > + "NCSI: Channel %u set as preferred channel\n", > > > + channel->id); > > > + } > > > + > > > + package->channel_whitelist = > > > + nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_MASK]); > > > + if (package->channel_whitelist == 0) > > > + netdev_dbg(ndp->ndev.dev, > > > + "NCSI: Package %u set to all channels disabled\n", > > > + package->id); > > > + > > > + package->preferred_channel = channel; > > > + > > > + if (nla_get_flag(info->attrs[NCSI_ATTR_MULTI_FLAG])) { > > > + package->multi_channel = true; > > > + netdev_info(ndp->ndev.dev, > > > + "NCSI: Multi-channel enabled on package %u\n", > > > + package_id); > > > + } else { > > > + package->multi_channel = false; > > > + } > > > + > > > + spin_unlock_irqrestore(&package->lock, flags); > > > + > > > + /* Bounce the NCSI channel to set changes */ > > > + ncsi_stop_dev(&ndp->ndev); > > > + ncsi_start_dev(&ndp->ndev); > > > > Same question as set_package_mask function. > > Is it possible to delay the restart? If we have two packages, we might send > > set_package_mask command once and set_channel_mask command twice. > > We will see the unnecessary reconfigurations in a very short period time. > > > > > + > > > + return 0; > > > +} > > > + > > > static const struct genl_ops ncsi_ops[] = { > > > { > > > .cmd = NCSI_CMD_PKG_INFO, > > > @@ -385,6 +534,18 @@ static const struct genl_ops ncsi_ops[] = { > > > .doit = ncsi_clear_interface_nl, > > > .flags = GENL_ADMIN_PERM, > > > }, > > > + { > > > + .cmd = NCSI_CMD_SET_PACKAGE_MASK, > > > + .policy = ncsi_genl_policy, > > > + .doit = ncsi_set_package_mask_nl, > > > + .flags = GENL_ADMIN_PERM, > > > + }, > > > + { > > > + .cmd = NCSI_CMD_SET_CHANNEL_MASK, > > > + .policy = ncsi_genl_policy, > > > + .doit = ncsi_set_channel_mask_nl, > > > + .flags = GENL_ADMIN_PERM, > > > + }, > > > }; > > > > > > static struct genl_family ncsi_genl_family __ro_after_init = { > > > diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c > > > index d66b34749027..02ce7626b579 100644 > > > --- a/net/ncsi/ncsi-rsp.c > > > +++ b/net/ncsi/ncsi-rsp.c > > > @@ -241,7 +241,7 @@ static int ncsi_rsp_handler_dcnt(struct ncsi_request *nr) > > > if (!ncm->enable) > > > return 0; > > > > > > - ncm->enable = 1; > > > + ncm->enable = 0; > > > return 0; > > > } > > > > > > -- > > > 2.19.0 > > > >
| |