[PATCH v3 0/3] IB/ipoib: Use dev_port to disambiguate port numbers

[PATCH v3 0/3] IB/ipoib: Use dev_port to disambiguate port numbers

[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])

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

This series was tested on and applies to 4.19-rc2.

[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

v1->v2: replace a line instead of inserting and then removing.
v2->v3: restore both attributes, output a notice of deprecation to kmsg.

Arseny Maslennikov (3):
  Documentation/ABI: document /sys/class/net/*/dev_port
  IB/ipoib: Use dev_port to expose network interface port numbers
  IB/ipoib: Log sysfs 'dev_id' accesses from userspace

 Documentation/ABI/testing/sysfs-class-net | 18 ++++++++++
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 40 +++++++++++++++++++++++
 2 files changed, 58 insertions(+)

-- 
2.19.0.rc1

[PATCH v3 1/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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-net b/Documentation/ABI/testing/sysfs-class-net
index 2f1788111cd9..ec2232f6a949 100644
--- a/Documentation/ABI/testing/sysfs-class-net
+++ b/Documentation/ABI/testing/sysfs-class-net
@@ -91,6 +91,24 @@ 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 attribute allows
+		userspace to distinguish the respective interfaces.
+
+		Note: some device drivers started to use 'dev_id' for this
+		purpose since long before 3.15 and have not adopted the new
+		attribute ever since. To query the port number, some tools look
+		exclusively at 'dev_port', while others only consult 'dev_id'.
+		If a network device has multiple client adapter ports as
+		described in the previous paragraph and does not set this
+		attribute to its port number, it's a kernel bug.
+
 What:		/sys/class/net/<iface>/dormant
 Date:		March 2006
 KernelVersion:	2.6.17
-- 
2.19.0.rc1

[PATCH v3 2/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.

Prior to this the kernel erroneously used the `dev_id' sysfs
field of those network interfaces to convey the port number to userspace.

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 76a066f2a2a0 (`net/mlx4_en: Expose port number through sysfs').

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

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index e3d28f9ad9c0..30f840f874b3 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1880,6 +1880,8 @@ 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_port = priv->port - 1;
+	/* Let's set this one too for backwards compatibility. */
 	priv->dev->dev_id = priv->port - 1;
 
 	return 0;
-- 
2.19.0.rc1

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

[Leon Romanovsky]

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

On Mon, Sep 03, 2018 at 07:13:15PM +0300, Arseny Maslennikov wrote:
> 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.
>
> Prior to this the kernel erroneously used the `dev_id' sysfs
> field of those network interfaces to convey the port number to userspace.
>
> 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 76a066f2a2a0 (`net/mlx4_en: Expose port number through sysfs').
>
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 ++
>  1 file changed, 2 insertions(+)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

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

[PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Arseny Maslennikov]

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

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 30f840f874b3..7386e5bde3d3 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
 	return device_create_file(&dev->dev, &dev_attr_pkey);
 }
 
+/*
+ * We erroneously exposed the iface's port number in the dev_id
+ * sysfs field long after dev_port was introduced for that purpose[1],
+ * and we need to stop everyone from relying on that.
+ * Let's overload the shower routine for the dev_id file here
+ * to gently bring the issue up.
+ *
+ * [1] https://www.spinics.net/lists/netdev/msg272123.html
+ */
+static ssize_t dev_id_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct net_device *ndev = to_net_dev(dev);
+	ssize_t ret = -EINVAL;
+
+	if (ndev->dev_id == ndev->dev_port) {
+		netdev_info_once(ndev,
+			"\"%s\" wants to know my dev_id. "
+			"Should it look at dev_port instead?\n",
+			current->comm);
+		netdev_info_once(ndev,
+			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
+	}
+
+	ret = sprintf(buf, "%#x\n", ndev->dev_id);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(dev_id);
+
+int ipoib_intercept_dev_id_attr(struct net_device *dev)
+{
+	device_remove_file(&dev->dev, &dev_attr_dev_id);
+	return device_create_file(&dev->dev, &dev_attr_dev_id);
+}
+
 static struct net_device *ipoib_add_port(const char *format,
 					 struct ib_device *hca, u8 port)
 {
@@ -2427,6 +2463,8 @@ static struct net_device *ipoib_add_port(const char *format,
 	 */
 	ndev->priv_destructor = ipoib_intf_free;
 
+	if (ipoib_intercept_dev_id_attr(ndev))
+		goto sysfs_failed;
 	if (ipoib_cm_add_mode_attr(ndev))
 		goto sysfs_failed;
 	if (ipoib_add_pkey_attr(ndev))
-- 
2.19.0.rc1

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Leon Romanovsky]

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

On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:
> Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 38 +++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 30f840f874b3..7386e5bde3d3 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
>  	return device_create_file(&dev->dev, &dev_attr_pkey);
>  }
>
> +/*
> + * We erroneously exposed the iface's port number in the dev_id
> + * sysfs field long after dev_port was introduced for that purpose[1],
> + * and we need to stop everyone from relying on that.
> + * Let's overload the shower routine for the dev_id file here
> + * to gently bring the issue up.
> + *
> + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> + */
> +static ssize_t dev_id_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	ssize_t ret = -EINVAL;
> +
> +	if (ndev->dev_id == ndev->dev_port) {
> +		netdev_info_once(ndev,
> +			"\"%s\" wants to know my dev_id. "
> +			"Should it look at dev_port instead?\n",
> +			current->comm);
> +		netdev_info_once(ndev,
> +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> +	}
> +
> +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(dev_id);
> +

I don't see this field among exposed by IPoIB, why should we expose it now?

> +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> +{
> +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> +	return device_create_file(&dev->dev, &dev_attr_dev_id);

Why isn't enough to rely on netdev code?

> +}
> +
>  static struct net_device *ipoib_add_port(const char *format,
>  					 struct ib_device *hca, u8 port)
>  {
> @@ -2427,6 +2463,8 @@ static struct net_device *ipoib_add_port(const char *format,
>  	 */
>  	ndev->priv_destructor = ipoib_intf_free;
>
> +	if (ipoib_intercept_dev_id_attr(ndev))
> +		goto sysfs_failed;
>  	if (ipoib_cm_add_mode_attr(ndev))
>  		goto sysfs_failed;
>  	if (ipoib_add_pkey_attr(ndev))
> --
> 2.19.0.rc1
>

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

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Arseny Maslennikov]

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

On Wed, Sep 05, 2018 at 04:50:35PM +0300, Leon Romanovsky wrote:
> On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:
> > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > ---
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 38 +++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 30f840f874b3..7386e5bde3d3 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> >  }
> >
> > +/*
> > + * We erroneously exposed the iface's port number in the dev_id
> > + * sysfs field long after dev_port was introduced for that purpose[1],
> > + * and we need to stop everyone from relying on that.
> > + * Let's overload the shower routine for the dev_id file here
> > + * to gently bring the issue up.
> > + *
> > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > + */
> > +static ssize_t dev_id_show(struct device *dev,
> > +			   struct device_attribute *attr, char *buf)
> > +{
> > +	struct net_device *ndev = to_net_dev(dev);
> > +	ssize_t ret = -EINVAL;
> > +
> > +	if (ndev->dev_id == ndev->dev_port) {
> > +		netdev_info_once(ndev,
> > +			"\"%s\" wants to know my dev_id. "
> > +			"Should it look at dev_port instead?\n",
> > +			current->comm);
> > +		netdev_info_once(ndev,
> > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > +	}
> > +
> > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RO(dev_id);
> > +
> 
> I don't see this field among exposed by IPoIB, why should we expose it now?
> 

To deviate from standard netdev behaviour, which only prints the
field out. Doug wanted this to also print a deprecation message, and
netdev (obviously) does not do that. See below.

> > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > +{
> > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> 
> Why isn't enough to rely on netdev code?
> 

Netdev code relies on macros around a *static* function 'netdev_show',
which is defined in net/core/net-sysfs.c; it is not listed in any header
files, and the macros aren't as well. This all leads me to believe it
was not really meant to be used from outside net/core/net-sysfs.

The only way we could use any netdev code here is to set up our own
handler (again), printk() a message, then call netdev_show — but we have
no access to it.

Of course, it also may be that I'm terribly missing a clue.

> > +}
> > +
> >  static struct net_device *ipoib_add_port(const char *format,
> >  					 struct ib_device *hca, u8 port)
> >  {
> > @@ -2427,6 +2463,8 @@ static struct net_device *ipoib_add_port(const char *format,
> >  	 */
> >  	ndev->priv_destructor = ipoib_intf_free;
> >
> > +	if (ipoib_intercept_dev_id_attr(ndev))
> > +		goto sysfs_failed;
> >  	if (ipoib_cm_add_mode_attr(ndev))
> >  		goto sysfs_failed;
> >  	if (ipoib_add_pkey_attr(ndev))
> > --
> > 2.19.0.rc1
> >



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

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Leon Romanovsky]

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

On Thu, Sep 06, 2018 at 10:04:33AM +0300, Arseny Maslennikov wrote:
> On Wed, Sep 05, 2018 at 04:50:35PM +0300, Leon Romanovsky wrote:
> > On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:
> > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > > ---
> > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 38 +++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > index 30f840f874b3..7386e5bde3d3 100644
> > > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > @@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > >  }
> > >
> > > +/*
> > > + * We erroneously exposed the iface's port number in the dev_id
> > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > + * and we need to stop everyone from relying on that.
> > > + * Let's overload the shower routine for the dev_id file here
> > > + * to gently bring the issue up.
> > > + *
> > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > + */
> > > +static ssize_t dev_id_show(struct device *dev,
> > > +			   struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct net_device *ndev = to_net_dev(dev);
> > > +	ssize_t ret = -EINVAL;
> > > +
> > > +	if (ndev->dev_id == ndev->dev_port) {
> > > +		netdev_info_once(ndev,
> > > +			"\"%s\" wants to know my dev_id. "
> > > +			"Should it look at dev_port instead?\n",
> > > +			current->comm);
> > > +		netdev_info_once(ndev,
> > > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > > +	}
> > > +
> > > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > > +
> > > +	return ret;
> > > +}
> > > +static DEVICE_ATTR_RO(dev_id);
> > > +
> >
> > I don't see this field among exposed by IPoIB, why should we expose it now?
> >
>
> To deviate from standard netdev behaviour, which only prints the
> field out. Doug wanted this to also print a deprecation message, and
> netdev (obviously) does not do that. See below.
>
> > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > +{
> > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> >
> > Why isn't enough to rely on netdev code?
> >
>
> Netdev code relies on macros around a *static* function 'netdev_show',
> which is defined in net/core/net-sysfs.c; it is not listed in any header
> files, and the macros aren't as well. This all leads me to believe it
> was not really meant to be used from outside net/core/net-sysfs.
>
> The only way we could use any netdev code here is to set up our own
> handler (again), printk() a message, then call netdev_show — but we have
> no access to it.
>
> Of course, it also may be that I'm terribly missing a clue.

Thanks,

IMHO, the end result of adequate Doug's request is a little bit too much.
I don't think that it justifies such remove->create construction.

Personal opinion.

>
> > > +}
> > > +
> > >  static struct net_device *ipoib_add_port(const char *format,
> > >  					 struct ib_device *hca, u8 port)
> > >  {
> > > @@ -2427,6 +2463,8 @@ static struct net_device *ipoib_add_port(const char *format,
> > >  	 */
> > >  	ndev->priv_destructor = ipoib_intf_free;
> > >
> > > +	if (ipoib_intercept_dev_id_attr(ndev))
> > > +		goto sysfs_failed;
> > >  	if (ipoib_cm_add_mode_attr(ndev))
> > >  		goto sysfs_failed;
> > >  	if (ipoib_add_pkey_attr(ndev))
> > > --
> > > 2.19.0.rc1
> > >
>
>



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

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Doug Ledford]

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

On Thu, 2018-09-06 at 16:03 +0300, Leon Romanovsky wrote:
> On Thu, Sep 06, 2018 at 10:04:33AM +0300, Arseny Maslennikov wrote:
> > On Wed, Sep 05, 2018 at 04:50:35PM +0300, Leon Romanovsky wrote:
> > > On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:
> > > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > > > ---
> > > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 38 +++++++++++++++++++++++
> > > >  1 file changed, 38 insertions(+)
> > > > 
> > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > index 30f840f874b3..7386e5bde3d3 100644
> > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > @@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > > >  }
> > > > 
> > > > +/*
> > > > + * We erroneously exposed the iface's port number in the dev_id
> > > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > > + * and we need to stop everyone from relying on that.
> > > > + * Let's overload the shower routine for the dev_id file here
> > > > + * to gently bring the issue up.
> > > > + *
> > > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > > + */
> > > > +static ssize_t dev_id_show(struct device *dev,
> > > > +			   struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct net_device *ndev = to_net_dev(dev);
> > > > +	ssize_t ret = -EINVAL;
> > > > +
> > > > +	if (ndev->dev_id == ndev->dev_port) {
> > > > +		netdev_info_once(ndev,
> > > > +			"\"%s\" wants to know my dev_id. "
> > > > +			"Should it look at dev_port instead?\n",
> > > > +			current->comm);
> > > > +		netdev_info_once(ndev,
> > > > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > > > +	}
> > > > +
> > > > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +static DEVICE_ATTR_RO(dev_id);
> > > > +
> > > 
> > > I don't see this field among exposed by IPoIB, why should we expose it now?
> > > 
> > 
> > To deviate from standard netdev behaviour, which only prints the
> > field out. Doug wanted this to also print a deprecation message, and
> > netdev (obviously) does not do that. See below.
> > 
> > > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > > +{
> > > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > > 
> > > Why isn't enough to rely on netdev code?
> > > 
> > 
> > Netdev code relies on macros around a *static* function 'netdev_show',
> > which is defined in net/core/net-sysfs.c; it is not listed in any header
> > files, and the macros aren't as well. This all leads me to believe it
> > was not really meant to be used from outside net/core/net-sysfs.
> > 
> > The only way we could use any netdev code here is to set up our own
> > handler (again), printk() a message, then call netdev_show — but we have
> > no access to it.
> > 
> > Of course, it also may be that I'm terribly missing a clue.
> 
> Thanks,
> 
> IMHO, the end result of adequate Doug's request is a little bit too much.
> I don't think that it justifies such remove->create construction.
> 
> Personal opinion.

I agree with you that the end result is kinda bulky, *but*, we will need
to know if there are things using the old dev_id before we can remove
it.  In particular, I'm concerned the IPoIB handling code of
NetworkManager uses it.  It's worth the cost I think.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Leon Romanovsky]

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

On Fri, Sep 07, 2018 at 01:14:37PM -0400, Doug Ledford wrote:
> On Thu, 2018-09-06 at 16:03 +0300, Leon Romanovsky wrote:
> > On Thu, Sep 06, 2018 at 10:04:33AM +0300, Arseny Maslennikov wrote:
> > > On Wed, Sep 05, 2018 at 04:50:35PM +0300, Leon Romanovsky wrote:
> > > > On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:
> > > > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > > > > ---
> > > > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 38 +++++++++++++++++++++++
> > > > >  1 file changed, 38 insertions(+)
> > > > >
> > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > > index 30f840f874b3..7386e5bde3d3 100644
> > > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > > @@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > > > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * We erroneously exposed the iface's port number in the dev_id
> > > > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > > > + * and we need to stop everyone from relying on that.
> > > > > + * Let's overload the shower routine for the dev_id file here
> > > > > + * to gently bring the issue up.
> > > > > + *
> > > > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > > > + */
> > > > > +static ssize_t dev_id_show(struct device *dev,
> > > > > +			   struct device_attribute *attr, char *buf)
> > > > > +{
> > > > > +	struct net_device *ndev = to_net_dev(dev);
> > > > > +	ssize_t ret = -EINVAL;
> > > > > +
> > > > > +	if (ndev->dev_id == ndev->dev_port) {
> > > > > +		netdev_info_once(ndev,
> > > > > +			"\"%s\" wants to know my dev_id. "
> > > > > +			"Should it look at dev_port instead?\n",
> > > > > +			current->comm);
> > > > > +		netdev_info_once(ndev,
> > > > > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > > > > +	}
> > > > > +
> > > > > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +static DEVICE_ATTR_RO(dev_id);
> > > > > +
> > > >
> > > > I don't see this field among exposed by IPoIB, why should we expose it now?
> > > >
> > >
> > > To deviate from standard netdev behaviour, which only prints the
> > > field out. Doug wanted this to also print a deprecation message, and
> > > netdev (obviously) does not do that. See below.
> > >
> > > > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > > > +{
> > > > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > > >
> > > > Why isn't enough to rely on netdev code?
> > > >
> > >
> > > Netdev code relies on macros around a *static* function 'netdev_show',
> > > which is defined in net/core/net-sysfs.c; it is not listed in any header
> > > files, and the macros aren't as well. This all leads me to believe it
> > > was not really meant to be used from outside net/core/net-sysfs.
> > >
> > > The only way we could use any netdev code here is to set up our own
> > > handler (again), printk() a message, then call netdev_show — but we have
> > > no access to it.
> > >
> > > Of course, it also may be that I'm terribly missing a clue.
> >
> > Thanks,
> >
> > IMHO, the end result of adequate Doug's request is a little bit too much.
> > I don't think that it justifies such remove->create construction.
> >
> > Personal opinion.
>
> I agree with you that the end result is kinda bulky, *but*, we will need
> to know if there are things using the old dev_id before we can remove
> it.  In particular, I'm concerned the IPoIB handling code of
> NetworkManager uses it.  It's worth the cost I think.

I did my checks now and saw that ibdev2netdev uses the value provided
from this dev_id, so every invocation of that script will generate the
warning.

See this line in Parav's repo
https://github.com/Mellanox/container_scripts/blob/master/ibdev2netdev#L112

Thanks

>
> --
> Doug Ledford <dledford@redhat.com>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



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

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Doug Ledford]

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

On Fri, 2018-09-07 at 20:28 +0300, Leon Romanovsky wrote:
> On Fri, Sep 07, 2018 at 01:14:37PM -0400, Doug Ledford wrote:
> > On Thu, 2018-09-06 at 16:03 +0300, Leon Romanovsky wrote:
> > > On Thu, Sep 06, 2018 at 10:04:33AM +0300, Arseny Maslennikov wrote:
> > > > On Wed, Sep 05, 2018 at 04:50:35PM +0300, Leon Romanovsky wrote:
> > > > > On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:
> > > > > > Signed-off-by: Arseny Maslennikov <ar@cs.msu.ru>
> > > > > > ---
> > > > > >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 38 +++++++++++++++++++++++
> > > > > >  1 file changed, 38 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > > > index 30f840f874b3..7386e5bde3d3 100644
> > > > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > > > > > @@ -2386,6 +2386,42 @@ int ipoib_add_pkey_attr(struct net_device *dev)
> > > > > >  	return device_create_file(&dev->dev, &dev_attr_pkey);
> > > > > >  }
> > > > > > 
> > > > > > +/*
> > > > > > + * We erroneously exposed the iface's port number in the dev_id
> > > > > > + * sysfs field long after dev_port was introduced for that purpose[1],
> > > > > > + * and we need to stop everyone from relying on that.
> > > > > > + * Let's overload the shower routine for the dev_id file here
> > > > > > + * to gently bring the issue up.
> > > > > > + *
> > > > > > + * [1] https://www.spinics.net/lists/netdev/msg272123.html
> > > > > > + */
> > > > > > +static ssize_t dev_id_show(struct device *dev,
> > > > > > +			   struct device_attribute *attr, char *buf)
> > > > > > +{
> > > > > > +	struct net_device *ndev = to_net_dev(dev);
> > > > > > +	ssize_t ret = -EINVAL;
> > > > > > +
> > > > > > +	if (ndev->dev_id == ndev->dev_port) {
> > > > > > +		netdev_info_once(ndev,
> > > > > > +			"\"%s\" wants to know my dev_id. "
> > > > > > +			"Should it look at dev_port instead?\n",
> > > > > > +			current->comm);
> > > > > > +		netdev_info_once(ndev,
> > > > > > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +static DEVICE_ATTR_RO(dev_id);
> > > > > > +
> > > > > 
> > > > > I don't see this field among exposed by IPoIB, why should we expose it now?
> > > > > 
> > > > 
> > > > To deviate from standard netdev behaviour, which only prints the
> > > > field out. Doug wanted this to also print a deprecation message, and
> > > > netdev (obviously) does not do that. See below.
> > > > 
> > > > > > +int ipoib_intercept_dev_id_attr(struct net_device *dev)
> > > > > > +{
> > > > > > +	device_remove_file(&dev->dev, &dev_attr_dev_id);
> > > > > > +	return device_create_file(&dev->dev, &dev_attr_dev_id);
> > > > > 
> > > > > Why isn't enough to rely on netdev code?
> > > > > 
> > > > 
> > > > Netdev code relies on macros around a *static* function 'netdev_show',
> > > > which is defined in net/core/net-sysfs.c; it is not listed in any header
> > > > files, and the macros aren't as well. This all leads me to believe it
> > > > was not really meant to be used from outside net/core/net-sysfs.
> > > > 
> > > > The only way we could use any netdev code here is to set up our own
> > > > handler (again), printk() a message, then call netdev_show — but we have
> > > > no access to it.
> > > > 
> > > > Of course, it also may be that I'm terribly missing a clue.
> > > 
> > > Thanks,
> > > 
> > > IMHO, the end result of adequate Doug's request is a little bit too much.
> > > I don't think that it justifies such remove->create construction.
> > > 
> > > Personal opinion.
> > 
> > I agree with you that the end result is kinda bulky, *but*, we will need
> > to know if there are things using the old dev_id before we can remove
> > it.  In particular, I'm concerned the IPoIB handling code of
> > NetworkManager uses it.  It's worth the cost I think.
> 
> I did my checks now and saw that ibdev2netdev uses the value provided
> from this dev_id, so every invocation of that script will generate the
> warning.
> 
> See this line in Parav's repo
> https://github.com/Mellanox/container_scripts/blob/master/ibdev2netdev#L112

I'm pretty sure libvirt + qemu will trigger it too.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Stephen Hemminger]

On Mon,  3 Sep 2018 19:13:16 +0300
Arseny Maslennikov <ar@cs.msu.ru> wrote:

> +	if (ndev->dev_id == ndev->dev_port) {
> +		netdev_info_once(ndev,
> +			"\"%s\" wants to know my dev_id. "
> +			"Should it look at dev_port instead?\n",
> +			current->comm);
> +		netdev_info_once(ndev,
> +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> +	}

Single line message is sufficient.
Also don't break strings in messages.

> +	}
> +
> +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> +
> +	return ret;

Why not?
	return sprintf...

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Arseny Maslennikov]

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

On Wed, Sep 05, 2018 at 04:47:27PM +0100, Stephen Hemminger wrote:
> On Mon,  3 Sep 2018 19:13:16 +0300
> Arseny Maslennikov <ar@cs.msu.ru> wrote:
> 
> > +	if (ndev->dev_id == ndev->dev_port) {
> > +		netdev_info_once(ndev,
> > +			"\"%s\" wants to know my dev_id. "
> > +			"Should it look at dev_port instead?\n",
> > +			current->comm);
> > +		netdev_info_once(ndev,
> > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > +	}
> 
> Single line message is sufficient.
> Also don't break strings in messages.
> 

OK, will fix in v4.


(Sorry if the following is too off-topic here)
Multi-line messages in separate printk calls can be racy, I get that.
But I'd like to hear some reasoning behind the style decision to not
break a long string into many string literals. (I'll most certainly not
be alone in this, Documentation/process/ does not mention reasons, only
the requirements themselves)

The only drawback I currently see is that breaking a long message into
multiple string literals makes it impossible to git grep the kernel tree
for the whole message text.
However, splitting a long line this way allows us to nicely wrap the
code at 80 columns, which is a readability boon.

Are there any other reasons to avoid that? Except maybe matters of taste. :)

> > +	}
> > +
> > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > +
> > +	return ret;
> 
> Why not?
> 	return sprintf...

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

Re: [PATCH v3 3/3] IB/ipoib: Log sysfs 'dev_id' accesses from userspace

[Leon Romanovsky]

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

On Thu, Sep 06, 2018 at 10:26:56AM +0300, Arseny Maslennikov wrote:
> On Wed, Sep 05, 2018 at 04:47:27PM +0100, Stephen Hemminger wrote:
> > On Mon,  3 Sep 2018 19:13:16 +0300
> > Arseny Maslennikov <ar@cs.msu.ru> wrote:
> >
> > > +	if (ndev->dev_id == ndev->dev_port) {
> > > +		netdev_info_once(ndev,
> > > +			"\"%s\" wants to know my dev_id. "
> > > +			"Should it look at dev_port instead?\n",
> > > +			current->comm);
> > > +		netdev_info_once(ndev,
> > > +			"See Documentation/ABI/testing/sysfs-class-net for more info.\n");
> > > +	}
> >
> > Single line message is sufficient.
> > Also don't break strings in messages.
> >
>
> OK, will fix in v4.
>
>
> (Sorry if the following is too off-topic here)
> Multi-line messages in separate printk calls can be racy, I get that.
> But I'd like to hear some reasoning behind the style decision to not
> break a long string into many string literals. (I'll most certainly not
> be alone in this, Documentation/process/ does not mention reasons, only
> the requirements themselves)
>
> The only drawback I currently see is that breaking a long message into
> multiple string literals makes it impossible to git grep the kernel tree
> for the whole message text.
> However, splitting a long line this way allows us to nicely wrap the
> code at 80 columns, which is a readability boon.
>
> Are there any other reasons to avoid that? Except maybe matters of taste. :)

AFAIK "grep" is the reason.

>
> > > +	}
> > > +
> > > +	ret = sprintf(buf, "%#x\n", ndev->dev_id);
> > > +
> > > +	return ret;
> >
> > Why not?
> > 	return sprintf...



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