Re: [PATCH net-next 1/2] net: ethtool: Add support for PHY packet generators

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.

[...]

quoted

--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h

@@ -1167,6 +1167,31 @@ struct ethtool_ts_info {

 	__u32	rx_reserved[3];
 };
 
+enum ethtool_phy_pkg_gen_flags {
+	ETH_PKT_RANDOM	= (1 << 0),
+	ETH_PKT_ERROR	= (1 << 1),

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.

quoted

[...]

quoted

--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c

@@ -1541,6 +1541,25 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)

 	return ret;
 }
 
+static int ethtool_phy_pkt_gen(struct net_device *dev, void __user *useraddr)
+{
+	struct phy_device *phydev = dev->phydev;
+	struct ethtool_phy_pkt_gen pkt_gen;
+	int err;
+
+	if (!phydev || !phydev->drv->pkt_gen)
+		return -EOPNOTSUPP;

[...]

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.