linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics
@ 2020-01-15 20:42 Florian Fainelli
  2020-01-15 23:19 ` Andrew Lunn
  2020-01-15 23:53 ` Andrew Lunn
  0 siblings, 2 replies; 4+ messages in thread
From: Florian Fainelli @ 2020-01-15 20:42 UTC (permalink / raw)
  To: netdev
  Cc: cphealy, rmk+kernel, Florian Fainelli, Andrew Lunn,
	Heiner Kallweit, David S. Miller, open list

We maintain global statistics for an entire MDIO bus, as well as broken
down, per MDIO bus address statistics. Given that it is possible for
MDIO devices such as switches to access MDIO bus addressies for which
there is not a mdio_device instance created (therefore not a a
corresponding device directory in sysfs either), we also maintain
per-address statistics under the statistics folder. The layout looks
like this:

/sys/class/mdio_bus/../statistics/
	transfers
	errrors
	writes
	reads
	transfers_<addr>
	errors_<addr>
	writes_<addr>
	reads_<addr>

When a mdio_device instance is registered, a statistics/ folder is
created with the tranfers, errors, writes and reads attributes which
point to the appropriate MDIO bus statistics structure.

Statistics are 64-bit unsigned quantities and maintained through the
u64_stats_sync.h helper functions.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- tracked per MDIO address statististics in separate attributes
- global statistics sum all per MDIO address statistics instead of
  requiring another stats structure

 Documentation/ABI/testing/sysfs-bus-mdio |  63 +++++++
 drivers/net/phy/mdio_bus.c               | 226 ++++++++++++++++++++++-
 include/linux/phy.h                      |  10 +
 3 files changed, 297 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-mdio

diff --git a/Documentation/ABI/testing/sysfs-bus-mdio b/Documentation/ABI/testing/sysfs-bus-mdio
new file mode 100644
index 000000000000..da86efc7781b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-mdio
@@ -0,0 +1,63 @@
+What:          /sys/bus/mdio_bus/devices/.../statistics/
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		This folder contains statistics about global and per
+		MDIO bus address statistics.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/transfers
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of transfers for this MDIO bus.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/errors
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of transfer errors for this MDIO bus.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/writes
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of write transactions for this MDIO bus.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/reads
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of read transactions for this MDIO bus.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/transfers_<addr>
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of transfers for this MDIO bus address.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/errors_<addr>
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of transfer errors for this MDIO bus address.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/writes_<addr>
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of write transactions for this MDIO bus address.
+
+What:          /sys/bus/mdio_bus/devices/.../statistics/reads_<addr>
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		Total number of read transactions for this MDIO bus address.
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8d753bb07227..f2d017b09362 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -158,9 +158,11 @@ struct mii_bus *mdiobus_alloc_size(size_t size)
 	if (size)
 		bus->priv = (void *)bus + aligned_size;
 
-	/* Initialise the interrupts to polling */
-	for (i = 0; i < PHY_MAX_ADDR; i++)
+	/* Initialise the interrupts to polling and 64-bit seqcounts */
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		bus->irq[i] = PHY_POLL;
+		u64_stats_init(&bus->stats[i].syncp);
+	}
 
 	return bus;
 }
@@ -249,9 +251,190 @@ static void mdiobus_release(struct device *d)
 	kfree(bus);
 }
 
+#define MDIO_BUS_STATS_ATTR(field, file)				\
+static ssize_t mdio_bus_##field##_show(struct device *dev,		\
+				       struct device_attribute *attr,	\
+				       char *buf)			\
+{									\
+	struct mii_bus *bus = to_mii_bus(dev);				\
+	return mdio_bus_global_stats_##field##_show(bus, buf);		\
+}									\
+static struct device_attribute dev_attr_mdio_bus_##field = {		\
+	.attr = { .name = file, .mode = 0444 },				\
+	.show = mdio_bus_##field##_show,				\
+};									\
+static ssize_t mdio_bus_device_##field##_show(struct device *dev,	\
+					      struct device_attribute *attr,\
+					      char *buf)		\
+{									\
+	struct mdio_device *mdiodev = to_mdio_device(dev);		\
+	struct mii_bus *bus = mdiodev->bus;				\
+	int addr = mdiodev->addr;					\
+	return mdio_bus_stats_##field##_show(&bus->stats[addr], buf);	\
+}									\
+static struct device_attribute dev_attr_mdio_bus_device_##field = {	\
+	.attr = { .name = file, .mode = 0444 },				\
+	.show = mdio_bus_device_##field##_show,				\
+}
+
+#define MDIO_BUS_STATS_SHOW_NAME(name, file, field)			\
+static ssize_t mdio_bus_stats_##name##_show(struct mdio_bus_stats *s,	\
+					    char *buf)			\
+{									\
+	unsigned int start;						\
+	ssize_t len;							\
+	u64 tmp = 0;							\
+	do {								\
+		start = u64_stats_fetch_begin(&s->syncp);		\
+		tmp += u64_stats_read(&s->field);			\
+	} while (u64_stats_fetch_retry(&s->syncp, start));		\
+	len = sprintf(buf, "%llu\n", tmp);				\
+	return len;							\
+}									\
+static ssize_t mdio_bus_global_stats_##name##_show(struct mii_bus *bus,	\
+						   char *buf)		\
+{									\
+	struct mdio_bus_stats *s;					\
+	unsigned int start;						\
+	unsigned int i;							\
+	ssize_t len;							\
+	u64 tmp = 0;							\
+	for (i = 0; i < PHY_MAX_ADDR; i++) {				\
+		s = &bus->stats[i];					\
+		do {							\
+			start = u64_stats_fetch_begin(&s->syncp);	\
+			tmp += u64_stats_read(&s->field);		\
+		} while (u64_stats_fetch_retry(&s->syncp, start));	\
+	}								\
+	len = sprintf(buf, "%llu\n", tmp);				\
+	return len;							\
+}									\
+MDIO_BUS_STATS_ATTR(name, file)
+
+#define MDIO_BUS_STATS_SHOW(field)					\
+	MDIO_BUS_STATS_SHOW_NAME(field, __stringify(field), field)
+
+#define MDIO_BUS_STATS_ADDR_ATTR(field, addr, file)			\
+static ssize_t mdio_bus_##field##_##addr##_show(struct device *dev,	\
+						struct device_attribute *attr, \
+						char *buf)		\
+{									\
+	struct mii_bus *bus = to_mii_bus(dev);				\
+	return mdio_bus_stats_##field##_show(&bus->stats[addr], buf);	\
+}									\
+static struct device_attribute dev_attr_mdio_bus_addr_##field##_##addr = { \
+	.attr = { .name = file, .mode = 0444 },				\
+	.show = mdio_bus_##field##_##addr##_show,			\
+}									\
+
+#define MDIO_BUS_STATS_ADDR_SHOW(field, addr)				\
+	MDIO_BUS_STATS_ADDR_ATTR(field, addr,				\
+				 __stringify(field) "_" __stringify(addr))
+
+MDIO_BUS_STATS_SHOW(transfers);
+MDIO_BUS_STATS_SHOW(errors);
+MDIO_BUS_STATS_SHOW(writes);
+MDIO_BUS_STATS_SHOW(reads);
+
+#define MDIO_BUS_STATS_ADDR_SHOW_GROUP(addr)				\
+	MDIO_BUS_STATS_ADDR_SHOW(transfers, addr);			\
+	MDIO_BUS_STATS_ADDR_SHOW(errors, addr);				\
+	MDIO_BUS_STATS_ADDR_SHOW(writes, addr);				\
+	MDIO_BUS_STATS_ADDR_SHOW(reads, addr)				\
+
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(0);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(1);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(2);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(3);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(4);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(5);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(6);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(7);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(8);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(9);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(10);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(11);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(12);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(13);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(14);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(15);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(16);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(17);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(18);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(19);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(20);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(21);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(22);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(23);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(24);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(25);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(26);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(27);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(28);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(29);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(30);
+MDIO_BUS_STATS_ADDR_SHOW_GROUP(31);
+
+#define MDIO_BUS_STATS_ADDR_ATTR_GROUP(addr)				\
+	&dev_attr_mdio_bus_addr_transfers_##addr.attr,			\
+	&dev_attr_mdio_bus_addr_errors_##addr.attr,			\
+	&dev_attr_mdio_bus_addr_writes_##addr.attr,			\
+	&dev_attr_mdio_bus_addr_reads_##addr.attr			\
+
+static struct attribute *mdio_bus_statistics_attrs[] = {
+	&dev_attr_mdio_bus_transfers.attr,
+	&dev_attr_mdio_bus_errors.attr,
+	&dev_attr_mdio_bus_writes.attr,
+	&dev_attr_mdio_bus_reads.attr,
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(0),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(1),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(2),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(3),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(4),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(5),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(6),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(7),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(8),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(9),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(10),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(11),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(12),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(13),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(14),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(15),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(16),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(17),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(18),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(19),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(20),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(21),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(22),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(23),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(24),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(25),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(26),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(27),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(28),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(29),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(30),
+	MDIO_BUS_STATS_ADDR_ATTR_GROUP(31),
+	NULL,
+};
+
+static const struct attribute_group mdio_bus_statistics_group = {
+	.name	= "statistics",
+	.attrs	= mdio_bus_statistics_attrs,
+};
+
+static const struct attribute_group *mdio_bus_groups[] = {
+	&mdio_bus_statistics_group,
+	NULL,
+};
+
 static struct class mdio_bus_class = {
 	.name		= "mdio_bus",
 	.dev_release	= mdiobus_release,
+	.dev_groups	= mdio_bus_groups,
 };
 
 #if IS_ENABLED(CONFIG_OF_MDIO)
@@ -530,6 +713,24 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 }
 EXPORT_SYMBOL(mdiobus_scan);
 
+static void mdiobus_stats_acct(struct mdio_bus_stats *stats, bool op, int ret)
+{
+	u64_stats_update_begin(&stats->syncp);
+
+	u64_stats_inc(&stats->transfers);
+	if (ret < 0) {
+		u64_stats_inc(&stats->errors);
+		goto out;
+	}
+
+	if (op)
+		u64_stats_inc(&stats->reads);
+	else
+		u64_stats_inc(&stats->writes);
+out:
+	u64_stats_update_end(&stats->syncp);
+}
+
 /**
  * __mdiobus_read - Unlocked version of the mdiobus_read function
  * @bus: the mii_bus struct
@@ -549,6 +750,7 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 	retval = bus->read(bus, addr, regnum);
 
 	trace_mdio_access(bus, 1, addr, regnum, retval, retval);
+	mdiobus_stats_acct(&bus->stats[addr], true, retval);
 
 	return retval;
 }
@@ -574,6 +776,7 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 	err = bus->write(bus, addr, regnum, val);
 
 	trace_mdio_access(bus, 0, addr, regnum, val, err);
+	mdiobus_stats_acct(&bus->stats[addr], false, err);
 
 	return err;
 }
@@ -719,8 +922,27 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+static struct attribute *mdio_bus_device_statistics_attrs[] = {
+	&dev_attr_mdio_bus_device_transfers.attr,
+	&dev_attr_mdio_bus_device_errors.attr,
+	&dev_attr_mdio_bus_device_writes.attr,
+	&dev_attr_mdio_bus_device_reads.attr,
+	NULL,
+};
+
+static const struct attribute_group mdio_bus_device_statistics_group = {
+	.name	= "statistics",
+	.attrs	= mdio_bus_device_statistics_attrs,
+};
+
+static const struct attribute_group *mdio_bus_dev_groups[] = {
+	&mdio_bus_device_statistics_group,
+	NULL,
+};
+
 struct bus_type mdio_bus_type = {
 	.name		= "mdio_bus",
+	.dev_groups	= mdio_bus_dev_groups,
 	.match		= mdio_bus_match,
 	.uevent		= mdio_uevent,
 };
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2929d0bc307f..b7de7d45135e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -22,6 +22,7 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 #include <linux/mod_devicetable.h>
+#include <linux/u64_stats_sync.h>
 
 #include <linux/atomic.h>
 
@@ -212,6 +213,14 @@ struct sfp_bus;
 struct sfp_upstream_ops;
 struct sk_buff;
 
+struct mdio_bus_stats {
+	struct u64_stats_sync syncp;
+	u64_stats_t transfers;
+	u64_stats_t errors;
+	u64_stats_t writes;
+	u64_stats_t reads;
+};
+
 /*
  * The Bus class for PHYs.  Devices which provide access to
  * PHYs should register using this structure
@@ -224,6 +233,7 @@ struct mii_bus {
 	int (*read)(struct mii_bus *bus, int addr, int regnum);
 	int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
 	int (*reset)(struct mii_bus *bus);
+	struct mdio_bus_stats stats[PHY_MAX_ADDR];
 
 	/*
 	 * A lock to ensure that only one thing can read/write
-- 
2.17.1


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

* Re: [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics
  2020-01-15 20:42 [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
@ 2020-01-15 23:19 ` Andrew Lunn
  2020-01-15 23:53 ` Andrew Lunn
  1 sibling, 0 replies; 4+ messages in thread
From: Andrew Lunn @ 2020-01-15 23:19 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, cphealy, rmk+kernel, Heiner Kallweit, David S. Miller, open list

On Wed, Jan 15, 2020 at 12:42:20PM -0800, Florian Fainelli wrote:
> We maintain global statistics for an entire MDIO bus, as well as broken
> down, per MDIO bus address statistics. Given that it is possible for
> MDIO devices such as switches to access MDIO bus addressies for which
> there is not a mdio_device instance created (therefore not a a
> corresponding device directory in sysfs either), we also maintain
> per-address statistics under the statistics folder. The layout looks
> like this:
> 
> /sys/class/mdio_bus/../statistics/
> 	transfers
> 	errrors
> 	writes
> 	reads
> 	transfers_<addr>
> 	errors_<addr>
> 	writes_<addr>
> 	reads_<addr>
> 
> When a mdio_device instance is registered, a statistics/ folder is
> created with the tranfers, errors, writes and reads attributes which
> point to the appropriate MDIO bus statistics structure.
> 
> Statistics are 64-bit unsigned quantities and maintained through the
> u64_stats_sync.h helper functions.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v2:
> 
> - tracked per MDIO address statististics in separate attributes

Hi Florian

This is much better. Here is an MDIO bus with a Marvel MV88E6390

andrew@zii-devel-c-bidi:/sys/class/mdio_bus/0.1/statistics$ awk ' { print FILENAME " " $0  } ' transfers_? transfers_?? transfers
transfers_0 93
transfers_1 80
transfers_2 80
transfers_3 80
transfers_4 102
transfers_5 18
transfers_6 7
transfers_7 7
transfers_8 7
transfers_9 7
transfers_10 82
transfers_11 0
transfers_12 0
transfers_13 0
transfers_14 0
transfers_15 0
transfers_16 0
transfers_17 0
transfers_18 0
transfers_19 0
transfers_20 0
transfers_21 0
transfers_22 0
transfers_23 0
transfers_24 0
transfers_25 0
transfers_26 0
transfers_27 288
transfers_28 3328
transfers_29 0
transfers_30 0
transfers_31 0
transfers 4179

As you can see, there are transfers on a number of addresses.

I've not looked at the code yet, but i can give:

Tested-by: Andrew Lunn <andrew@lunn.ch>

I will review the code soon.

    Andrew

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

* Re: [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics
  2020-01-15 20:42 [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
  2020-01-15 23:19 ` Andrew Lunn
@ 2020-01-15 23:53 ` Andrew Lunn
  2020-01-16  0:01   ` Florian Fainelli
  1 sibling, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2020-01-15 23:53 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, cphealy, rmk+kernel, Heiner Kallweit, David S. Miller, open list

> +#define MDIO_BUS_STATS_ADDR_ATTR(field, addr, file)			\
> +static ssize_t mdio_bus_##field##_##addr##_show(struct device *dev,	\
> +						struct device_attribute *attr, \
> +						char *buf)		\
> +{									\
> +	struct mii_bus *bus = to_mii_bus(dev);				\
> +	return mdio_bus_stats_##field##_show(&bus->stats[addr], buf);	\
> +}									\

Hi Florian

Lots of Macro magic here. But it is reasonably understandable.
However, the compiler is maybe not doing the best of jobs:

00000064 l     F .text	00000030 mdio_bus_reads_31_show
00000094 l     F .text	00000030 mdio_bus_reads_30_show
000000c4 l     F .text	00000030 mdio_bus_reads_29_show
000000f4 l     F .text	00000030 mdio_bus_reads_28_show
00000124 l     F .text	00000030 mdio_bus_reads_27_show
00000154 l     F .text	00000030 mdio_bus_reads_26_show
00000184 l     F .text	00000030 mdio_bus_reads_25_show
000001b4 l     F .text	00000034 mdio_bus_reads_24_show
000001e8 l     F .text	00000034 mdio_bus_reads_23_show
0000021c l     F .text	00000034 mdio_bus_reads_22_show
00000250 l     F .text	00000034 mdio_bus_reads_21_show
00000284 l     F .text	00000034 mdio_bus_reads_20_show
000002b8 l     F .text	00000034 mdio_bus_reads_19_show
000002ec l     F .text	00000034 mdio_bus_reads_18_show
00000320 l     F .text	00000034 mdio_bus_reads_17_show
00000354 l     F .text	00000034 mdio_bus_reads_16_show
00000388 l     F .text	00000034 mdio_bus_reads_15_show
000003bc l     F .text	00000034 mdio_bus_reads_14_show
000003f0 l     F .text	00000034 mdio_bus_reads_13_show
00000424 l     F .text	00000034 mdio_bus_reads_12_show
00000458 l     F .text	00000034 mdio_bus_reads_11_show
0000048c l     F .text	00000034 mdio_bus_reads_10_show
000004c0 l     F .text	00000034 mdio_bus_reads_9_show
000004f4 l     F .text	00000034 mdio_bus_reads_8_show
00000528 l     F .text	00000034 mdio_bus_reads_7_show
0000055c l     F .text	00000034 mdio_bus_reads_6_show
00000590 l     F .text	00000034 mdio_bus_reads_5_show
000005c4 l     F .text	00000034 mdio_bus_reads_4_show
000005f8 l     F .text	00000034 mdio_bus_reads_3_show
0000062c l     F .text	00000034 mdio_bus_reads_2_show
00000660 l     F .text	00000034 mdio_bus_reads_1_show
00000694 l     F .text	00000034 mdio_bus_reads_0_show

It appears to be inlining everything, so end up with lots of
functions, and they are not tiny.

I'm wondering if we can get ride of this per address
reads/write/transfer function. Could you stuff the addr into var of
struct dev_ext_attribute?

https://elixir.bootlin.com/linux/latest/source/include/linux/device.h#L813

	Andrew

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

* Re: [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics
  2020-01-15 23:53 ` Andrew Lunn
@ 2020-01-16  0:01   ` Florian Fainelli
  0 siblings, 0 replies; 4+ messages in thread
From: Florian Fainelli @ 2020-01-16  0:01 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, cphealy, rmk+kernel, Heiner Kallweit, David S. Miller, open list

Hi Andrew,

On 1/15/20 3:53 PM, Andrew Lunn wrote:
>> +#define MDIO_BUS_STATS_ADDR_ATTR(field, addr, file)			\
>> +static ssize_t mdio_bus_##field##_##addr##_show(struct device *dev,	\
>> +						struct device_attribute *attr, \
>> +						char *buf)		\
>> +{									\
>> +	struct mii_bus *bus = to_mii_bus(dev);				\
>> +	return mdio_bus_stats_##field##_show(&bus->stats[addr], buf);	\
>> +}									\
> 
> Hi Florian
> 
> Lots of Macro magic here. But it is reasonably understandable.
> However, the compiler is maybe not doing the best of jobs:
> 
> 00000064 l     F .text	00000030 mdio_bus_reads_31_show
> 00000094 l     F .text	00000030 mdio_bus_reads_30_show
> 000000c4 l     F .text	00000030 mdio_bus_reads_29_show
> 000000f4 l     F .text	00000030 mdio_bus_reads_28_show
> 00000124 l     F .text	00000030 mdio_bus_reads_27_show
> 00000154 l     F .text	00000030 mdio_bus_reads_26_show
> 00000184 l     F .text	00000030 mdio_bus_reads_25_show
> 000001b4 l     F .text	00000034 mdio_bus_reads_24_show
> 000001e8 l     F .text	00000034 mdio_bus_reads_23_show
> 0000021c l     F .text	00000034 mdio_bus_reads_22_show
> 00000250 l     F .text	00000034 mdio_bus_reads_21_show
> 00000284 l     F .text	00000034 mdio_bus_reads_20_show
> 000002b8 l     F .text	00000034 mdio_bus_reads_19_show
> 000002ec l     F .text	00000034 mdio_bus_reads_18_show
> 00000320 l     F .text	00000034 mdio_bus_reads_17_show
> 00000354 l     F .text	00000034 mdio_bus_reads_16_show
> 00000388 l     F .text	00000034 mdio_bus_reads_15_show
> 000003bc l     F .text	00000034 mdio_bus_reads_14_show
> 000003f0 l     F .text	00000034 mdio_bus_reads_13_show
> 00000424 l     F .text	00000034 mdio_bus_reads_12_show
> 00000458 l     F .text	00000034 mdio_bus_reads_11_show
> 0000048c l     F .text	00000034 mdio_bus_reads_10_show
> 000004c0 l     F .text	00000034 mdio_bus_reads_9_show
> 000004f4 l     F .text	00000034 mdio_bus_reads_8_show
> 00000528 l     F .text	00000034 mdio_bus_reads_7_show
> 0000055c l     F .text	00000034 mdio_bus_reads_6_show
> 00000590 l     F .text	00000034 mdio_bus_reads_5_show
> 000005c4 l     F .text	00000034 mdio_bus_reads_4_show
> 000005f8 l     F .text	00000034 mdio_bus_reads_3_show
> 0000062c l     F .text	00000034 mdio_bus_reads_2_show
> 00000660 l     F .text	00000034 mdio_bus_reads_1_show
> 00000694 l     F .text	00000034 mdio_bus_reads_0_show
> 
> It appears to be inlining everything, so end up with lots of
> functions, and they are not tiny.
> 
> I'm wondering if we can get ride of this per address
> reads/write/transfer function. Could you stuff the addr into var of
> struct dev_ext_attribute?

That is a great suggestion, thanks! This should reduce the number of
those _show functions, expect v3 soon.
-- 
Florian

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

end of thread, other threads:[~2020-01-16  0:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 20:42 [PATCH net-next v2] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
2020-01-15 23:19 ` Andrew Lunn
2020-01-15 23:53 ` Andrew Lunn
2020-01-16  0:01   ` Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).