Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Simon Horman]

[CC Sergei, linux-renesas-soc]

On Tue, Sep 18, 2018 at 03:02:36PM +0200, Andrew Lunn wrote:
> > Hi Andrew,
> 
> Hi Simon
> 
> Thanks for the dumps
>  
> > 1. net-next: cf7d97e1e54d ("net: mdio: remove duplicated include from mdio_bus.c")
> > 
> >   basic status: no link
> >   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> >   advertising:  100baseTx-FD 100baseTx-HD flow-control
> >   link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > 
> > 2. net-next with this patch reverted
> > 
> >   basic status: autonegotiation complete, link ok
> >   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> >   advertising:  100baseTx-FD 100baseTx-HD
> >   link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> 
> So flow-control is not present here.
> 
> >   basic status: autonegotiation complete, link ok
> >   capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> >   advertising:  100baseTx-FD 100baseTx-HD
> >   link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> 
> And here also.

Thanks for raising this, I noticed it too.

> Looking at the code, i see:
> 
> /* E-MAC init function */
> static void ravb_emac_init(struct net_device *ndev)
> {
>         struct ravb_private *priv = netdev_priv(ndev);
> 
>         /* Receive frame limit set register */
>         ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> 
>         /* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
>         ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
>                    (ndev->features & NETIF_F_RXCSUM ? ECMR_RCSC : 0) |
>                    ECMR_TE | ECMR_RE, ECMR);
> 
> Does this mean Pause is not supported in the hardware?

According to my reading of the documentation Pause is supported by the
hardware and the above code seems to conflict with the comment (possibly
both the code and comment predate the current documentation). My reading of
the documentation is that the above unconditionally _enables_ receiving and
sending Pause frames with time parameter value 0.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Andrew Lunn]

> > And here also.
> 
> Thanks for raising this, I noticed it too.
> 
> > Looking at the code, i see:
> > 
> > /* E-MAC init function */
> > static void ravb_emac_init(struct net_device *ndev)
> > {
> >         struct ravb_private *priv = netdev_priv(ndev);
> > 
> >         /* Receive frame limit set register */
> >         ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> > 
> >         /* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
> >         ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
> >                    (ndev->features & NETIF_F_RXCSUM ? ECMR_RCSC : 0) |
> >                    ECMR_TE | ECMR_RE, ECMR);
> > 
> > Does this mean Pause is not supported in the hardware?
> 
> According to my reading of the documentation Pause is supported by the
> hardware and the above code seems to conflict with the comment (possibly
> both the code and comment predate the current documentation). My reading of
> the documentation is that the above unconditionally _enables_ receiving and
> sending Pause frames with time parameter value 0.

Hi Simon

We should first prove that this additional Pause is causing the
issue. After that, we can decide if we want to add Pause support to
the driver. Please could you test this patch.

Thanks
	Andrew

>From 0f69f4991454d48f34b05d5dc006c04a180c7842 Mon Sep 17 00:00:00 2001
From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 18 Sep 2018 18:12:54 -0500
Subject: [PATCH] ravb: Disable Pause Advertisement

The previous commit to ravb had the side effect of making the PHY
advertise Pause. This previously did not happen, and it appears the
MAC does not support Pause. By default, phydev->supported has Pause
enabled, but phydev->advertising does not. Rather than rely on this,
be explicit, and remove the Pause link mode.

Reported-by: Simon Horman <horms@verge.net.au>
Fixes: 41124fa64d4b ("net: ethernet: Add helper to remove a supported link mode")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/renesas/ravb_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index fb2a1125780d..d7630c0fdb0a 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1073,9 +1073,10 @@ static int ravb_phy_init(struct net_device *ndev)
 		netdev_info(ndev, "limited PHY to 100Mbit/s\n");
 	}
 
-	/* 10BASE is not supported */
+	/* 10BASE and Pause is not supported */
 	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
 	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
+	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_Pause_BIT);
 
 	phy_attached_info(phydev);
 
-- 
2.19.0.rc1

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Simon Horman]

On Wed, Sep 19, 2018 at 02:32:00PM +0200, Andrew Lunn wrote:
> > > And here also.
> > 
> > Thanks for raising this, I noticed it too.
> > 
> > > Looking at the code, i see:
> > > 
> > > /* E-MAC init function */
> > > static void ravb_emac_init(struct net_device *ndev)
> > > {
> > >         struct ravb_private *priv = netdev_priv(ndev);
> > > 
> > >         /* Receive frame limit set register */
> > >         ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> > > 
> > >         /* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
> > >         ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
> > >                    (ndev->features & NETIF_F_RXCSUM ? ECMR_RCSC : 0) |
> > >                    ECMR_TE | ECMR_RE, ECMR);
> > > 
> > > Does this mean Pause is not supported in the hardware?
> > 
> > According to my reading of the documentation Pause is supported by the
> > hardware and the above code seems to conflict with the comment (possibly
> > both the code and comment predate the current documentation). My reading of
> > the documentation is that the above unconditionally _enables_ receiving and
> > sending Pause frames with time parameter value 0.
> 
> Hi Simon
> 
> We should first prove that this additional Pause is causing the
> issue. After that, we can decide if we want to add Pause support to
> the driver. Please could you test this patch.

Hi Andrew,

thanks for your patch. I agree with the approach you have taken here,
however, unfortunately this patch does not seem to resolve the problem that
I have observed.

With this patch on top of net-next ([1] & [2]) I see:

[1] net-next as of two days ago, the version used when reporting
    results earlier in this thread
    cf7d97e1e54d ("net: mdio: remove duplicated include from mdio_bus.c")

# mii-tool -vv eth0
Using SIOCGMIIPHY=0x8947
eth0: no link
  registers for MII PHY 0: 
    1140 7949 0022 1622 0981 c1e1 000d 0000
    0000 0300 0000 0000 0000 0000 0000 3000
    0000 0000 0000 0000 7002 0000 0000 0200
    0000 0000 0000 0500 0000 0000 0000 0000
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: no link
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD
  link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

[2] net-next as of this morning
    faa08325b429 ("isdn/hisax: Remove unnecessary parenthesis")

# mii-tool -vv eth0
Using SIOCGMIIPHY=0x8947
eth0: no link
  registers for MII PHY 0: 
    1140 7949 0022 1622 0981 c1e1 000d 0000
    0000 0300 0000 0000 0000 0000 0000 3000
    0000 0000 0000 0000 6002 0000 0000 0200
    0000 0000 0000 0500 0000 0000 0000 0000
  product info: vendor 00:08:85, model 34 rev 2
  basic mode:   autonegotiation enabled
  basic status: no link
  capabilities: 1000baseT-HD 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
  advertising:  100baseTx-FD 100baseTx-HD
  link partner: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD

I note a difference in the 3rd line of hex output: 7002 vs 6002
but I am unsure if that is relevant.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Andrew Lunn]

> eth0: no link
>   registers for MII PHY 0: 
>     1140 7949 0022 1622 0981 c1e1 000d 0000

Hi Simon

The ID registers 0022 1622 indicate this is a Micrel KSZ9031.
Are you using the micrel PHY driver?

> I note a difference in the 3rd line of hex output: 7002 vs 6002
> but I am unsure if that is relevant.

Register 20, or 0x14. The datasheet says "Reserved" and there is no
description given :-(

I will decode the other registers and see if i can find anything.

    Andrew

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Simon Horman]

On Thu, Sep 20, 2018 at 02:51:06PM +0200, Andrew Lunn wrote:
> > eth0: no link
> >   registers for MII PHY 0: 
> >     1140 7949 0022 1622 0981 c1e1 000d 0000
> 
> Hi Simon
> 
> The ID registers 0022 1622 indicate this is a Micrel KSZ9031.
> Are you using the micrel PHY driver?


Yes, when the Link is successfully negotiated I see:

Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)

> > I note a difference in the 3rd line of hex output: 7002 vs 6002
> > but I am unsure if that is relevant.
> 
> Register 20, or 0x14. The datasheet says "Reserved" and there is no
> description given :-(
> 
> I will decode the other registers and see if i can find anything.

Thanks, very much appreciated.

I believe your patch to disable Asym_Pause solves the immediate
problem I have observed.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Simon Horman]

On Fri, Sep 21, 2018 at 10:17:35AM +0200, Simon Horman wrote:
> On Thu, Sep 20, 2018 at 02:51:06PM +0200, Andrew Lunn wrote:
> > > eth0: no link
> > >   registers for MII PHY 0: 
> > >     1140 7949 0022 1622 0981 c1e1 000d 0000
> > 
> > Hi Simon
> > 
> > The ID registers 0022 1622 indicate this is a Micrel KSZ9031.
> > Are you using the micrel PHY driver?
> 
> 
> Yes, when the Link is successfully negotiated I see:
> 
> Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)
> 
> > > I note a difference in the 3rd line of hex output: 7002 vs 6002
> > > but I am unsure if that is relevant.
> > 
> > Register 20, or 0x14. The datasheet says "Reserved" and there is no
> > description given :-(
> > 
> > I will decode the other registers and see if i can find anything.
> 
> Thanks, very much appreciated.
> 
> I believe your patch to disable Asym_Pause solves the immediate
> problem I have observed.

Andrew, how would you like to resolve this?
Let me know how I can help.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Andrew Lunn]

On Mon, Sep 24, 2018 at 05:36:00PM +0200, Simon Horman wrote:
> On Fri, Sep 21, 2018 at 10:17:35AM +0200, Simon Horman wrote:
> > On Thu, Sep 20, 2018 at 02:51:06PM +0200, Andrew Lunn wrote:
> > > > eth0: no link
> > > >   registers for MII PHY 0: 
> > > >     1140 7949 0022 1622 0981 c1e1 000d 0000
> > > 
> > > Hi Simon
> > > 
> > > The ID registers 0022 1622 indicate this is a Micrel KSZ9031.
> > > Are you using the micrel PHY driver?
> > 
> > 
> > Yes, when the Link is successfully negotiated I see:
> > 
> > Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)
> > 
> > > > I note a difference in the 3rd line of hex output: 7002 vs 6002
> > > > but I am unsure if that is relevant.
> > > 
> > > Register 20, or 0x14. The datasheet says "Reserved" and there is no
> > > description given :-(
> > > 
> > > I will decode the other registers and see if i can find anything.
> > 
> > Thanks, very much appreciated.
> > 
> > I believe your patch to disable Asym_Pause solves the immediate
> > problem I have observed.
> 
> Andrew, how would you like to resolve this?
> Let me know how I can help.

Hi Simon

I submitted it to netdev in the usual way. I hope DaveM will accept
and merge it.

    Andrew

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Simon Horman]

On Mon, Sep 24, 2018 at 05:50:23PM +0200, Andrew Lunn wrote:
> On Mon, Sep 24, 2018 at 05:36:00PM +0200, Simon Horman wrote:
> > On Fri, Sep 21, 2018 at 10:17:35AM +0200, Simon Horman wrote:
> > > On Thu, Sep 20, 2018 at 02:51:06PM +0200, Andrew Lunn wrote:
> > > > > eth0: no link
> > > > >   registers for MII PHY 0: 
> > > > >     1140 7949 0022 1622 0981 c1e1 000d 0000
> > > > 
> > > > Hi Simon
> > > > 
> > > > The ID registers 0022 1622 indicate this is a Micrel KSZ9031.
> > > > Are you using the micrel PHY driver?
> > > 
> > > 
> > > Yes, when the Link is successfully negotiated I see:
> > > 
> > > Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=204)
> > > 
> > > > > I note a difference in the 3rd line of hex output: 7002 vs 6002
> > > > > but I am unsure if that is relevant.
> > > > 
> > > > Register 20, or 0x14. The datasheet says "Reserved" and there is no
> > > > description given :-(
> > > > 
> > > > I will decode the other registers and see if i can find anything.
> > > 
> > > Thanks, very much appreciated.
> > > 
> > > I believe your patch to disable Asym_Pause solves the immediate
> > > problem I have observed.
> > 
> > Andrew, how would you like to resolve this?
> > Let me know how I can help.
> 
> Hi Simon
> 
> I submitted it to netdev in the usual way. I hope DaveM will accept
> and merge it.

Thanks, I see that now. Sorry for not noticing it before pinging you.
And thanks a lot for your patience in resolving this problem.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 24 Sep 2018 17:50:23 +0200

> I submitted it to netdev in the usual way. I hope DaveM will accept
> and merge it.

Andrew did I miss your patch somehow?

If so, sorry, please resend.

Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode

[Simon Horman]

On Wed, Sep 26, 2018 at 08:08:46PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Mon, 24 Sep 2018 17:50:23 +0200
> 
> > I submitted it to netdev in the usual way. I hope DaveM will accept
> > and merge it.
> 
> Andrew did I miss your patch somehow?
> 
> If so, sorry, please resend.

Hi Dave,

I believe you have the patch in net-next as:

65c5877f6462 ("ravb: Disable Pause Advertisement")