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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160217215533.GC25818@lunn.ch>
Date:	Wed, 17 Feb 2016 22:55:33 +0100
From:	Andrew Lunn <andrew@...n.ch>
To:	Ben Hutchings <ben@...adent.org.uk>
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, 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.

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

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

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

	Andrew