Message-ID: <1455751697.21571.34.camel@decadent.org.uk>
Date:	Wed, 17 Feb 2016 23:28:17 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Andrew Lunn <andrew@...n.ch>
Cc:	David Miller <davem@...emloft.net>,
	Florian Fainelli <f.fainelli@...il.com>,
	netdev <netdev@...r.kernel.org>
Subject: 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:
> > 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.
> > [...]
> > > --- 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".

> > > +};
> > > +
> > > +/**
> > > + * 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.

[...] 
> > 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.

> > > + * @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.

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?
>  
> > 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.

> > [...]
> > > --- 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.
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)

Powered by blists - more mailing lists