[PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Virtual devices like SIW or RXE don't set FW version because
they don't have one, use that fact to rely on having empty
fw_ver file to sense such virtual devices.

Such change is needed to ensure that virtual devices which are
attached to real hardware won't be renamed, because during
device attachment, user already supplied desired name.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 kernel-boot/rdma_rename.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/kernel-boot/rdma_rename.c b/kernel-boot/rdma_rename.c
index 41649d7d0..1588a0f45 100644
--- a/kernel-boot/rdma_rename.c
+++ b/kernel-boot/rdma_rename.c
@@ -356,9 +356,12 @@ err_free:
 
 static int by_pci(struct data *d)
 {
+	bool is_virtual = false;
 	struct pci_info p = {};
 	char *subsystem;
 	char buf[256] = {};
+	FILE *fw_ver_p;
+	char *fw_ver;
 	char *subs;
 	int ret;
 
@@ -373,9 +376,41 @@ static int by_pci(struct data *d)
 		goto out;
 	}
 	buf[ret] = 0;
-
 	subs = basename(buf);
-	if (strcmp(subs, "pci")) {
+
+	ret =  asprintf(&fw_ver, "/sys/class/infiniband/%s/fw_ver", d->curr);
+	if (ret < 0) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = 0;
+	fw_ver_p = fopen(fw_ver, "r");
+	if (fw_ver_p) {
+		char *s = fgets(buf, 3, fw_ver_p);
+
+		fclose(fw_ver_p);
+
+		if (!s)
+			/* Signal that we can't read fw_ver file */
+			ret = -EINVAL;
+		/*
+		 * Virtual devices like SIW and RXE
+		 * don't set their FW version
+		 */
+		if (strlen(s) < 2)
+			is_virtual = true;
+	} else {
+		ret = -EINVAL;
+	}
+
+	if (ret) {
+		/* Something very wrong, all IB devices have fw_ver file */
+		pr_err("%s: Can't open/read fw_ver file\n", d->curr);
+		goto out;
+	}
+
+	if (strcmp(subs, "pci") || is_virtual) {
 		/* Ball out virtual devices */
 		pr_dbg("%s: Non-PCI device (%s) was detected\n", d->curr, subs);
 		ret = -EINVAL;
-- 
2.20.1

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Leon Romanovsky]

On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Virtual devices like SIW or RXE don't set FW version because
> they don't have one, use that fact to rely on having empty
> fw_ver file to sense such virtual devices.
>
> Such change is needed to ensure that virtual devices which are
> attached to real hardware won't be renamed, because during
> device attachment, user already supplied desired name.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  kernel-boot/rdma_rename.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)

I have slightly better variant of this patch, but want to get feedback
on the concept before reposting.

Thanks

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Jason Gunthorpe]

On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Virtual devices like SIW or RXE don't set FW version because
> they don't have one, use that fact to rely on having empty
> fw_ver file to sense such virtual devices.

Have you checked that every physical device does set fw version?

Seems hacky

Jason

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Leon Romanovsky]

On Thu, Sep 26, 2019 at 12:34:31PM +0000, Jason Gunthorpe wrote:
> On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Virtual devices like SIW or RXE don't set FW version because
> > they don't have one, use that fact to rely on having empty
> > fw_ver file to sense such virtual devices.
>
> Have you checked that every physical device does set fw version?

I checked it with "grep", another solution which I had in mind is to
write "virtual" keyword as FW version.

Thanks

>
> Seems hacky
>
> Jason

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Jonathan Toppins]

On 09/26/2019 08:34 AM, Jason Gunthorpe wrote:
> On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
>> From: Leon Romanovsky <leonro@mellanox.com>
>>
>> Virtual devices like SIW or RXE don't set FW version because
>> they don't have one, use that fact to rely on having empty
>> fw_ver file to sense such virtual devices.
> 
> Have you checked that every physical device does set fw version?
> 
> Seems hacky

agreed, how are tuntap devices handled, is there a similar handling that
can be applied here?

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Leon Romanovsky]

On Thu, Sep 26, 2019 at 01:58:38PM -0400, Jonathan Toppins wrote:
> On 09/26/2019 08:34 AM, Jason Gunthorpe wrote:
> > On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> >> From: Leon Romanovsky <leonro@mellanox.com>
> >>
> >> Virtual devices like SIW or RXE don't set FW version because
> >> they don't have one, use that fact to rely on having empty
> >> fw_ver file to sense such virtual devices.
> >
> > Have you checked that every physical device does set fw version?
> >
> > Seems hacky
>
> agreed, how are tuntap devices handled, is there a similar handling that
> can be applied here?

Unfortunately, we can't do the same, RDMA doesn't have notion of stacked devices.

1.
TUN devices are initialized with ARPHRD_NONE type.
https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L1396

It causes for systemd-udev to skip their rename.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L781

2.
TAP devices are skipped due to the fact that iflink != ifindex on such devices.
https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L810

So, yes hacky, but the solution is tailored to RDMA subsystem where ALL
devices have FW and we can ensure that ALL future devices will report any
sort of string through fw_ver file.

Thanks

>

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Leon Romanovsky]

On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Virtual devices like SIW or RXE don't set FW version because
> they don't have one, use that fact to rely on having empty
> fw_ver file to sense such virtual devices.
>
> Such change is needed to ensure that virtual devices which are
> attached to real hardware won't be renamed, because during
> device attachment, user already supplied desired name.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  kernel-boot/rdma_rename.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)

PR https://github.com/linux-rdma/rdma-core/pull/588

Thanks

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Jason Gunthorpe]

On Sat, Sep 28, 2019 at 07:54:16PM +0300, Leon Romanovsky wrote:
> On Thu, Sep 26, 2019 at 01:58:38PM -0400, Jonathan Toppins wrote:
> > On 09/26/2019 08:34 AM, Jason Gunthorpe wrote:
> > > On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> > >> From: Leon Romanovsky <leonro@mellanox.com>
> > >>
> > >> Virtual devices like SIW or RXE don't set FW version because
> > >> they don't have one, use that fact to rely on having empty
> > >> fw_ver file to sense such virtual devices.
> > >
> > > Have you checked that every physical device does set fw version?
> > >
> > > Seems hacky
> >
> > agreed, how are tuntap devices handled, is there a similar handling that
> > can be applied here?
> 
> Unfortunately, we can't do the same, RDMA doesn't have notion of stacked devices.
> 
> 1.
> TUN devices are initialized with ARPHRD_NONE type.
> https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L1396
> 
> It causes for systemd-udev to skip their rename.
> https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L781
> 
> 2.
> TAP devices are skipped due to the fact that iflink != ifindex on such devices.
> https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L810
> 
> So, yes hacky, but the solution is tailored to RDMA subsystem where ALL
> devices have FW and we can ensure that ALL future devices will report any
> sort of string through fw_ver file.

It still seems really hacky, why not add some device flag or something
instead? Is this better because it works with old kernels?

Jason

Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual

[Leon Romanovsky]

On Thu, Oct 03, 2019 at 04:31:32PM +0000, Jason Gunthorpe wrote:
> On Sat, Sep 28, 2019 at 07:54:16PM +0300, Leon Romanovsky wrote:
> > On Thu, Sep 26, 2019 at 01:58:38PM -0400, Jonathan Toppins wrote:
> > > On 09/26/2019 08:34 AM, Jason Gunthorpe wrote:
> > > > On Thu, Sep 26, 2019 at 12:42:53PM +0300, Leon Romanovsky wrote:
> > > >> From: Leon Romanovsky <leonro@mellanox.com>
> > > >>
> > > >> Virtual devices like SIW or RXE don't set FW version because
> > > >> they don't have one, use that fact to rely on having empty
> > > >> fw_ver file to sense such virtual devices.
> > > >
> > > > Have you checked that every physical device does set fw version?
> > > >
> > > > Seems hacky
> > >
> > > agreed, how are tuntap devices handled, is there a similar handling that
> > > can be applied here?
> >
> > Unfortunately, we can't do the same, RDMA doesn't have notion of stacked devices.
> >
> > 1.
> > TUN devices are initialized with ARPHRD_NONE type.
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/tun.c#L1396
> >
> > It causes for systemd-udev to skip their rename.
> > https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L781
> >
> > 2.
> > TAP devices are skipped due to the fact that iflink != ifindex on such devices.
> > https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-net_id.c#L810
> >
> > So, yes hacky, but the solution is tailored to RDMA subsystem where ALL
> > devices have FW and we can ensure that ALL future devices will report any
> > sort of string through fw_ver file.
>
> It still seems really hacky, why not add some device flag or something
> instead? Is this better because it works with old kernels?

Yes, I'm trying to find a way to do such discovery without changing
kernel, because we have only two virtual devices and I don't expect
to see any new ones in forth coming future.

However, this patch is wrong because there are at least two drivers (hns
and EFA) didn't implement .get_dev_fw_str() and they will have empty
fw_ver.

 805 void ib_get_device_fw_str(struct ib_device *dev, char *str)
 806 {
 807         if (dev->ops.get_dev_fw_str)
 808                 dev->ops.get_dev_fw_str(dev, str);
 809         else
 810                 str[0] = '\0';
 811 }
 812 EXPORT_SYMBOL(ib_get_device_fw_str);

Special flag seems too much for me, what about writing special string to
fw_ver to indicate virtual device? For example "virtual".

Thanks

>
> Jason