[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 at 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 at 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;
> +       }
>