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.
>
> I just wanted to avoid the costly ib_device_get_netdev() call
> during query_qp(), query_port() and listen().
>
> Thank you!
> Bernard.
>
