All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
@ 2020-01-13  4:53 Florian Fainelli
  2020-01-13  4:55 ` Florian Fainelli
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Florian Fainelli @ 2020-01-13  4:53 UTC (permalink / raw)
  To: netdev
  Cc: cphealy, rmk+kernel, kuba, Florian Fainelli, Andrew Lunn,
	Heiner Kallweit, David S. Miller, open list

Maintain per MDIO device and MDIO bus statistics comprised of the number
of transfers/operations, reads and writes and errors. This is useful for
tracking the per-device and global MDIO bus bandwidth and doing
optimizations as necessary.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-mdio |  34 +++++++
 drivers/net/phy/mdio_bus.c               | 116 +++++++++++++++++++++++
 drivers/net/phy/mdio_device.c            |   1 +
 include/linux/mdio.h                     |  10 ++
 include/linux/phy.h                      |   2 +
 5 files changed, 163 insertions(+)
 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..a552d92890f1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-mdio
@@ -0,0 +1,34 @@
+What:          /sys/bus/mdio_bus/devices/.../statistics/
+Date:          January 2020
+KernelVersion: 5.6
+Contact:       netdev@vger.kernel.org
+Description:
+		This folder contains statistics about MDIO bus transactions.
+
+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.
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 229e480179ff..805bc2e3b139 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -168,6 +168,9 @@ struct mii_bus *mdiobus_alloc_size(size_t size)
 	for (i = 0; i < PHY_MAX_ADDR; i++)
 		bus->irq[i] = PHY_POLL;
 
+	/* Initialize 64-bit seqcounts */
+	u64_stats_init(&bus->stats.syncp);
+
 	return bus;
 }
 EXPORT_SYMBOL(mdiobus_alloc_size);
@@ -255,9 +258,77 @@ 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_stats_##field##_show(&bus->stats, 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);		\
+	return mdio_bus_stats_##field##_show(&mdiodev->stats, 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, format_string)	\
+static ssize_t mdio_bus_stats_##name##_show(struct mdio_bus_stats *s,	\
+					    char *buf)			\
+{									\
+	unsigned int start;						\
+	ssize_t len;							\
+	u64 tmp;							\
+	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, format_string ## "\n", tmp);			\
+	return len;							\
+}									\
+MDIO_BUS_STATS_ATTR(name, file)
+
+#define MDIO_BUS_STATS_SHOW(field, format_string)			\
+	MDIO_BUS_STATS_SHOW_NAME(field, __stringify(field),		\
+				      field, format_string)
+
+MDIO_BUS_STATS_SHOW(transfers, "%llu");
+MDIO_BUS_STATS_SHOW(errors, "%llu");
+MDIO_BUS_STATS_SHOW(writes, "%llu");
+MDIO_BUS_STATS_SHOW(reads, "%llu");
+
+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,
+	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)
@@ -536,6 +607,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
@@ -548,6 +637,7 @@ EXPORT_SYMBOL(mdiobus_scan);
  */
 int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 {
+	struct mdio_device *mdiodev = bus->mdio_map[addr];
 	int retval;
 
 	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
@@ -555,6 +645,9 @@ 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, true, retval);
+	if (mdiodev)
+		mdiobus_stats_acct(&mdiodev->stats, true, retval);
 
 	return retval;
 }
@@ -573,6 +666,7 @@ EXPORT_SYMBOL(__mdiobus_read);
  */
 int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 {
+	struct mdio_device *mdiodev = bus->mdio_map[addr];
 	int err;
 
 	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
@@ -580,6 +674,9 @@ 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, false, err);
+	if (mdiodev)
+		mdiobus_stats_acct(&mdiodev->stats, false, err);
 
 	return err;
 }
@@ -725,8 +822,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/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index c1d345c3cab3..e89ed990de7d 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -53,6 +53,7 @@ struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr)
 	if (!mdiodev)
 		return ERR_PTR(-ENOMEM);
 
+	u64_stats_init(&mdiodev->stats.syncp);
 	mdiodev->dev.release = mdio_device_release;
 	mdiodev->dev.parent = &bus->dev;
 	mdiodev->dev.bus = &mdio_bus_type;
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index a7604248777b..d6035d973a0d 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -8,6 +8,7 @@
 
 #include <uapi/linux/mdio.h>
 #include <linux/mod_devicetable.h>
+#include <linux/u64_stats_sync.h>
 
 struct gpio_desc;
 struct mii_bus;
@@ -23,11 +24,20 @@ enum mdio_mutex_lock_class {
 	MDIO_MUTEX_NESTED,
 };
 
+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;
+};
+
 struct mdio_device {
 	struct device dev;
 
 	struct mii_bus *bus;
 	char modalias[MDIO_NAME_SIZE];
+	struct mdio_bus_stats stats;
 
 	int (*bus_match)(struct device *dev, struct device_driver *drv);
 	void (*device_free)(struct mdio_device *mdiodev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5932bb8e9c35..8d3ac1ebfef2 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>
 
@@ -224,6 +225,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;
 
 	/*
 	 * A lock to ensure that only one thing can read/write
-- 
2.19.1


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

* Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
  2020-01-13  4:53 [PATCH net-next] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
@ 2020-01-13  4:55 ` Florian Fainelli
  2020-01-13 13:21 ` Andrew Lunn
  2020-01-14  4:44   ` kbuild test robot
  2 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2020-01-13  4:55 UTC (permalink / raw)
  To: netdev
  Cc: cphealy, rmk+kernel, kuba, Andrew Lunn, Heiner Kallweit,
	David S. Miller, open list



On 1/12/2020 8:53 PM, Florian Fainelli wrote:
> Maintain per MDIO device and MDIO bus statistics comprised of the number
> of transfers/operations, reads and writes and errors. This is useful for
> tracking the per-device and global MDIO bus bandwidth and doing
> optimizations as necessary.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

[snip]

> +#define MDIO_BUS_STATS_SHOW_NAME(name, file, field, format_string)	\
> +static ssize_t mdio_bus_stats_##name##_show(struct mdio_bus_stats *s,	\
> +					    char *buf)			\
> +{									\
> +	unsigned int start;						\
> +	ssize_t len;							\
> +	u64 tmp;							\
> +	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, format_string ## "\n", tmp);			\

	^ ===== that hunk right there does not build, I sent a non-fixed up
patch, I would still appreciate comments though.
-- 
Florian

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

* Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
  2020-01-13  4:53 [PATCH net-next] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
  2020-01-13  4:55 ` Florian Fainelli
@ 2020-01-13 13:21 ` Andrew Lunn
  2020-01-13 18:00   ` Florian Fainelli
  2020-01-14  4:44   ` kbuild test robot
  2 siblings, 1 reply; 7+ messages in thread
From: Andrew Lunn @ 2020-01-13 13:21 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, cphealy, rmk+kernel, kuba, Heiner Kallweit,
	David S. Miller, open list

On Sun, Jan 12, 2020 at 08:53:19PM -0800, Florian Fainelli wrote:
> Maintain per MDIO device and MDIO bus statistics comprised of the number
> of transfers/operations, reads and writes and errors. This is useful for
> tracking the per-device and global MDIO bus bandwidth and doing
> optimizations as necessary.

Hi Florian

One point for discussion is, is sysfs the right way to do this?
Should we be using ethtool and exporting the statistics like other
statistics?

The argument against it, is we have devices which are not related to a
network interfaces on MDIO busses. For a PHY we could plumb the per
PHY mdio device statistics into the exiting PHY statistics. But we
also have Ethernet switches on MDIO devices, which don't have an
association to a netdev interface. Broadcom also have some generic PHY
device on MDIO busses, for USB, SATA, etc. And whole bus statistics
don't fit the netdev model at all.

So sysfs does make sense. But i would also suggest we do plumb per PHY
MDIO device statistics into the exiting ethtool call.

> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-mdio |  34 +++++++
>  drivers/net/phy/mdio_bus.c               | 116 +++++++++++++++++++++++
>  drivers/net/phy/mdio_device.c            |   1 +
>  include/linux/mdio.h                     |  10 ++
>  include/linux/phy.h                      |   2 +
>  5 files changed, 163 insertions(+)
>  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..a552d92890f1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-mdio
> @@ -0,0 +1,34 @@
> +What:          /sys/bus/mdio_bus/devices/.../statistics/
> +Date:          January 2020
> +KernelVersion: 5.6
> +Contact:       netdev@vger.kernel.org
> +Description:
> +		This folder contains statistics about MDIO bus transactions.
> +
> +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.

Looking at this description, it is not clear we have whole bus and per
device statistics. 

>  int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
>  {
> +	struct mdio_device *mdiodev = bus->mdio_map[addr];
>  	int retval;
>  
>  	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
> @@ -555,6 +645,9 @@ 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, true, retval);
> +	if (mdiodev)
> +		mdiobus_stats_acct(&mdiodev->stats, true, retval);
>  
>  	return retval;

I think for most Ethernet switches, these per device counters are
going to be misleading. The switch often takes up multiple addresses
on the bus, but the switch is represented as a single mdiodev with one
address. So the counters will reflect the transfers on that one
address, not the whole switch. The device tree binding does not have
enough information for us to associated one mdiodev to multiple
addresses. And for some of the Marvell switches, it is a sparse address
map, and i have seen PHY devices in the holes. So in the sysfs
documentation, we should probably add a warning that when used with an
Ethernet switch, the counters are unlikely to be accurate, and should
be interpreted with care.

   Andrew

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

* Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
  2020-01-13 13:21 ` Andrew Lunn
@ 2020-01-13 18:00   ` Florian Fainelli
  2020-01-13 18:29     ` Andrew Lunn
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2020-01-13 18:00 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, cphealy, rmk+kernel, kuba, Heiner Kallweit,
	David S. Miller, open list

Hi Andrew,

On 1/13/20 5:21 AM, Andrew Lunn wrote:
> On Sun, Jan 12, 2020 at 08:53:19PM -0800, Florian Fainelli wrote:
>> Maintain per MDIO device and MDIO bus statistics comprised of the number
>> of transfers/operations, reads and writes and errors. This is useful for
>> tracking the per-device and global MDIO bus bandwidth and doing
>> optimizations as necessary.
> 
> Hi Florian
> 
> One point for discussion is, is sysfs the right way to do this?
> Should we be using ethtool and exporting the statistics like other
> statistics?
> 
> The argument against it, is we have devices which are not related to a
> network interfaces on MDIO busses. For a PHY we could plumb the per
> PHY mdio device statistics into the exiting PHY statistics. But we
> also have Ethernet switches on MDIO devices, which don't have an
> association to a netdev interface. Broadcom also have some generic PHY
> device on MDIO busses, for USB, SATA, etc. And whole bus statistics
> don't fit the netdev model at all.

Correct, that was the reasoning, which I should probably put in the
commit message.

> 
> So sysfs does make sense. But i would also suggest we do plumb per PHY
> MDIO device statistics into the exiting ethtool call.

It looks like replicating statistics that are already available via
another mechanism is kind of frowned upon, see this for an example:



> 
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-mdio |  34 +++++++
>>  drivers/net/phy/mdio_bus.c               | 116 +++++++++++++++++++++++
>>  drivers/net/phy/mdio_device.c            |   1 +
>>  include/linux/mdio.h                     |  10 ++
>>  include/linux/phy.h                      |   2 +
>>  5 files changed, 163 insertions(+)
>>  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..a552d92890f1
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-mdio
>> @@ -0,0 +1,34 @@
>> +What:          /sys/bus/mdio_bus/devices/.../statistics/
>> +Date:          January 2020
>> +KernelVersion: 5.6
>> +Contact:       netdev@vger.kernel.org
>> +Description:
>> +		This folder contains statistics about MDIO bus transactions.
>> +
>> +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.
> 
> Looking at this description, it is not clear we have whole bus and per
> device statistics. 
> 
>>  int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
>>  {
>> +	struct mdio_device *mdiodev = bus->mdio_map[addr];
>>  	int retval;
>>  
>>  	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
>> @@ -555,6 +645,9 @@ 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, true, retval);
>> +	if (mdiodev)
>> +		mdiobus_stats_acct(&mdiodev->stats, true, retval);
>>  
>>  	return retval;
> 
> I think for most Ethernet switches, these per device counters are
> going to be misleading. The switch often takes up multiple addresses
> on the bus, but the switch is represented as a single mdiodev with one
> address.

For MDIO switches you would usually have the mdio_device claim the
pseudo PHY address and all other MDIO addresses should correspond to
built-in PHYs, for which we also have mdio_device instances, is there a
case that I am missing?

> So the counters will reflect the transfers on that one
> address, not the whole switch. The device tree binding does not have
> enough information for us to associated one mdiodev to multiple
> addresses. And for some of the Marvell switches, it is a sparse address
> map, and i have seen PHY devices in the holes. So in the sysfs
> documentation, we should probably add a warning that when used with an
> Ethernet switch, the counters are unlikely to be accurate, and should
> be interpreted with care.

If the answer to my question above is that we still have reads to
addresses for which we do not have mdio_device (which we might very well
have), then we could either:

- create <mdio_bus>:<address>/statistics/ folders even for non-existent
devices, but just to track the per-address statistics
- create <mdio_bus>/<address>/statistics and when a mdio_device instance
exists we symbolic link <mdio_bus>:<address>/statistics ->
../<mdio_bus>/<addr>/statistics

Would that work?
-- 
Florian

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

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

> For MDIO switches you would usually have the mdio_device claim the
> pseudo PHY address and all other MDIO addresses should correspond to
> built-in PHYs, for which we also have mdio_device instances, is there a
> case that I am missing?

Marvell switches don't work like this. It varies from family to
family. The 6390 family for example has address 0x0-0xa representing
registers for ports 0 to 10, 32 registers per port. address 0x1b and
0x1c contain registers for global configuration. 0x1e is used to
communication with the internal MCU, and 0x1f is the TCAM. The
internal PHYs are not in this MDIO address space at all, there is
another MDIO bus, implemented by a couple of the global registers.

The 6352 has a different layout. 0x0-0x4 represent the internal
PHYs. 0xf is the SERDES for SGMII, 0x10-x016 are the port registers,
0x1b 0x1c are global.

There is one family, i don't remember which, which uses 16
addresses. And you can put two of them on one MDIO bus.

In each of these cases, we have one mdiodev, per switch, claiming a
single address, generally 0.

Other vendors switches are similar, using multiple addresses on one
bus.

> If the answer to my question above is that we still have reads to
> addresses for which we do not have mdio_device (which we might very well
> have), then we could either:
> 
> - create <mdio_bus>:<address>/statistics/ folders even for non-existent
> devices, but just to track the per-address statistics
> - create <mdio_bus>/<address>/statistics and when a mdio_device instance
> exists we symbolic link <mdio_bus>:<address>/statistics ->
> ../<mdio_bus>/<addr>/statistics
> 
> Would that work?

Keeping the statistics for all 32 addresses in the struct mii_bus
would be good. Then directories 0-31. And symlinks. Yes, that is good.

      Andrew

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

* Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
  2020-01-13  4:53 [PATCH net-next] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
@ 2020-01-14  4:44   ` kbuild test robot
  2020-01-13 13:21 ` Andrew Lunn
  2020-01-14  4:44   ` kbuild test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-01-14  4:44 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: kbuild-all, netdev, cphealy, rmk+kernel, kuba, Florian Fainelli,
	Andrew Lunn, Heiner Kallweit, David S. Miller, open list

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master ipvs/master v5.5-rc6 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-phy-Maintain-MDIO-device-and-bus-statistics/20200113-125529
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5c9166f038257d6130865b267cb45d87f126b3dd
config: x86_64-lkp (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_transfers_show':
   drivers/net/phy/mdio_bus.c:305:32: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(transfers, "%llu");
                                   ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
>> drivers/net/phy/mdio_bus.c:305:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(transfers, "%llu");
    ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_errors_show':
   drivers/net/phy/mdio_bus.c:306:29: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(errors, "%llu");
                                ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c:306:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(errors, "%llu");
    ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_writes_show':
   drivers/net/phy/mdio_bus.c:307:29: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(writes, "%llu");
                                ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c:307:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(writes, "%llu");
    ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_reads_show':
   drivers/net/phy/mdio_bus.c:308:28: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(reads, "%llu");
                               ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c:308:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(reads, "%llu");
    ^~~~~~~~~~~~~~~~~~~

vim +305 drivers/net/phy/mdio_bus.c

   300	
   301	#define MDIO_BUS_STATS_SHOW(field, format_string)			\
   302		MDIO_BUS_STATS_SHOW_NAME(field, __stringify(field),		\
   303					      field, format_string)
   304	
 > 305	MDIO_BUS_STATS_SHOW(transfers, "%llu");
   306	MDIO_BUS_STATS_SHOW(errors, "%llu");
   307	MDIO_BUS_STATS_SHOW(writes, "%llu");
   308	MDIO_BUS_STATS_SHOW(reads, "%llu");
   309	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28745 bytes --]

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

* Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics
@ 2020-01-14  4:44   ` kbuild test robot
  0 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-01-14  4:44 UTC (permalink / raw)
  To: kbuild-all

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

Hi Florian,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master ipvs/master v5.5-rc6 next-20200110]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Florian-Fainelli/net-phy-Maintain-MDIO-device-and-bus-statistics/20200113-125529
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5c9166f038257d6130865b267cb45d87f126b3dd
config: x86_64-lkp (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_transfers_show':
   drivers/net/phy/mdio_bus.c:305:32: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(transfers, "%llu");
                                   ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
>> drivers/net/phy/mdio_bus.c:305:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(transfers, "%llu");
    ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_errors_show':
   drivers/net/phy/mdio_bus.c:306:29: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(errors, "%llu");
                                ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c:306:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(errors, "%llu");
    ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_writes_show':
   drivers/net/phy/mdio_bus.c:307:29: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(writes, "%llu");
                                ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c:307:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(writes, "%llu");
    ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c: In function 'mdio_bus_stats_reads_show':
   drivers/net/phy/mdio_bus.c:308:28: error: pasting ""%llu"" and ""\n"" does not give a valid preprocessing token
    MDIO_BUS_STATS_SHOW(reads, "%llu");
                               ^
   drivers/net/phy/mdio_bus.c:296:21: note: in definition of macro 'MDIO_BUS_STATS_SHOW_NAME'
     len = sprintf(buf, format_string ## "\n", tmp);   \
                        ^~~~~~~~~~~~~
   drivers/net/phy/mdio_bus.c:308:1: note: in expansion of macro 'MDIO_BUS_STATS_SHOW'
    MDIO_BUS_STATS_SHOW(reads, "%llu");
    ^~~~~~~~~~~~~~~~~~~

vim +305 drivers/net/phy/mdio_bus.c

   300	
   301	#define MDIO_BUS_STATS_SHOW(field, format_string)			\
   302		MDIO_BUS_STATS_SHOW_NAME(field, __stringify(field),		\
   303					      field, format_string)
   304	
 > 305	MDIO_BUS_STATS_SHOW(transfers, "%llu");
   306	MDIO_BUS_STATS_SHOW(errors, "%llu");
   307	MDIO_BUS_STATS_SHOW(writes, "%llu");
   308	MDIO_BUS_STATS_SHOW(reads, "%llu");
   309	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28745 bytes --]

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

end of thread, other threads:[~2020-01-14  4:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13  4:53 [PATCH net-next] net: phy: Maintain MDIO device and bus statistics Florian Fainelli
2020-01-13  4:55 ` Florian Fainelli
2020-01-13 13:21 ` Andrew Lunn
2020-01-13 18:00   ` Florian Fainelli
2020-01-13 18:29     ` Andrew Lunn
2020-01-14  4:44 ` kbuild test robot
2020-01-14  4:44   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.