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:

quoted

On Wed, Sep 05, 2018 at 04:50:35PM +0300, Leon Romanovsky wrote:

quoted

On Mon, Sep 03, 2018 at 07:13:16PM +0300, Arseny Maslennikov wrote:

quoted

Signed-off-by: Arseny Maslennikov <redacted>
---
 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.

quoted

quoted

+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 [off-list ref]
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD