Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
@ 2019-09-26  9:42 Leon Romanovsky
  2019-09-26 12:29 ` Leon Romanovsky
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-09-26  9:42 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, RDMA mailing list

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-26  9:42 [PATCH rdma-core] kernel-boot: Tighten check if device is virtual Leon Romanovsky
@ 2019-09-26 12:29 ` Leon Romanovsky
  2019-09-26 12:34 ` Jason Gunthorpe
  2019-10-02 11:47 ` Leon Romanovsky
  2 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-09-26 12:29 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: RDMA mailing list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-26  9:42 [PATCH rdma-core] kernel-boot: Tighten check if device is virtual Leon Romanovsky
  2019-09-26 12:29 ` Leon Romanovsky
@ 2019-09-26 12:34 ` Jason Gunthorpe
  2019-09-26 12:42   ` Leon Romanovsky
  2019-09-26 17:58   ` Jonathan Toppins
  2019-10-02 11:47 ` Leon Romanovsky
  2 siblings, 2 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2019-09-26 12:34 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-26 12:34 ` Jason Gunthorpe
@ 2019-09-26 12:42   ` Leon Romanovsky
  2019-09-26 17:58   ` Jonathan Toppins
  1 sibling, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-09-26 12:42 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Doug Ledford, RDMA mailing list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-26 12:34 ` Jason Gunthorpe
  2019-09-26 12:42   ` Leon Romanovsky
@ 2019-09-26 17:58   ` Jonathan Toppins
  2019-09-28 16:54     ` Leon Romanovsky
  1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Toppins @ 2019-09-26 17:58 UTC (permalink / raw)
  To: Jason Gunthorpe, Leon Romanovsky
  Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list

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?


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-26 17:58   ` Jonathan Toppins
@ 2019-09-28 16:54     ` Leon Romanovsky
  2019-10-03 16:31       ` Jason Gunthorpe
  0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2019-09-28 16:54 UTC (permalink / raw)
  To: Jonathan Toppins; +Cc: Jason Gunthorpe, Doug Ledford, RDMA mailing list

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

>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-26  9:42 [PATCH rdma-core] kernel-boot: Tighten check if device is virtual Leon Romanovsky
  2019-09-26 12:29 ` Leon Romanovsky
  2019-09-26 12:34 ` Jason Gunthorpe
@ 2019-10-02 11:47 ` Leon Romanovsky
  2 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-10-02 11:47 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: RDMA mailing list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-09-28 16:54     ` Leon Romanovsky
@ 2019-10-03 16:31       ` Jason Gunthorpe
  2019-10-05  6:12         ` Leon Romanovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2019-10-03 16:31 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Jonathan Toppins, Doug Ledford, RDMA mailing list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-core] kernel-boot: Tighten check if device is virtual
  2019-10-03 16:31       ` Jason Gunthorpe
@ 2019-10-05  6:12         ` Leon Romanovsky
  0 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-10-05  6:12 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Jonathan Toppins, Doug Ledford, RDMA mailing list

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26  9:42 [PATCH rdma-core] kernel-boot: Tighten check if device is virtual Leon Romanovsky
2019-09-26 12:29 ` Leon Romanovsky
2019-09-26 12:34 ` Jason Gunthorpe
2019-09-26 12:42   ` Leon Romanovsky
2019-09-26 17:58   ` Jonathan Toppins
2019-09-28 16:54     ` Leon Romanovsky
2019-10-03 16:31       ` Jason Gunthorpe
2019-10-05  6:12         ` Leon Romanovsky
2019-10-02 11:47 ` Leon Romanovsky

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org linux-rdma@archiver.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/ public-inbox