[PATCH v2 0/2] net: phy: aquantia: indicate rate adaptation

[PATCH v2 0/2] net: phy: aquantia: indicate rate adaptation

[Madalin Bucur]

Changes since v1:
  rewrote commit messages to evidentiate that this is a PHY
  generic feature, not a particular feature of the Aquantia
  PHYs and added more details on how the 1G link issue this
  is circumventing came about.

This patch-set introduces a bit into the phy_device
structure to indicate the PHYs ability to do rate
adaptation between the system and line interfaces and
sets this bit for the Aquantia PHYs that have this feature.

By taking into account the presence of the said bit, address
an issue with the LS1046ARDB board 10G interface no longer
working with an 1G link partner after autonegotiation support
was added for the Aquantia PHY on board in

commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-negotiation configuration")

Before this commit the values advertised by the PHY were not
influenced by the dpaa_eth driver removal of system-side unsupported
modes as the aqr_config_aneg() was basically a no-op. After this
commit, the modes removed by the dpaa_eth driver were no longer
advertised thus autonegotiation with 1G link partners failed.

Madalin Bucur (2):
  net: phy: aquantia: add rate_adaptation indication
  dpaa_eth: support all modes with rate adapting PHYs

 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
 drivers/net/phy/aquantia_main.c                |  3 +++
 include/linux/phy.h                            |  3 +++
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.1.0

[PATCH v2 1/2] net: phy: aquantia: add rate_adaptation indication

[Madalin Bucur]

Some PHYs are able to perform rate adaptation between
the system interface and the line interfaces. When such
a PHY is deployed, the ethernet driver should not limit
the modes supported or advertised by the PHY. This patch
introduces the bit that allows checking for this feature
in the phy_device structure and its use for the Aquantia
PHYs that have this capability.

Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
---
 drivers/net/phy/aquantia_main.c | 3 +++
 include/linux/phy.h             | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 975789d9349d..36fdd523b758 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device *phydev)
 	u16 reg;
 	int ret;
 
+	/* add here as this is called for all devices */
+	phydev->rate_adaptation = 1;
+
 	if (phydev->autoneg == AUTONEG_DISABLE)
 		return genphy_c45_pma_setup_forced(phydev);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dd4a91f1feaa..2a5c202333fc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -387,6 +387,9 @@ struct phy_device {
 	/* Interrupts are enabled */
 	unsigned interrupts:1;
 
+	/* Rate adaptation in the PHY */
+	unsigned rate_adaptation:1;
+
 	enum phy_state state;
 
 	u32 dev_flags;
-- 
2.1.0

[PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Madalin Bucur]

Stop removing modes that are not supported on the system interface
when the connected PHY is capable of rate adaptation. This addresses
an issue with the LS1046ARDB board 10G interface no longer working
with an 1G link partner after autonegotiation support was added
for the Aquantia PHY on board in

commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-negotiation configuration")

Before this commit the values advertised by the PHY were not
influenced by the dpaa_eth driver removal of system-side unsupported
modes as the aqr_config_aneg() was basically a no-op. After this
commit, the modes removed by the dpaa_eth driver were no longer
advertised thus autonegotiation with 1G link partners failed.

Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index a301f0095223..d3eb235450e5 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device *net_dev)
 		return -ENODEV;
 	}
 
-	/* Remove any features not supported by the controller */
-	ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev->if_support);
-	linkmode_and(phy_dev->supported, phy_dev->supported, mask);
+	if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
+	    !phy_dev->rate_adaptation) {
+		/* Remove any features not supported by the controller */
+		ethtool_convert_legacy_u32_to_link_mode(mask,
+							mac_dev->if_support);
+		linkmode_and(phy_dev->supported, phy_dev->supported, mask);
+	}
 
 	phy_support_asym_pause(phy_dev);
 
-- 
2.1.0

Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Vladimir Oltean]

Hi Madalin,

On Mon, 27 Jan 2020 at 17:08, Madalin Bucur <madalin.bucur@oss.nxp.com> wrote:
>
> Stop removing modes that are not supported on the system interface
> when the connected PHY is capable of rate adaptation. This addresses
> an issue with the LS1046ARDB board 10G interface no longer working
> with an 1G link partner after autonegotiation support was added
> for the Aquantia PHY on board in
>
> commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-negotiation configuration")
>
> Before this commit the values advertised by the PHY were not
> influenced by the dpaa_eth driver removal of system-side unsupported
> modes as the aqr_config_aneg() was basically a no-op. After this
> commit, the modes removed by the dpaa_eth driver were no longer
> advertised thus autonegotiation with 1G link partners failed.
>
> Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>
> Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index a301f0095223..d3eb235450e5 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device *net_dev)
>                 return -ENODEV;
>         }
>
> -       /* Remove any features not supported by the controller */
> -       ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev->if_support);
> -       linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> +       if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
> +           !phy_dev->rate_adaptation) {
> +               /* Remove any features not supported by the controller */
> +               ethtool_convert_legacy_u32_to_link_mode(mask,
> +                                                       mac_dev->if_support);
> +               linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> +       }

Is this sufficient?
I suppose this works because you have flow control enabled by default?
What would happen if the user would disable flow control with ethtool?

>
>         phy_support_asym_pause(phy_dev);
>
> --
> 2.1.0
>

Regards,
-Vladimir

RE: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Monday, January 27, 2020 5:31 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Andrew Lunn <andrew@lunn.ch>;
> Florian Fainelli <f.fainelli@gmail.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; netdev <netdev@vger.kernel.org>; ykaukab@suse.de
> Subject: Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting
> PHYs
> 
> Hi Madalin,
> 
> On Mon, 27 Jan 2020 at 17:08, Madalin Bucur <madalin.bucur@oss.nxp.com>
> wrote:
> >
> > Stop removing modes that are not supported on the system interface
> > when the connected PHY is capable of rate adaptation. This addresses
> > an issue with the LS1046ARDB board 10G interface no longer working
> > with an 1G link partner after autonegotiation support was added
> > for the Aquantia PHY on board in
> >
> > commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-
> negotiation configuration")
> >
> > Before this commit the values advertised by the PHY were not
> > influenced by the dpaa_eth driver removal of system-side unsupported
> > modes as the aqr_config_aneg() was basically a no-op. After this
> > commit, the modes removed by the dpaa_eth driver were no longer
> > advertised thus autonegotiation with 1G link partners failed.
> >
> > Reported-by: Mian Yousaf Kaukab <ykaukab@suse.de>
> > Signed-off-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index a301f0095223..d3eb235450e5 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device
> *net_dev)
> >                 return -ENODEV;
> >         }
> >
> > -       /* Remove any features not supported by the controller */
> > -       ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev-
> >if_support);
> > -       linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> > +       if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
> > +           !phy_dev->rate_adaptation) {
> > +               /* Remove any features not supported by the controller
> */
> > +               ethtool_convert_legacy_u32_to_link_mode(mask,
> > +                                                       mac_dev-
> >if_support);
> > +               linkmode_and(phy_dev->supported, phy_dev->supported,
> mask);
> > +       }
> 
> Is this sufficient?
> I suppose this works because you have flow control enabled by default?
> What would happen if the user would disable flow control with ethtool?

User misconfiguration is not something I'd try to prevent from driver code...

> >
> >         phy_support_asym_pause(phy_dev);
> >
> > --
> > 2.1.0
> >
> 
> Regards,
> -Vladimir

Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Andrew Lunn]

> Is this sufficient?
> I suppose this works because you have flow control enabled by default?
> What would happen if the user would disable flow control with ethtool?

It will still work. Network protocols expect packets to be dropped,
there are bottlenecks on the network, and those bottlenecks change
dynamically. TCP will still be able to determine how much traffic it
can send without too much packet loss, independent of if the
bottleneck is here between the MAC and the PHY, or later when it hits
an RFC 1149 link.

    Andrew

Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Vladimir Oltean]

Hi Andrew,

On Mon, 27 Jan 2020 at 18:04, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Is this sufficient?
> > I suppose this works because you have flow control enabled by default?
> > What would happen if the user would disable flow control with ethtool?
>
> It will still work. Network protocols expect packets to be dropped,
> there are bottlenecks on the network, and those bottlenecks change
> dynamically. TCP will still be able to determine how much traffic it
> can send without too much packet loss, independent of if the
> bottleneck is here between the MAC and the PHY, or later when it hits
> an RFC 1149 link.

Following this logic, this patch isn't needed at all, right? The PHY
will drop frames that it can't hold in its small FIFOs when adapting a
link speed to another, and higher-level protocols will cope. And flow
control at large isn't needed.

What I was trying to see Madalin's opinion on was whether in fact we
want to keep the RX flow control as 'fixed on' if the MAC supports it
and the PHY needs it, _as a function of the current phy_mode and maybe
link speed_ (the underlined part is important IMO).

>
>     Andrew
>

Thanks,
-Vladimir

Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Andrew Lunn]

On Tue, Jan 28, 2020 at 05:41:31PM +0200, Vladimir Oltean wrote:
> Hi Andrew,
> 
> On Mon, 27 Jan 2020 at 18:04, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > Is this sufficient?
> > > I suppose this works because you have flow control enabled by default?
> > > What would happen if the user would disable flow control with ethtool?
> >
> > It will still work. Network protocols expect packets to be dropped,
> > there are bottlenecks on the network, and those bottlenecks change
> > dynamically. TCP will still be able to determine how much traffic it
> > can send without too much packet loss, independent of if the
> > bottleneck is here between the MAC and the PHY, or later when it hits
> > an RFC 1149 link.
> 
> Following this logic, this patch isn't needed at all, right? The PHY
> will drop frames that it can't hold in its small FIFOs when adapting a
> link speed to another, and higher-level protocols will cope. And flow
> control at large isn't needed.

Hi Vladimir

It is something worth testing. It might depend on the size of the
FIFO, and offloads like GSO. If the interface is given a 64KByte skb
to send, and the hardware sends it in a 10G line rate burst, is that
going to overflow the small FIFO? If the PHY trigger flow control fast
enough to pause the MAC within this burst, the performance could be
better.

So it is not a binary works/broken, but what is performance like? Can
you get 1Gbps, or does it drop to a much lower speed with a lot of
retires?

Flow control might not be the only option. It is simple, so if it
works, great. But when the PHY reports it has link at 1G, could you
enable some traffic shaping in the MAC to limit it to 1G, even if it
is going out a 10G SERDES?

   Andrew

RE: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Tuesday, January 28, 2020 5:42 PM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; David S. Miller
> <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>; Heiner
> Kallweit <hkallweit1@gmail.com>; netdev <netdev@vger.kernel.org>;
> ykaukab@suse.de
> Subject: Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate
> adapting PHYs
> 
> Hi Andrew,
> 
> On Mon, 27 Jan 2020 at 18:04, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > Is this sufficient?
> > > I suppose this works because you have flow control enabled by
> default?
> > > What would happen if the user would disable flow control with
> ethtool?
> >
> > It will still work. Network protocols expect packets to be dropped,
> > there are bottlenecks on the network, and those bottlenecks change
> > dynamically. TCP will still be able to determine how much traffic it
> > can send without too much packet loss, independent of if the
> > bottleneck is here between the MAC and the PHY, or later when it hits
> > an RFC 1149 link.
> 
> Following this logic, this patch isn't needed at all, right? The PHY
> will drop frames that it can't hold in its small FIFOs when adapting a
> link speed to another, and higher-level protocols will cope. And flow
> control at large isn't needed.

I'm afraid you missed the patch description that explains there will be
no link with a 1G partner without this change:

<< After this
commit, the modes removed by the dpaa_eth driver were no longer
advertised thus autonegotiation with 1G link partners failed.>>

> What I was trying to see Madalin's opinion on was whether in fact we
> want to keep the RX flow control as 'fixed on' if the MAC supports it
> and the PHY needs it, _as a function of the current phy_mode and maybe
> link speed_ (the underlined part is important IMO).

That's a separate concern, by default all is fine, should the user want to
shoot himself in the foot, we may need to allow him to do it.

> >
> >     Andrew
> >
> 
> Thanks,
> -Vladimir

Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Vladimir Oltean]

Hi Madalin,

On Wed, 29 Jan 2020 at 11:09, Madalin Bucur (OSS)
<madalin.bucur@oss.nxp.com> wrote:
>
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@gmail.com>
> > Sent: Tuesday, January 28, 2020 5:42 PM
> > To: Andrew Lunn <andrew@lunn.ch>
> > Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; David S. Miller
> > <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>; Heiner
> > Kallweit <hkallweit1@gmail.com>; netdev <netdev@vger.kernel.org>;
> > ykaukab@suse.de
> > Subject: Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate
> > adapting PHYs
> >
> > Hi Andrew,
> >
> > On Mon, 27 Jan 2020 at 18:04, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > Is this sufficient?
> > > > I suppose this works because you have flow control enabled by
> > default?
> > > > What would happen if the user would disable flow control with
> > ethtool?
> > >
> > > It will still work. Network protocols expect packets to be dropped,
> > > there are bottlenecks on the network, and those bottlenecks change
> > > dynamically. TCP will still be able to determine how much traffic it
> > > can send without too much packet loss, independent of if the
> > > bottleneck is here between the MAC and the PHY, or later when it hits
> > > an RFC 1149 link.
> >
> > Following this logic, this patch isn't needed at all, right? The PHY
> > will drop frames that it can't hold in its small FIFOs when adapting a
> > link speed to another, and higher-level protocols will cope. And flow
> > control at large isn't needed.
>
> I'm afraid you missed the patch description that explains there will be
> no link with a 1G partner without this change:
>

So why not just remove that linkmode_and() at all, then?
What is it trying to accomplish, anyway? Avoiding the user from
shooting themselves in the foot maybe?

> << After this
> commit, the modes removed by the dpaa_eth driver were no longer
> advertised thus autonegotiation with 1G link partners failed.>>
>
> > What I was trying to see Madalin's opinion on was whether in fact we
> > want to keep the RX flow control as 'fixed on' if the MAC supports it
> > and the PHY needs it, _as a function of the current phy_mode and maybe
> > link speed_ (the underlined part is important IMO).
>
> That's a separate concern, by default all is fine, should the user want to
> shoot himself in the foot, we may need to allow him to do it.
>> > >
> > >     Andrew
> > >
> >
> > Thanks,
> > -Vladimir

-Vladimir

RE: [PATCH v2 2/2] dpaa_eth: support all modes with rate adapting PHYs

[Madalin Bucur (OSS)]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Wednesday, January 29, 2020 12:19 PM
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller <davem@davemloft.net>;
> Florian Fainelli <f.fainelli@gmail.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; netdev <netdev@vger.kernel.org>; ykaukab@suse.de
> Subject: Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate
> adapting PHYs
> 
> Hi Madalin,
> 
> On Wed, 29 Jan 2020 at 11:09, Madalin Bucur (OSS)
> <madalin.bucur@oss.nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Sent: Tuesday, January 28, 2020 5:42 PM
> > > To: Andrew Lunn <andrew@lunn.ch>
> > > Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; David S. Miller
> > > <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>;
> Heiner
> > > Kallweit <hkallweit1@gmail.com>; netdev <netdev@vger.kernel.org>;
> > > ykaukab@suse.de
> > > Subject: Re: [PATCH v2 2/2] dpaa_eth: support all modes with rate
> > > adapting PHYs
> > >
> > > Hi Andrew,
> > >
> > > On Mon, 27 Jan 2020 at 18:04, Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > Is this sufficient?
> > > > > I suppose this works because you have flow control enabled by
> > > default?
> > > > > What would happen if the user would disable flow control with
> > > ethtool?
> > > >
> > > > It will still work. Network protocols expect packets to be dropped,
> > > > there are bottlenecks on the network, and those bottlenecks change
> > > > dynamically. TCP will still be able to determine how much traffic
> it
> > > > can send without too much packet loss, independent of if the
> > > > bottleneck is here between the MAC and the PHY, or later when it
> hits
> > > > an RFC 1149 link.
> > >
> > > Following this logic, this patch isn't needed at all, right? The PHY
> > > will drop frames that it can't hold in its small FIFOs when adapting
> a
> > > link speed to another, and higher-level protocols will cope. And flow
> > > control at large isn't needed.
> >
> > I'm afraid you missed the patch description that explains there will be
> > no link with a 1G partner without this change:
> >
> 
> So why not just remove that linkmode_and() at all, then?
> What is it trying to accomplish, anyway? Avoiding the user from
> shooting themselves in the foot maybe?

If you would take the time to read the patch set, I think it would be clear
that no, I do not intend to remove that altogether, but only when the PHY
can make the different modes work by performing rate adaptation.

> > << After this
> > commit, the modes removed by the dpaa_eth driver were no longer
> > advertised thus autonegotiation with 1G link partners failed.>>
> >
> > > What I was trying to see Madalin's opinion on was whether in fact we
> > > want to keep the RX flow control as 'fixed on' if the MAC supports it
> > > and the PHY needs it, _as a function of the current phy_mode and
> maybe
> > > link speed_ (the underlined part is important IMO).
> >
> > That's a separate concern, by default all is fine, should the user want
> to
> > shoot himself in the foot, we may need to allow him to do it.
> >> > >
> > > >     Andrew
> > > >
> > >
> > > Thanks,
> > > -Vladimir
> 
> -Vladimir