> -----Original Message-----
> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Tuesday, December 10, 2024 8:13 PM
> To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Remove direct link to net_device
>
> On Tue, Dec 10, 2024 at 05:08:51PM +0000, Bernard Metzler wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > > Sent: Tuesday, December 10, 2024 3:56 PM
> > > To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
> > > Cc: linux-rdma@xxxxxxxxxxxxxxx; leon@xxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; syzkaller-
> > > bugs@xxxxxxxxxxxxxxxx; zyjzyj2000@xxxxxxxxx;
> > > syzbot+4b87489410b4efd181bf@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Remove direct link to
> net_device
> > >
> > > On Tue, Dec 10, 2024 at 02:03:51PM +0100, Bernard Metzler wrote:
> > > > diff --git a/drivers/infiniband/sw/siw/siw.h
> > > b/drivers/infiniband/sw/siw/siw.h
> > > > index 86d4d6a2170e..c8f75527b513 100644
> > > > --- a/drivers/infiniband/sw/siw/siw.h
> > > > +++ b/drivers/infiniband/sw/siw/siw.h
> > > > @@ -69,16 +69,19 @@ struct siw_pd {
> > > >
> > > > struct siw_device {
> > > > struct ib_device base_dev;
> > > > - struct net_device *netdev;
> > > > struct siw_dev_cap attrs;
> > > >
> > > > u32 vendor_part_id;
> > > > + struct {
> > > > + int ifindex;
> > >
> > > ifindex is only stable so long as you are holding a reference on the
> > > netdev..
> > > > --- a/drivers/infiniband/sw/siw/siw_main.c
> > > > +++ b/drivers/infiniband/sw/siw/siw_main.c
> > > > @@ -287,7 +287,6 @@ static struct siw_device
> *siw_device_create(struct
> > > net_device *netdev)
> > > > return NULL;
> > > >
> > > > base_dev = &sdev->base_dev;
> > > > - sdev->netdev = netdev;
> > >
> > > Like here needed to grab a reference before storing the pointer in the
> > > sdev struct.
> > >
> >
> > This patch was supposed to remove siw's link to netdev. So no
> > reference to netdev would be needed. I did it under the
> > assumption siw can locally keep all needed information up to
> > date via netdev_event().
> > But it seems the netdev itself can change during the lifetime of
> > a siw device? With that ifindex would become wrong.
> >
> > If the netdev can change without the siw driver being informed,
> > holding a netdev pointer or reference seems useless.
> >
> > So it would be best to always use ib_device_get_netdev() to get
> > a valid netdev pointer, if current netdev info is needed by the
> > driver?
>
> Or call to dev_hold(netdev) in siw_device_create(). It will make sure
> that netdev is stable.
>
> Thanks
>
> BTW, Need to check, maybe IB/core layer already called to dev_hold.
>
Yes, drivers are calling ib_device_set_netdev(ibdev, netdev, port)
during newlink(), which assigns the netdev to the ib_device's port
info. The ibdev takes a hold on the ports netdev and handles the
pointer assignment, clearing etc. appropriately. Unlinking is done
via ib_device_set_netdev(, NULL, ), or ib_unregister_device()
which unlinks and puts netdevs as well.
Given we have an instance taking care of the netdev, it is
probably best to remove all direct netdev references from the
driver - just to avoid replicating all its complex handling.
So siw will use ib_device_get_netdev() whenever netdev info
is required. It comes with some extra overhead, but its's more
consistent.
BTW: The rdma_link interface is asymmetric, lacking an unlink()
call. For using software only drivers it might be beneficial to
allow explicit unlinking a driver from a device. Any thoughts
on that?
Best,
Bernard.
> >
> > I just wanted to avoid the costly ib_device_get_netdev() call
> > during query_qp(), query_port() and listen().
> >
> > Thank you!
> > Bernard.
> >
