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

[Samuel Mendoza-Jonas <sam@mendozajonas.com>]

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