[PATCH 0/3] net, IB/ipoib: Use dev_port to disambiguate

[PATCH 0/3] net, IB/ipoib: Use dev_port to disambiguate

[Arseny Maslennikov]

Pre-3.15 userspace had trouble distinguishing different ports of a NIC
on a single PCI bus/device/function. To solve this, a sysfs field `dev_port'
was introduced quite a while ago (commit v3.14-rc3-739-g3f85944fe207), and
some relevant device drivers were fixed to use it, but not in case of IPoIB.

The convention for some reason never got documented in the kernel, but
was immediately adopted by userspace (notably udev[1][2], biosdevname[3])

3/3 documents the sysfs field — that's why I'm CC-ing netdev.

This series was tested on current LTS and 4.18.

[1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
[2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
[3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38

Arseny Maslennikov (3):
  IB/ipoib: Use dev_port to expose network interface port numbers
  IB/ipoib: Stop using dev_id to expose port numbers
  Documentation/ABI: document /sys/class/net/*/dev_port

 Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.18.0

[PATCH 1/3] IB/ipoib: Use dev_port to expose network interface port numbers

[Arseny Maslennikov]

Some InfiniBand network devices have multiple ports on the same PCI
function. This initializes the `dev_port' sysfs field of those
network interfaces with their port number.

The use of `dev_id' was considered correct until Linux 3.15,
when another field, `dev_port', was defined for this particular
purpose and `dev_id' was reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.

Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 26cde95bc0f3..6eb0594fffec 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2253,6 +2253,7 @@ static struct net_device *ipoib_add_port(const char *format,
 
 	SET_NETDEV_DEV(priv->dev, hca->dev.parent);
 	priv->dev->dev_id = port - 1;
+	priv->dev->dev_port = port - 1;
 
 	result = ib_query_port(hca, port, &attr);
 	if (result) {
-- 
2.18.0

[PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers

[Arseny Maslennikov]

Some InfiniBand network devices have multiple ports on the same PCI
function. Prior to this the kernel erroneously used the `dev_id' sysfs
field of those network interfaces to convey the port number to userspace.

`dev_id' is currently reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.

Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.

I would be OK with this commit not being backported to stable, since
it might break admin-supplied udev rules and the likes.

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 6eb0594fffec..f64535038147 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
 	}
 
 	SET_NETDEV_DEV(priv->dev, hca->dev.parent);
-	priv->dev->dev_id = port - 1;
 	priv->dev->dev_port = port - 1;
 
 	result = ib_query_port(hca, port, &attr);
-- 
2.18.0

[PATCH 3/3] Documentation/ABI: document /sys/class/net/*/dev_port

[Arseny Maslennikov]

The sysfs field was introduced 4 years ago along with fixes to various
drivers that erroneously used `dev_id' for that purpose, but it was not
properly documented anywhere.
See commit v3.14-rc3-739-g3f85944fe207.

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net
index 2f1788111cd9..1593d8997ade 100644
--- a/Documentation/ABI/testing/sysfs-class-net
+++ b/Documentation/ABI/testing/sysfs-class-net
@@ -91,6 +91,16 @@ Description:
 		stacked (e.g: VLAN interfaces) but still have the same MAC
 		address as their parent device.
 
+What:		/sys/class/net/<iface>/dev_port
+Date:		February 2014
+KernelVersion:	3.15
+Contact:	netdev@vger.kernel.org
+Description:
+		Indicates the port number of this network device, formatted
+		as a decimal value. Some NICs have multiple independent ports
+		on the same PCI bus, device and function. This field allows
+		userspace to distinguish the respective interfaces.
+
 What:		/sys/class/net/<iface>/dormant
 Date:		March 2006
 KernelVersion:	2.6.17
-- 
2.18.0

Re: [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers

[Yuval Shaia]

On Mon, Aug 13, 2018 at 02:42:23PM +0300, Arseny Maslennikov wrote:
> Some InfiniBand network devices have multiple ports on the same PCI
> function. Prior to this the kernel erroneously used the `dev_id' sysfs
> field of those network interfaces to convey the port number to userspace.
> 
> `dev_id' is currently reserved for distinguishing stacked ifaces
> (e.g: VLANs) with the same hardware address as their parent device.
> 
> Similar fixes to net/mlx4_en and many other drivers, which started
> exporting this information through `dev_id' before 3.15, were accepted
> into the kernel 4 years ago.
> See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
> `net/mlx4_en: Expose port number through sysfs'.
> 
> I would be OK with this commit not being backported to stable, since
> it might break admin-supplied udev rules and the likes.
> 
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 6eb0594fffec..f64535038147 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
>  	}
>  
>  	SET_NETDEV_DEV(priv->dev, hca->dev.parent);
> -	priv->dev->dev_id = port - 1;

Correct me if i'm wrong here but besides some changes in commit message
looks like patch 1/3 is the same as 2/3, isn't it?

Yuval

>  	priv->dev->dev_port = port - 1;
>  
>  	result = ib_query_port(hca, port, &attr);
> -- 
> 2.18.0
> 

Re: [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

On Mon, Aug 13, 2018 at 03:40:20PM +0300, Yuval Shaia wrote:
> On Mon, Aug 13, 2018 at 02:42:23PM +0300, Arseny Maslennikov wrote:
> > Some InfiniBand network devices have multiple ports on the same PCI
> > function. Prior to this the kernel erroneously used the `dev_id' sysfs
> > field of those network interfaces to convey the port number to userspace.
> > 
> > `dev_id' is currently reserved for distinguishing stacked ifaces
> > (e.g: VLANs) with the same hardware address as their parent device.
> > 
> > Similar fixes to net/mlx4_en and many other drivers, which started
> > exporting this information through `dev_id' before 3.15, were accepted
> > into the kernel 4 years ago.
> > See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
> > `net/mlx4_en: Expose port number through sysfs'.
> > 
> > I would be OK with this commit not being backported to stable, since
> > it might break admin-supplied udev rules and the likes.
> > 
> > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > ---
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 6eb0594fffec..f64535038147 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -2252,7 +2252,6 @@ static struct net_device *ipoib_add_port(const char *format,
> >  	}
> >  
> >  	SET_NETDEV_DEV(priv->dev, hca->dev.parent);
> > -	priv->dev->dev_id = port - 1;
> 
> Correct me if i'm wrong here but besides some changes in commit message
> looks like patch 1/3 is the same as 2/3, isn't it?
> 
> Yuval
> 

1/3 has an extra line, 2/3 removes a different line.

(a) If you apply both 1/3 and 2/3, the port number can be seen at
/sys/class/net/*/dev_port and not at .../dev_id.

(b) If you apply only 1/3, the port number can be seen at _both_
.../dev_port and .../dev_id (to preserve backward compatibility with
e.g. existing udev rules that rely on "ATTR{dev_id}")

By splitting those up we have both options (a) and (b) available,
so the maintainers are free to decide which one is wiser.

> >  	priv->dev->dev_port = port - 1;
> >  
> >  	result = ib_query_port(hca, port, &attr);
> > -- 
> > 2.18.0
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] net, IB/ipoib: Use dev_port to disambiguate

[Jason Gunthorpe]

On Mon, Aug 13, 2018 at 02:42:21PM +0300, Arseny Maslennikov wrote:
> Pre-3.15 userspace had trouble distinguishing different ports of a NIC
> on a single PCI bus/device/function. To solve this, a sysfs field `dev_port'
> was introduced quite a while ago (commit v3.14-rc3-739-g3f85944fe207), and
> some relevant device drivers were fixed to use it, but not in case of IPoIB.
> 
> The convention for some reason never got documented in the kernel, but
> was immediately adopted by userspace (notably udev[1][2], biosdevname[3])
> 
> 3/3 documents the sysfs field — that's why I'm CC-ing netdev.
> 
> This series was tested on current LTS and 4.18.
> 
> [1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
> [2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
> [3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38
> 
> Arseny Maslennikov (3):
>   IB/ipoib: Use dev_port to expose network interface port numbers
>   IB/ipoib: Stop using dev_id to expose port numbers
>   Documentation/ABI: document /sys/class/net/*/dev_port
> 
>  Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
>  drivers/infiniband/ulp/ipoib/ipoib_main.c |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)

This series doesn't apply to rdma for-next, and it is the merge window
now.

Can you resubmit this aginst 4.19-rc1 in two weeks? Thanks

Jason

Re: [PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers

[Sergei Shtylyov]

Hello!

On 8/29/2018 12:01 AM, Arseny Maslennikov wrote:

> Some InfiniBand network devices have multiple ports on the same PCI
> function. Prior to this the kernel erroneously used the `dev_id' sysfs
> field of those network interfaces to convey the port number to userspace.
> 
> `dev_id' is currently reserved for distinguishing stacked ifaces
> (e.g: VLANs) with the same hardware address as their parent device.
> 
> Similar fixes to net/mlx4_en and many other drivers, which started
> exporting this information through `dev_id' before 3.15, were accepted
> into the kernel 4 years ago.
> See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
> `net/mlx4_en: Expose port number through sysfs'.

    See commit 76a066f2a2a0 ("net/mlx4_en: Expose port number through sysfs").
> This commit is separated from the previous one since we may wish to
> preserve backwards compatibility with userspace being already dependent
> on `dev_id' being different.
> 
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
[...]

MBR, Sergei

[PATCH 2/3] IB/ipoib: Stop using dev_id to expose port numbers

[Arseny Maslennikov]

Some InfiniBand network devices have multiple ports on the same PCI
function. Prior to this the kernel erroneously used the `dev_id' sysfs
field of those network interfaces to convey the port number to userspace.

`dev_id' is currently reserved for distinguishing stacked ifaces
(e.g: VLANs) with the same hardware address as their parent device.

Similar fixes to net/mlx4_en and many other drivers, which started
exporting this information through `dev_id' before 3.15, were accepted
into the kernel 4 years ago.
See 76a066f2a2a0268b565459c417b59724b5a3197b, commit message:
`net/mlx4_en: Expose port number through sysfs'.

This commit is separated from the previous one since we may wish to
preserve backwards compatibility with userspace being already dependent
on `dev_id' being different.

Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index fcd69273de91..ba16a63ee303 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1880,7 +1880,6 @@ static int ipoib_parent_init(struct net_device *ndev)
 	       sizeof(union ib_gid));
 
 	SET_NETDEV_DEV(priv->dev, priv->ca->dev.parent);
-	priv->dev->dev_id = priv->port - 1;
 	priv->dev->dev_port = priv->port - 1;
 
 	return 0;
-- 
2.18.0