Patchwork [2/2,net-next] gianfar: Add ethtool -A support for pause frame

login
register
mail settings
Submitter Claudiu Manoil
Date Aug. 7, 2013, 10:24 a.m.
Message ID <1375871056-10420-2-git-send-email-claudiu.manoil@freescale.com>
Download mbox | patch
Permalink /patch/265464/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Claudiu Manoil - Aug. 7, 2013, 10:24 a.m.
Allow Rx/Tx pause frame configuration via ethtool.
The gfar devices feature link autonegotioation by default.
The device is being configured with the new pause frame
parameters if the link is up, depending on link duplex (no
pause frames for half-duplex links), or during link autoneg
(see adjust_link()).

Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
 drivers/net/ethernet/freescale/gianfar.h         |  1 +
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 30 ++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
Ben Hutchings - Aug. 7, 2013, 7:12 p.m.
On Wed, 2013-08-07 at 13:24 +0300, Claudiu Manoil wrote:
> Allow Rx/Tx pause frame configuration via ethtool.
> The gfar devices feature link autonegotioation by default.

So the MAC configuration bits are actually copied to the PHY autoneg
basic page, and then the PHY autoneg result is automatically used by the
MAC?

This is of course possible to do in hardware, but... since this MAC is
not smart enough to ignore pause settings when running in half-duplex
mode, I seriously doubt it is doing all this by itself.

> The device is being configured with the new pause frame
> parameters if the link is up, depending on link duplex (no
> pause frames for half-duplex links), or during link autoneg
> (see adjust_link()).
[...]
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -535,6 +535,34 @@ static int gfar_sringparam(struct net_device *dev,
>  	return err;
>  }
>  
> +static void gfar_gpauseparam(struct net_device *dev,
> +			     struct ethtool_pauseparam *pause)
> +{
> +	struct gfar_private *priv = netdev_priv(dev);
> +
> +	pause->autoneg = AUTONEG_ENABLE;
> +	if (priv->rx_pause)
> +		pause->rx_pause = 1;
> +	if (priv->tx_pause)
> +		pause->tx_pause = 1;
> +}
> +
> +static int gfar_spauseparam(struct net_device *dev,
> +			    struct ethtool_pauseparam *pause)
> +{
> +	struct gfar_private *priv = netdev_priv(dev);
> +	struct phy_device *phydev = priv->phydev;

You need to reject an unsupported setting of pause->autoneg here.

Ben.

> +	priv->rx_pause = !!pause->rx_pause;
> +	priv->tx_pause = !!pause->tx_pause;
> +
> +	/* update h/w settings, if link is up */
> +	if (phydev && phydev->link)
> +		gfar_configure_pause(priv, !!phydev->duplex);
> +
> +	return 0;
> +}
> +
>  int gfar_set_features(struct net_device *dev, netdev_features_t features)
>  {
>  	struct gfar_private *priv = netdev_priv(dev);
[...]
Claudiu Manoil - Aug. 8, 2013, 5:10 p.m.
Hi Ben,

On 8/7/2013 10:12 PM, Ben Hutchings wrote:
> On Wed, 2013-08-07 at 13:24 +0300, Claudiu Manoil wrote:
>> Allow Rx/Tx pause frame configuration via ethtool.
>> The gfar devices feature link autonegotioation by default.
>
> So the MAC configuration bits are actually copied to the PHY autoneg
> basic page, and then the PHY autoneg result is automatically used by the
> MAC?
>
> This is of course possible to do in hardware, but... since this MAC is
> not smart enough to ignore pause settings when running in half-duplex
> mode, I seriously doubt it is doing all this by itself.
>

I just wanted to say actually that the pause->autoneg parameter is not
needed by the gianfar driver, but I didn't know what to do with it in
get_pauseparam(), apparently pause->autoneg needs a value (or can
simply ignore this param?).

I don't see what autonegotiation has to do with enabling/disabling
pause frame generation in this case. My understanding is that link
autonegotiation is taken care somewhere else, by the phy state machine.
Each time this happens, the gianfar driver gets notified via the
adjust_link() hook that it implements and makes the necessary configs
in the mac registers.

Besides, autoneg info is already being displayed by ethtool (see
print below).
So I don't understand the use of pause->autoneg. What should I do with
it?

Thanks,
Claudiu
Ben Hutchings - Aug. 8, 2013, 6:45 p.m.
On Thu, 2013-08-08 at 20:10 +0300, Claudiu Manoil wrote:
> Hi Ben,
> 
> On 8/7/2013 10:12 PM, Ben Hutchings wrote:
> > On Wed, 2013-08-07 at 13:24 +0300, Claudiu Manoil wrote:
> >> Allow Rx/Tx pause frame configuration via ethtool.
> >> The gfar devices feature link autonegotioation by default.
> >
> > So the MAC configuration bits are actually copied to the PHY autoneg
> > basic page, and then the PHY autoneg result is automatically used by the
> > MAC?
> >
> > This is of course possible to do in hardware, but... since this MAC is
> > not smart enough to ignore pause settings when running in half-duplex
> > mode, I seriously doubt it is doing all this by itself.
> >
> 
> I just wanted to say actually that the pause->autoneg parameter is not
> needed by the gianfar driver, but I didn't know what to do with it in
> get_pauseparam(), apparently pause->autoneg needs a value (or can
> simply ignore this param?).
> 
> I don't see what autonegotiation has to do with enabling/disabling
> pause frame generation in this case. My understanding is that link
> autonegotiation is taken care somewhere else, by the phy state machine.
> Each time this happens, the gianfar driver gets notified via the
> adjust_link() hook that it implements and makes the necessary configs
> in the mac registers.

That's what it *should* be doing.  But it's not; it's using
priv->rx_pause and priv->tx_pause, not phydev->pause and
phydev->asym_pause.  I.e. pause autoneg is really always *disabled*.

> Besides, autoneg info is already being displayed by ethtool (see
> print below).
> So I don't understand the use of pause->autoneg. What should I do with
> it?
[...]

Pause frame generation may be forced even though autonegotiation is
enabled.  I don't think this is a good idea but many drivers do support
it.

And gianfar does appear to allow autonegotiation to be disabled, anyway,
so in any case you have to support both forced and autonegotiated cases.

Ben.
Lutz Jaenicke - Aug. 9, 2013, 8:26 a.m.
I have been working on the same issue and came up with a similar
modification (that additionally implements LFC if supported)
I have attached the two patches created:
* gianfar: implement flow control handling
  which looks quite similar to Claudiu's patches
* gianfar: add support for LFC (Lossless Flow Control)
  which is more or less just extracted from some patch I found on the internet
    linux-2.6.29.6-mpc8308e_rdb.patch
The patches I have taken from a custom 3.4 based kernel so they may not
apply cleanly but maybe bits of it can be used to generate fully supported
solution.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Claudiu Manoil - Aug. 9, 2013, 10:12 a.m.
Thanks,

First patch seems to include the complete implementation of ethtool -A,
fixing the pause issue for half-duplex links as well, and it also
implements pause autoneg. (nice)
I'm going to apply an test this code and re-spin the resulting
patch(es).
Are you the author of this patch?
(gianfar: implement flow control handling)

Claudiu

On 8/9/2013 11:26 AM, Lutz Jaenicke wrote:
> I have been working on the same issue and came up with a similar
> modification (that additionally implements LFC if supported)
> I have attached the two patches created:
> * gianfar: implement flow control handling
>    which looks quite similar to Claudiu's patches
> * gianfar: add support for LFC (Lossless Flow Control)
>    which is more or less just extracted from some patch I found on the internet
>      linux-2.6.29.6-mpc8308e_rdb.patch
> The patches I have taken from a custom 3.4 based kernel so they may not
> apply cleanly but maybe bits of it can be used to generate fully supported
> solution.
>
>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lutz Jaenicke - Aug. 9, 2013, 10:15 a.m.
On Fri, Aug 09, 2013 at 01:12:15PM +0300, Claudiu Manoil wrote:
> Thanks,
> 
> First patch seems to include the complete implementation of ethtool -A,
> fixing the pause issue for half-duplex links as well, and it also
> implements pause autoneg. (nice)
> I'm going to apply an test this code and re-spin the resulting
> patch(es).
> Are you the author of this patch?
> (gianfar: implement flow control handling)

Yes.

Best regards,
	Lutz

Patch

diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index e6a03f4..aaac843 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -1186,6 +1186,7 @@  void gfar_init_sysfs(struct net_device *dev);
 int gfar_set_features(struct net_device *dev, netdev_features_t features);
 extern void gfar_check_rx_parser_mode(struct gfar_private *priv);
 extern void gfar_vlan_mode(struct net_device *dev, netdev_features_t features);
+void gfar_configure_pause(struct gfar_private *priv, bool en);
 
 extern const struct ethtool_ops gfar_ethtool_ops;
 
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 21cd881..6cf89c1 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -535,6 +535,34 @@  static int gfar_sringparam(struct net_device *dev,
 	return err;
 }
 
+static void gfar_gpauseparam(struct net_device *dev,
+			     struct ethtool_pauseparam *pause)
+{
+	struct gfar_private *priv = netdev_priv(dev);
+
+	pause->autoneg = AUTONEG_ENABLE;
+	if (priv->rx_pause)
+		pause->rx_pause = 1;
+	if (priv->tx_pause)
+		pause->tx_pause = 1;
+}
+
+static int gfar_spauseparam(struct net_device *dev,
+			    struct ethtool_pauseparam *pause)
+{
+	struct gfar_private *priv = netdev_priv(dev);
+	struct phy_device *phydev = priv->phydev;
+
+	priv->rx_pause = !!pause->rx_pause;
+	priv->tx_pause = !!pause->tx_pause;
+
+	/* update h/w settings, if link is up */
+	if (phydev && phydev->link)
+		gfar_configure_pause(priv, !!phydev->duplex);
+
+	return 0;
+}
+
 int gfar_set_features(struct net_device *dev, netdev_features_t features)
 {
 	struct gfar_private *priv = netdev_priv(dev);
@@ -1806,6 +1834,8 @@  const struct ethtool_ops gfar_ethtool_ops = {
 	.set_coalesce = gfar_scoalesce,
 	.get_ringparam = gfar_gringparam,
 	.set_ringparam = gfar_sringparam,
+	.get_pauseparam = gfar_gpauseparam,
+	.set_pauseparam = gfar_spauseparam,
 	.get_strings = gfar_gstrings,
 	.get_sset_count = gfar_sset_count,
 	.get_ethtool_stats = gfar_fill_stats,