RE: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices

[Bernard Metzler <BMT@zurich.ibm.com>]

> -----Original Message-----
> From: Chuck Lever III <chuck.lever@oracle.com>
> Sent: Saturday, 3 June 2023 02:33
> To: Tom Talpey <tom@talpey.com>
> Cc: Chuck Lever <cel@kernel.org>; Jason Gunthorpe <jgg@nvidia.com>; linux-
> rdma <linux-rdma@vger.kernel.org>; Bernard Metzler <BMT@zurich.ibm.com>;
> netdev@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices
> 
> 
> 
> > On Jun 2, 2023, at 6:18 PM, Tom Talpey <tom@talpey.com> wrote:
> >
> > On 6/2/2023 3:24 PM, Chuck Lever wrote:
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >> We would like to enable the use of siw on top of a VPN that is
> >> constructed and managed via a tun device. That hasn't worked up
> >> until now because ARPHRD_NONE devices (such as tun devices) have
> >> no GID for the RDMA/core to look up.
> >> But it turns out that the egress device has already been picked for
> >> us. addr_handler() just has to do the right thing with it.
> >> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >>  drivers/infiniband/core/cma.c |    4 ++++
> >>  1 file changed, 4 insertions(+)
> >> diff --git a/drivers/infiniband/core/cma.c
> b/drivers/infiniband/core/cma.c
> >> index 56e568fcd32b..3351dc5afa17 100644
> >> --- a/drivers/infiniband/core/cma.c
> >> +++ b/drivers/infiniband/core/cma.c
> >> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32
> port,
> >>   ndev = dev_get_by_index(dev_addr->net, bound_if_index);
> >>   if (!ndev)
> >>   return ERR_PTR(-ENODEV);
> >> + } else if (dev_type == ARPHRD_NONE) {
> >> + sgid_attr = rdma_get_gid_attr(device, port, 0);
> >> + goto out;
> >>   } else {
> >>   gid_type = IB_GID_TYPE_IB;
> >>   }
> >>     sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port,
> ndev);
> >> +out:
> >>   dev_put(ndev);
> >>   return sgid_attr;
> >>  }
> >
> > I like it, but doesn't this test in siw_main.c also need to change?
> >
> > static struct siw_device *siw_device_create(struct net_device *netdev)
> > {
> > ...
> > --> if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
> > addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
> >    netdev->dev_addr);
> > } else {
> > /*
> > * This device does not have a HW address,
> > * but connection mangagement lib expects gid != 0
> > */
> > size_t len = min_t(size_t, strlen(base_dev->name), 6);
> > char addr[6] = { };
> >
> > memcpy(addr, base_dev->name, len);
> > addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
> >    addr);
> > }
> 
> I'm not sure that code does anything. The base_dev's name field
> is actually not initialized at that point, so nothing is copied
> here.
> 
Oh in that case it’s an issue here.

> If you're asking whether siw needs to build a non-zero GID to
> make the posted patch work, more testing is needed; but I don't
> believe the GID has any relevance -- the egress ib_device is
> selected based entirely on the source IP address in this case.
> 

The whole GID based address resolution I think is an
artefact of IB/RoCE address handling. iWarp is supposed to
run on TCP streams, which endpoints are well defined by L3
addresses. IP routing shall define the outgoing interface...
siw tries to play well and invents GIDs to satisfy
the RDMA core concepts. But a GID is not part of the iWarp
concept. I am not sure for 'real' HW iWarp devices, but to
me it looks like the iwcm code could be done more
independently, if no application expects valid GIDs.