Re: [PATCH for-rc v2] RDMA/ipoib: Set rtnl_link_ops for ipoib interfaces

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 04, 2020 at 10:04:54AM -0300, Jason Gunthorpe wrote:
> On Sun, Oct 04, 2020 at 03:51:07PM +0300, Kamal Heib wrote:
> > On Fri, Oct 02, 2020 at 09:02:36AM -0300, Jason Gunthorpe wrote:
> > > On Wed, Sep 30, 2020 at 12:40:13PM +0300, Kamal Heib wrote:
> > > > To avoid inconsistent user experience for PKey interfaces that are
> > > > created via netlink vs PKey interfaces that are created via sysfs and the
> > > > base interface, make sure to set the rtnl_link_ops for all ipoib network
> > > > devices, so the ipoib attributes will be reported/modified via iproute2
> > > > for all ipoib interfaces regardless of how they are created.
> > > > 
> > > > Also, after setting the rtnl_link_ops for the base interface, implement
> > > > the dellink() callback to block users from trying to remove it.
> > > > 
> > > > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
> > > > Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
> > > > v2: Update commit message.
> > > >  drivers/infiniband/ulp/ipoib/ipoib_main.c    |  2 ++
> > > >  drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 11 +++++++++++
> > > >  drivers/infiniband/ulp/ipoib/ipoib_vlan.c    |  2 ++
> > > >  3 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > index ab75b7f745d4..96b6be5d507d 100644
> > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > @@ -2477,6 +2477,8 @@ static struct net_device *ipoib_add_port(const char *format,
> > > >  	/* call event handler to ensure pkey in sync */
> > > >  	queue_work(ipoib_workqueue, &priv->flush_heavy);
> > > >  
> > > > +	ndev->rtnl_link_ops = ipoib_get_link_ops();
> > > > +
> > > >  	result = register_netdev(ndev);
> > > 
> > > Why do we need this one? I understand fixing the sysfs but not this part.
> > 
> > We need this one to report/modify the ipoib attributes for the parent
> > interface (please take a look at pkey, mode, and umcast after applying
> > this patch):
> > 
> > $ ip -d link show dev mlx5_ib0
> > 29: mlx5_ib0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc mq state UP mode DEFAULT group default qlen 256
> >     link/infiniband 00:00:1f:e4:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a3:19:64 brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 65520
> >     ipoib pkey  0xffff mode  datagram umcast  0000 addrgenmode none numtxqueues 256 numrxqueues 32 gso_max_size 65536 gso_max_segs 65535
> 
> So this is what you mean by "base interface" ? Hmm OK. Highlight this
> a bit more in the commit message since this is something everyone
> will see
>

Yes, correct.
I'll update the commit message.

Thanks,
Kamal

> > > >  static size_t ipoib_get_size(const struct net_device *dev)
> > > >  {
> > > >  	return nla_total_size(2) +	/* IFLA_IPOIB_PKEY   */
> > > > @@ -158,6 +168,7 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = {
> > > >  	.priv_size	= sizeof(struct ipoib_dev_priv),
> > > >  	.setup		= ipoib_setup_common,
> > > >  	.newlink	= ipoib_new_child_link,
> > > > +	.dellink	= ipoib_del_child_link,
> > > >  	.changelink	= ipoib_changelink,
> > > >  	.get_size	= ipoib_get_size,
> > > >  	.fill_info	= ipoib_fill_info,
> > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> > > > index 30865605e098..c60db9f3f5ac 100644
> > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> > > > @@ -129,6 +129,8 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
> > > >  		goto out_early;
> > > >  	}
> > > >  
> > > > +	ndev->rtnl_link_ops = ipoib_get_link_ops();
> > > > +
> > > 
> > > If this is only for the sysfs case why isn't it in ipoib_vlan_add()
> > > which is the sysfs only flow?
> > >
> > 
> > Basically, I was making sure set the rtnl_link_ops before calling
> > register_netdevice(), but you are right this can be moved to the
> > ipoib_vlan_add() as the setting of the rtnl_link_ops for pkey that is
> > created via netlink is done before calling newlink callback, I'll send a
> > v3 that move it to ipoib_vlan_add().
> 
> Please
> 
> Jason



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux