| Message ID | 1580137671-22081-3-git-send-email-madalin.bucur@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: aquantia: indicate rate adaptation | expand |
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
> -----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
> 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
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
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
> -----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
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
> -----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
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);
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(-)