| Message ID | 168573386075.5660.5037682341906748826.stgit@oracle-102.nfsv4bat.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] RDMA/core: Handle ARPHRD_NONE devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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); } Tom.
> 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. 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. -- Chuck Lever
> -----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.
> On Jun 3, 2023, at 9:51 AM, Bernard Metzler <BMT@zurich.ibm.com> wrote: > > > >> -----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. I have a patch that fabricates a proper GID here that I can post separately. >> 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. -- Chuck Lever
> -----Original Message----- > From: Chuck Lever III <chuck.lever@oracle.com> > Sent: Saturday, 3 June 2023 15:53 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: Tom Talpey <tom@talpey.com>; Chuck Lever <cel@kernel.org>; Jason > Gunthorpe <jgg@nvidia.com>; linux-rdma <linux-rdma@vger.kernel.org>; > netdev@vger.kernel.org > Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices > > > > > On Jun 3, 2023, at 9:51 AM, Bernard Metzler <BMT@zurich.ibm.com> wrote: > > > > > > > >> -----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. > > I have a patch that fabricates a proper GID here that I can > post separately. > Sounds good! > > >> 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. > > > -- > Chuck Lever >
On Sat, Jun 03, 2023 at 01:51:10PM +0000, Bernard Metzler wrote: > 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. GIDs are part of verbs, but I'm not really sure how iwarp fit itself into verbs with the mandatory CM and all. Possibly the only purpose of the GID is to convey some information from the CM to uverbs layer. Jason
On Fri, Jun 02, 2023 at 03:24:30PM -0400, 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); It seems believable, should it be locked to iwarp devices? More broadly, should iwarp devices just always do this and skip all the rest of it? I think it also has to check that the returned netdev in the sgid_attr matches the egress netdev selected? Jason
> On Jun 6, 2023, at 11:48 AM, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Fri, Jun 02, 2023 at 03:24:30PM -0400, 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); > > It seems believable, should it be locked to iwarp devices? > > More broadly, should iwarp devices just always do this and skip all > the rest of it? > > I think it also has to check that the returned netdev in the sgid_attr > matches the egress netdev selected? Both @ndev and sgid_attr.ndev are NULL here. So I assume you want the code to check, on return, that sgid_attr.device == device? -- Chuck Lever
On Tue, Jun 06, 2023 at 08:15:36PM +0000, Chuck Lever III wrote: > > > > On Jun 6, 2023, at 11:48 AM, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > On Fri, Jun 02, 2023 at 03:24:30PM -0400, 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); > > > > It seems believable, should it be locked to iwarp devices? > > > > More broadly, should iwarp devices just always do this and skip all > > the rest of it? > > > > I think it also has to check that the returned netdev in the sgid_attr > > matches the egress netdev selected? > > Both @ndev and sgid_attr.ndev are NULL here. The nedev to check is the dev_addr->bound_dev_if, that represents the netdev It is some iwarp mistake that the sgid attr's don't have proper netdevs, that is newer than iwarp so it never got updated. So, maybe it is too hard to fix for this, and maybe we can just assume it all has to be right Jason
> On Jun 6, 2023, at 7:17 PM, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Tue, Jun 06, 2023 at 08:15:36PM +0000, Chuck Lever III wrote: >> >> >>> On Jun 6, 2023, at 11:48 AM, Jason Gunthorpe <jgg@nvidia.com> wrote: >>> >>> On Fri, Jun 02, 2023 at 03:24:30PM -0400, 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); >>> >>> It seems believable, should it be locked to iwarp devices? >>> >>> More broadly, should iwarp devices just always do this and skip all >>> the rest of it? >>> >>> I think it also has to check that the returned netdev in the sgid_attr >>> matches the egress netdev selected? >> >> Both @ndev and sgid_attr.ndev are NULL here. > > The nedev to check is the dev_addr->bound_dev_if, that represents the netdev > > It is some iwarp mistake that the sgid attr's don't have proper > netdevs, that is newer than iwarp so it never got updated. > > So, maybe it is too hard to fix for this, and maybe we can just assume > it all has to be right sgid_attr.index is not set it looks like. I will send you an "official" version of this patch and we can go from there. -- Chuck Lever
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; }