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

[Leon Romanovsky <leon@kernel.org>]

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