From: Andrew Lunn <andrew@lunn.ch> Date: 2016-02-17 20:32:30
Some Ethernet PHYs contain a simple packet generator. This can be
useful for bringing up new devices, trying to determine if a problem
lies in the MAC-PHY connection or PHY-Socket. Also, the PHY generators
can generate invalid packets, which is hard to do in software.
Add support ethtool(1) and wire up the Marvell PHY packet generator.
Andrew Lunn (2):
net: ethtool: Add support for PHY packet generators
phy: marvell: Add support for phy packet generator
drivers/net/phy/marvell.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 4 ++
include/uapi/linux/ethtool.h | 26 +++++++++++++
net/core/ethtool.c | 22 +++++++++++
4 files changed, 144 insertions(+)
--
2.7.0
From: Andrew Lunn <andrew@lunn.ch> Date: 2016-02-17 20:32:14
Some PHY devices contain a simple packet generator. Features vary, but
often they can be used to generate packets of different sizes,
different contents, with or without errors, and with different inter
packet gaps. Add support to the core ethtool code to support this.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
include/linux/phy.h | 4 ++++
include/uapi/linux/ethtool.h | 26 ++++++++++++++++++++++++++
net/core/ethtool.c | 22 ++++++++++++++++++++++
3 files changed, 52 insertions(+)
@@ -589,6 +589,10 @@ struct phy_driver {void(*get_strings)(structphy_device*dev,u8*data);void(*get_stats)(structphy_device*dev,structethtool_stats*stats,u64*data);++/* Make use of the PHY packet generator */+int(*pkt_gen)(structphy_device*dev,+structethtool_phy_pkt_gen*pkt_gen);};#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \structphy_driver,mdiodrv)
From: Andrew Lunn <andrew@lunn.ch> Date: 2016-02-17 20:32:22
Most of the Marvell PHYs include a packet generator, for sending up to
255 packets, of size 64 or 1518 bytes. Packets can either contain
repeated 0x5a 0xa5, or random bytes. Packets can contain CRC errors or
symbol errors. Some PHYs allow the inter packet gap to be changed.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/phy/marvell.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
From: Ben Hutchings <hidden> Date: 2016-02-17 21:06:39
On Wed, 2016-02-17 at 21:32 +0100, Andrew Lunn wrote:
Some PHY devices contain a simple packet generator. Features vary, but
often they can be used to generate packets of different sizes,
different contents, with or without errors, and with different inter
packet gaps. Add support to the core ethtool code to support this.
What kind of error? CRC error, FEC error, symbol error?
+};
+
+/**
+ * struct ethtool_phy_pkt_get - command to request the phy to generate packets.
+ * @cmd: command number = %ETHTOOL_PHY_PKT_GEN
+ * @count: number of packets to generate
+ * @len: length of generated packets
+ * @ipg: inter packet gap in bytes.
What if the PHY doesn't allow varying the IPG? Should there be a way
to find out what its supported IPG is, or to request the default value?
Similarly, should there be a way to find out the minimum/maximum length it supports?
+ * @flags: a bitmask of flags from &enum ethtool_phy_pkg_gen_flags
+ *
+ * PHY drivers may not support all of these parameters. If the
+ * requested parameter value cannot be supported an error should be
+ * returned.
Should, or must?
How does userland tell when the PHY has finished? Should it be
possible to cancel this (similar to ETHTOOL_PHYS_ID)?
What should happen if the stack tries to send a packet while the PHY is
in this mode? Is it discarded? Should the driver indicate carrier-off
so that this is obvious?
[...]
[...]
Why should this be tied to phylib? Nothing else in the ethtool
interface is.
Ben.
--
Ben Hutchings
Lowery's Law:
If it jams, force it. If it breaks, it needed replacing anyway.
From: Andrew Lunn <andrew@lunn.ch> Date: 2016-02-17 21:55:38
On Wed, Feb 17, 2016 at 09:06:19PM +0000, Ben Hutchings wrote:
On Wed, 2016-02-17 at 21:32 +0100, Andrew Lunn wrote:
quoted
Some PHY devices contain a simple packet generator. Features vary, but
often they can be used to generate packets of different sizes,
different contents, with or without errors, and with different inter
packet gaps. Add support to the core ethtool code to support this.
What kind of error? CRC error, FEC error, symbol error?
Hi Ben
I want to try to keep the API generic, since different PHYs are
different capabilities. The Marvell phy will generate symbol errors
and CRC errors. You cannot control it in a finer way than that.
quoted
+};
+
+/**
+ * struct ethtool_phy_pkt_get - command to request the phy to generate packets.
+ * @cmd: command number = %ETHTOOL_PHY_PKT_GEN
+ * @count: number of packets to generate
+ * @len: length of generated packets
+ * @ipg: inter packet gap in bytes.
What if the PHY doesn't allow varying the IPG? Should there be a way
to find out what its supported IPG is, or to request the default value?
If you pass 0, it will use the default IPG. If you pass a value other
than 0, and it is not supported, it return -EINVAL.
For the Marvell PHYs some don't support setting the IPG, it is hard
set to 12. On those phys passing anything other than 0 gives
-EINVAL. When an IPG is allowed, a value of 0 gives the default 12,
since 0 is invalid, and a value > 256 also gives -EINVAL, since that
is the limit imposed by the hardware.
Similarly, should there be a way to find out the minimum/maximum length it supports?
I'm trying to keep it simple. Do we really want to add a complex
mechanism to query every available parameter to determine the range of
values it can take? I find it better that the driver accepts the
values of 0 meaning pick a sensible default, and for any value not 0,
return an error if it cannot be supported by the hardware.
I tried to emphasise this in the man page patch.
quoted
+ * @flags: a bitmask of flags from &enum ethtool_phy_pkg_gen_flags
+ *
+ * PHY drivers may not support all of these parameters. If the
+ * requested parameter value cannot be supported an error should be
+ * returned.
Should, or must?
Must would be better.
How does userland tell when the PHY has finished? Should it be
possible to cancel this (similar to ETHTOOL_PHYS_ID)?
The call is blocking and returns when all the packets are sent.
For the Marvell hardware, you can send up to 255 packets. At 10Mbps,
1518 byte packets and 256 it takes about 0.3 seconds.
I don't really like the idea of making it non-blocking. i.e. set it
generating packets and sometime later stop it. It can lead to some
very non-obvious issues. Why is my Ethernet card spamming the net at
line rate, yet the netdev TX counters are not going up?
What should happen if the stack tries to send a packet while the PHY is
in this mode? Is it discarded? Should the driver indicate carrier-off
so that this is obvious?
Depends on the hardware. Marvell PHYs will discard any packets coming
from the MAC.
[...]
Why should this be tied to phylib? Nothing else in the ethtool
interface is.
We need the phy lock to be held. We don't want anything else accessing
phy registers at the same time. For the Marvell hardware, we need to
change the page. If for example genphy_read_status() was used to poll
the status of the PHY, it could read from the wrong page and get very
confused.
Andrew
From: Ben Hutchings <hidden> Date: 2016-02-17 23:28:31
On Wed, 2016-02-17 at 22:55 +0100, Andrew Lunn wrote:
On Wed, Feb 17, 2016 at 09:06:19PM +0000, Ben Hutchings wrote:
quoted
On Wed, 2016-02-17 at 21:32 +0100, Andrew Lunn wrote:
quoted
Some PHY devices contain a simple packet generator. Features vary, but
often they can be used to generate packets of different sizes,
different contents, with or without errors, and with different inter
packet gaps. Add support to the core ethtool code to support this.
What kind of error? CRC error, FEC error, symbol error?
Hi Ben
I want to try to keep the API generic, since different PHYs are
different capabilities. The Marvell phy will generate symbol errors
and CRC errors. You cannot control it in a finer way than that.
Sure. But this should be commented, something like "all packets have
layer 1 and/or layer 2 errors".
Similarly the random flag should be commented as something like
"randomise packet header and payload".
quoted
quoted
+};
+
+/**
+ * struct ethtool_phy_pkt_get - command to request the phy to generate packets.
+ * @cmd: command number = %ETHTOOL_PHY_PKT_GEN
+ * @count: number of packets to generate
+ * @len: length of generated packets
+ * @ipg: inter packet gap in bytes.
What if the PHY doesn't allow varying the IPG? Should there be a way
to find out what its supported IPG is, or to request the default value?
If you pass 0, it will use the default IPG. If you pass a value other
than 0, and it is not supported, it return -EINVAL.
Include that in the comment.
[...]
quoted
Similarly, should there be a way to find out the minimum/maximum length it supports?
I'm trying to keep it simple. Do we really want to add a complex
mechanism to query every available parameter to determine the range of
values it can take?
That is an important part of making this feature generic.
I find it better that the driver accepts the
values of 0 meaning pick a sensible default, and for any value not 0,
return an error if it cannot be supported by the hardware.
I tried to emphasise this in the man page patch.
The ethtool API is not a private API for the ethtool utility, despite
its name. The API must be documented in ethtool.h.
quoted
quoted
+ * @flags: a bitmask of flags from &enum ethtool_phy_pkg_gen_flags
+ *
+ * PHY drivers may not support all of these parameters. If the
+ * requested parameter value cannot be supported an error should be
+ * returned.
Should, or must?
Must would be better.
quoted
How does userland tell when the PHY has finished? Should it be
possible to cancel this (similar to ETHTOOL_PHYS_ID)?
The call is blocking and returns when all the packets are sent.
For the Marvell hardware, you can send up to 255 packets. At 10Mbps,
1518 byte packets and 256 it takes about 0.3 seconds.
OK, then it needs to drop the RTNL lock and the ethtool core should
probably call into the driver multiple times, similarly to
ETHTOOL_PHYS_ID..
I don't really like the idea of making it non-blocking. i.e. set it
generating packets and sometime later stop it. It can lead to some
very non-obvious issues. Why is my Ethernet card spamming the net at
line rate, yet the netdev TX counters are not going up?
quoted
What should happen if the stack tries to send a packet while the PHY is
in this mode? Is it discarded? Should the driver indicate carrier-off
so that this is obvious?
Depends on the hardware. Marvell PHYs will discard any packets coming
from the MAC.
I know that the PHY behaviour varies but the API should be consistent
across drivers and the drivers will have to do a little work to ensure
that.
[...]
Why should this be tied to phylib? Nothing else in the ethtool
interface is.
We need the phy lock to be held. We don't want anything else accessing
phy registers at the same time. For the Marvell hardware, we need to
change the page. If for example genphy_read_status() was used to poll
the status of the PHY, it could read from the wrong page and get very
confused.
So what are net drivers that don't use phylib supposed to do to support this?
Ben.
--
Ben Hutchings
Lowery's Law:
If it jams, force it. If it breaks, it needed replacing anyway.
+
+ do {
+ usleep_range(3000, 4000);
+ reg = phy_read(phydev, MII_88E1540_PKT_GEN);
+ } while (max_loop-- && (reg & MII_88E1540_PKT_GEN_ENABLE));
+
+ if (!max_loop)
+ err = -ETIMEDOUT;
If I am a HW engineer trying to qualify a PHY after enabling the random
packet generator built into it, I might prefer a "start generation" and
"stop generation" (this echoes back to Ben's comments on the ethtool
API) as opposed to calling the same function multiple times, because the
duration will vary based on link speed, and potentially models of PHYs too.
From: David Miller <davem@davemloft.net> Date: 2016-02-18 20:45:41
From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 17 Feb 2016 21:32:05 +0100
Some Ethernet PHYs contain a simple packet generator. This can be
useful for bringing up new devices, trying to determine if a problem
lies in the MAC-PHY connection or PHY-Socket. Also, the PHY generators
can generate invalid packets, which is hard to do in software.
Add support ethtool(1) and wire up the Marvell PHY packet generator.
You really cannot make this blocking, every time we've added a blocking
ethtool op that could take a non-trivial amount of time we've been
burnt.
So as Ben mentioned blocking for 0.3 seconds or whatever is a non-starter.