[PATCH net-next 0/2] Add support for PHY packet generators

[PATCH net-next 0/2] Add support for PHY packet generators

[Andrew Lunn]

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

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

[Andrew Lunn]

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(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index d6f3641e7933..f7770c687b4e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -589,6 +589,10 @@ struct phy_driver {
 	void (*get_strings)(struct phy_device *dev, u8 *data);
 	void (*get_stats)(struct phy_device *dev,
 			  struct ethtool_stats *stats, u64 *data);
+
+	/* Make use of the PHY packet generator */
+	int (*pkt_gen)(struct phy_device *dev,
+		       struct ethtool_phy_pkt_gen *pkt_gen);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 190aea0faaf4..bce6c95f458c 100644
--- 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),
+};
+
+/**
+ * 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.
+ * @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.
+ */
+struct ethtool_phy_pkt_gen {
+	__u32	cmd;
+	__u32	count;
+	__u32	len;
+	__u32	ipg;
+	__u32	flags;
+};
+
 /*
  * %ETHTOOL_SFEATURES changes features present in features[].valid to the
  * values of corresponding bits in features[].requested. Bits in .requested
@@ -1284,6 +1309,7 @@ enum ethtool_sfeatures_retval_bits {
 #define ETHTOOL_GTUNABLE	0x00000048 /* Get tunable configuration */
 #define ETHTOOL_STUNABLE	0x00000049 /* Set tunable configuration */
 #define ETHTOOL_GPHYSTATS	0x0000004a /* get PHY-specific statistics */
+#define ETHTOOL_PHY_PKT_GEN	0x0000004b /* Gnerate packets in the PHY */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c2d3118b1395..d5e8cd6a26e9 100644
--- 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;
+
+	if (copy_from_user(&pkt_gen, useraddr, sizeof(pkt_gen)))
+		return -EFAULT;
+
+	mutex_lock(&phydev->lock);
+	err = phydev->drv->pkt_gen(phydev, &pkt_gen);
+	mutex_unlock(&phydev->lock);
+
+	return err;
+}
+
 static int ethtool_get_perm_addr(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_perm_addr epaddr;
@@ -2135,6 +2154,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GPHYSTATS:
 		rc = ethtool_get_phy_stats(dev, useraddr);
 		break;
+	case ETHTOOL_PHY_PKT_GEN:
+		rc = ethtool_phy_pkt_gen(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.7.0

[PATCH net-next 2/2] phy: marvell: Add support for phy packet generator

[Andrew Lunn]

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(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index e3eb96443c97..1c42aef6dcf3 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -133,6 +133,15 @@
 #define MII_88E3016_DISABLE_SCRAMBLER	0x0200
 #define MII_88E3016_AUTO_MDIX_CROSSOVER	0x0030
 
+#define MII_88E1540_PKT_GEN		16
+#define MII_88E1540_PKT_GEN_BURST_SHIFT	8
+#define MII_88E1540_PKT_GEN_ENABLE	BIT(3)
+#define MII_88E1540_PKT_GEN_FIXED	BIT(2)
+#define MII_88E1540_PKT_GEN_LEN_1518	BIT(1)
+#define MII_88E1540_PKT_GEN_ERROR	BIT(0)
+
+#define MII_88E1540_PKT_GEN_IPG		19
+
 MODULE_DESCRIPTION("Marvell PHY driver");
 MODULE_AUTHOR("Andy Fleming");
 MODULE_LICENSE("GPL");
@@ -1057,6 +1066,83 @@ static void marvell_get_stats(struct phy_device *phydev,
 		data[i] = marvell_get_stat(phydev, i);
 }
 
+static int marvell_pkt_gen(struct phy_device *phydev,
+			   struct ethtool_phy_pkt_gen *pkt_gen)
+{
+	int err, oldpage, reg, max_loop = 100;
+	u32 phy_id = phydev->drv->phy_id;
+	bool has_ipg = false;
+
+	switch (phy_id) {
+	case MARVELL_PHY_ID_88E1510:
+	case MARVELL_PHY_ID_88E1540:
+		has_ipg = true;
+	}
+
+	if (pkt_gen->count < 1 || pkt_gen->count > 255)
+		return -EINVAL;
+
+	if (pkt_gen->len == 0)
+		pkt_gen->len = 64;
+
+	if (pkt_gen->len != 64 && pkt_gen->len != 1518)
+		return -EINVAL;
+
+	if (has_ipg) {
+		if (pkt_gen->ipg == 0)
+			pkt_gen->ipg = 12;
+
+		if (pkt_gen->ipg > 256)
+			return -EINVAL;
+	} else {
+		if (pkt_gen->ipg != 0)
+			return -EINVAL;
+	}
+
+	oldpage = phy_read(phydev, MII_MARVELL_PHY_PAGE);
+	if (oldpage < 0) {
+		err = oldpage;
+		goto out;
+	}
+
+	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, 6);
+	if (err)
+		goto out;
+
+	if (has_ipg) {
+		err = phy_write(phydev, MII_88E1540_PKT_GEN_IPG,
+				pkt_gen->ipg - 1);
+		if (err)
+			goto out;
+	}
+
+	reg = MII_88E1540_PKT_GEN_ENABLE;
+	reg |= pkt_gen->count << MII_88E1540_PKT_GEN_BURST_SHIFT;
+	if (pkt_gen->len == 1518)
+		reg |= MII_88E1540_PKT_GEN_LEN_1518;
+	if (!(pkt_gen->flags & ETH_PKT_RANDOM))
+		reg |= MII_88E1540_PKT_GEN_FIXED;
+	if (pkt_gen->flags & ETH_PKT_ERROR)
+		reg |= MII_88E1540_PKT_GEN_ERROR;
+
+	err = phy_write(phydev, MII_88E1540_PKT_GEN, reg);
+	if (err)
+		goto out;
+
+	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;
+
+out:
+	phy_write(phydev, MII_MARVELL_PHY_PAGE, oldpage);
+
+	return err;
+}
+
 static int marvell_probe(struct phy_device *phydev)
 {
 	struct marvell_priv *priv;
@@ -1102,6 +1188,7 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.pkt_gen = marvell_pkt_gen,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -1156,6 +1243,7 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.pkt_gen = marvell_pkt_gen,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -1212,6 +1300,7 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.pkt_gen = marvell_pkt_gen,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -1248,6 +1337,7 @@ static struct phy_driver marvell_drivers[] = {
 		.config_intr = &marvell_config_intr,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.pkt_gen = marvell_pkt_gen,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -1284,6 +1374,7 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.pkt_gen = marvell_pkt_gen,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
@@ -1304,6 +1395,7 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.resume = &genphy_resume,
 		.suspend = &genphy_suspend,
+		.pkt_gen = marvell_pkt_gen,
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
-- 
2.7.0

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2424 bytes --]

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?

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

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

Ben.

-- 
Ben Hutchings
Lowery's Law:
             If it jams, force it. If it breaks, it needed replacing anyway.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Andrew Lunn]

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

Re: [PATCH net-next 2/2] phy: marvell: Add support for phy packet generator

[Lino Sanfilippo]

Hi,

On 17.02.2016 21:32, Andrew Lunn wrote:
> +
> +	oldpage = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> +	if (oldpage < 0) {
> +		err = oldpage;
> +		goto out;
> +	}
> +

shouldn't this return immediately? Jump to out label and writing an
error value to the phy does not seem to be correct.

Regards,
Lino

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net-next 2/2] phy: marvell: Add support for phy packet generator

[Florian Fainelli]

On 17/02/2016 12:32, Andrew Lunn wrote:
> +static int marvell_pkt_gen(struct phy_device *phydev,
> +			   struct ethtool_phy_pkt_gen *pkt_gen)
> +{
> +	int err, oldpage, reg, max_loop = 100;
> +	u32 phy_id = phydev->drv->phy_id;
> +	bool has_ipg = false;

[snip]


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

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

[David Miller]

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.