Re: [PATCH net-next] net: phy: Maintain MDIO device and bus statistics

[Florian Fainelli]

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@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxxxxxxx
>> +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@xxxxxxxxxxxxxxx
>> +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