> -----Original Message-----
> From: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
> Sent: Thursday, December 12, 2024 2:26 PM
> To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>; Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Jason Gunthorpe <jgg@xxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Remove direct link to net_device
>
> 在 2024/12/12 11:20, Bernard Metzler 写道:
> >
> >
> >> -----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?
>
> I agree with you. In the following link, I add del_link call. If you all
> agree, I can separate this patch from the patchset
> https%
> 3A__patchwork.kernel.org_project_linux-2Drdma_cover_20230624073927.707915-
> 2D1-2Dyanjun.zhu-
> 40intel.com_&d=DwIDaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=4ynb4Sj_4MUcZXbhvovE4tYSbq
> xyOwdSiLedP4yO55g&m=ZZJw4ol3n9SvffK8DJ1lwRkQXBPZ-qpbQp6X_oCD_cew0-
> 58ycdbtP0xYixv2ZHU&s=4VqxUZZs6UW1_XUZD01FoLNvoG1V-ETdi64KqLONDeI&e= .
>
>
Ah, nldev dellink is already there but no rdma_link_ops.dellink().
It just calls ib_unregister_device(), which is sufficient for siw,
but rxe want to do some extra work.
Thanks,
Bernard.
