| Message ID | 20200217172242.GZ25745@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | rework phylink interface for split MAC/PCS support | expand |
On Mon, Feb 17, 2020 at 05:22:43PM +0000, Russell King - ARM Linux admin wrote: > Hi, > > The following series changes the phylink interface to allow us to > better support split MAC / MAC PCS setups. The fundamental change > required for this turns out to be quite simple. Hi Russell Do you have a branch i can pull and test? Thanks Andrew
On Mon, Feb 17, 2020 at 05:23:54PM +0000, Russell King wrote:
> + Please see :c:func:`mac_link_up` for more information on this.
FYI, Jon recently added the ability to specify functions as
+ Please see mac_link_up() for more information on this.
and it's now the preferred way to do this. Nothing that should stand in
the way of this patch-set, of course.
On Mon, Feb 17, 2020 at 10:03:59AM -0800, Matthew Wilcox wrote: > On Mon, Feb 17, 2020 at 05:23:54PM +0000, Russell King wrote: > > + Please see :c:func:`mac_link_up` for more information on this. > > FYI, Jon recently added the ability to specify functions as > > + Please see mac_link_up() for more information on this. > > and it's now the preferred way to do this. Nothing that should stand in > the way of this patch-set, of course. Thanks for letting me know - it sounds like the subject of a future patch to convert all instances. In the mean time, I suggest keeping to the current style in the file for consistency...
On Mon, Feb 17, 2020 at 06:33:24PM +0100, Andrew Lunn wrote: > On Mon, Feb 17, 2020 at 05:22:43PM +0000, Russell King - ARM Linux admin wrote: > > Hi, > > > > The following series changes the phylink interface to allow us to > > better support split MAC / MAC PCS setups. The fundamental change > > required for this turns out to be quite simple. > > Hi Russell > > Do you have a branch i can pull and test? Nothing beyond the branches I've mentioned in the previous heads-up as yet, sorry.
On Mon, Feb 17, 2020 at 06:51:31PM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 17, 2020 at 06:33:24PM +0100, Andrew Lunn wrote: > > On Mon, Feb 17, 2020 at 05:22:43PM +0000, Russell King - ARM Linux admin wrote: > > > Hi, > > > > > > The following series changes the phylink interface to allow us to > > > better support split MAC / MAC PCS setups. The fundamental change > > > required for this turns out to be quite simple. > > > > Hi Russell > > > > Do you have a branch i can pull and test? > > Nothing beyond the branches I've mentioned in the previous heads-up as > yet, sorry. In any case, for any particular network driver, there are three patches maximum that you need - the first, and the one or two patches specific to the network driver, depending whether it's a DSA driver or not. You don't need all 8 patches to test this series. All can be applied on top of yesterday's net-next, specifically 92df9f8a745e ("Merge branch 'mvneta-xdp-ethtool-stats'")
All, This is now almost four months old, but I see that I didn't copy the message to everyone who should've been, especially for the five remaining drivers. I had asked for input from maintainers to help me convert their phylink-using drivers to the new style where mac_link_up() performs the speed, duplex and pause setup rather than mac_config(). So far, I have had very little assistance with this, and it is now standing in the way of further changes to phylink, particularly with proper PCS support. You are effectively blocking this work; I can't break your code as that will cause a kernel regression. This is one of the reasons why there were not many phylink patches merged for the last merge window. The following drivers in current net-next remain unconverted: drivers/net/ethernet/mediatek/mtk_eth_soc.c drivers/net/dsa/ocelot/felix.c drivers/net/dsa/qca/ar9331.c drivers/net/dsa/bcm_sf2.c drivers/net/dsa/b53/b53_common.c These can be easily identified by grepping for conditionals where the expression matches the "MLO_PAUSE_.X" regexp. I have an untested patch that I will be sending out today for mtk_eth_soc.c, but the four DSA ones definitely require their authors or maintainers to either make the changes, or assist with that since their code is not straight forward. Essentially, if you are listed in this email's To: header, then you are listed as a maintainer for one of the affected drivers, and I am requesting assistance from you for this task please. Thanks. Russell. On Mon, Feb 17, 2020 at 05:22:43PM +0000, Russell King - ARM Linux admin wrote: > Hi, > > The following series changes the phylink interface to allow us to > better support split MAC / MAC PCS setups. The fundamental change > required for this turns out to be quite simple. > > Today, mac_config() is used for everything to do with setting the > parameters for the MAC, and mac_link_up() is used to inform the > MAC driver that the link is now up (and so to allow packet flow.) > mac_config() also has had a few implementation issues, with folk > who believe that members such as "speed" and "duplex" are always > valid, where "link" gets used inappropriately, etc. > > With the proposed patches, all this changes subtly - but in a > backwards compatible way at this stage. > > We pass the the full resolved link state (speed, duplex, pause) to > mac_link_up(), and it is now guaranteed that these parameters to > this function will always be valid (no more SPEED_UNKNOWN or > DUPLEX_UNKNOWN here - unless phylink is fed with such things.) > > Drivers should convert over to using the state in mac_link_up() > rather than configuring the speed, duplex and pause in the > mac_config() method. The patch series includes a number of MAC > drivers which I've thought have been easy targets - I've left the > remainder as I think they need maintainer input. However, *all* > drivers will need conversion for future phylink development. > > Documentation/networking/sfp-phylink.rst | 17 +++- > drivers/net/dsa/b53/b53_common.c | 4 +- > drivers/net/dsa/b53/b53_priv.h | 4 +- > drivers/net/dsa/bcm_sf2.c | 4 +- > drivers/net/dsa/lantiq_gswip.c | 4 +- > drivers/net/dsa/mt7530.c | 4 +- > drivers/net/dsa/mv88e6xxx/chip.c | 79 +++++++++++++---- > drivers/net/dsa/sja1105/sja1105_main.c | 4 +- > drivers/net/ethernet/cadence/macb.h | 1 - > drivers/net/ethernet/cadence/macb_main.c | 53 ++++++----- > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 61 ++++++++----- > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h | 1 + > drivers/net/ethernet/marvell/mvneta.c | 63 ++++++++----- > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 102 +++++++++++++--------- > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 7 +- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +- > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 37 ++++---- > drivers/net/phy/phylink.c | 9 +- > include/linux/phylink.h | 57 ++++++++---- > include/net/dsa.h | 4 +- > net/dsa/port.c | 7 +- > 21 files changed, 350 insertions(+), 176 deletions(-) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up >
Hi Russell, On Sun, 21 Jun 2020 at 17:34, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > All, > > This is now almost four months old, but I see that I didn't copy the > message to everyone who should've been, especially for the five > remaining drivers. > > I had asked for input from maintainers to help me convert their > phylink-using drivers to the new style where mac_link_up() performs > the speed, duplex and pause setup rather than mac_config(). So far, > I have had very little assistance with this, and it is now standing > in the way of further changes to phylink, particularly with proper > PCS support. You are effectively blocking this work; I can't break > your code as that will cause a kernel regression. > > This is one of the reasons why there were not many phylink patches > merged for the last merge window. > > The following drivers in current net-next remain unconverted: > > drivers/net/ethernet/mediatek/mtk_eth_soc.c > drivers/net/dsa/ocelot/felix.c > drivers/net/dsa/qca/ar9331.c > drivers/net/dsa/bcm_sf2.c > drivers/net/dsa/b53/b53_common.c > > These can be easily identified by grepping for conditionals where the > expression matches the "MLO_PAUSE_.X" regexp. > > I have an untested patch that I will be sending out today for > mtk_eth_soc.c, but the four DSA ones definitely require their authors > or maintainers to either make the changes, or assist with that since > their code is not straight forward. > > Essentially, if you are listed in this email's To: header, then you > are listed as a maintainer for one of the affected drivers, and I am > requesting assistance from you for this task please. > > Thanks. > > Russell. > If forcing MAC speed is to be moved in mac_link_up(), and if (as you requested in the mdio-lynx-pcs thread) configuring the PCS is to be moved in pcs_link_up() and pcs_config() respectively, then what remains to be done in mac_config()? Regards, -Vladimir
On Sun, Jun 21, 2020 at 10:37:43PM +0300, Vladimir Oltean wrote: > Hi Russell, > > On Sun, 21 Jun 2020 at 17:34, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > All, > > > > This is now almost four months old, but I see that I didn't copy the > > message to everyone who should've been, especially for the five > > remaining drivers. > > > > I had asked for input from maintainers to help me convert their > > phylink-using drivers to the new style where mac_link_up() performs > > the speed, duplex and pause setup rather than mac_config(). So far, > > I have had very little assistance with this, and it is now standing > > in the way of further changes to phylink, particularly with proper > > PCS support. You are effectively blocking this work; I can't break > > your code as that will cause a kernel regression. > > > > This is one of the reasons why there were not many phylink patches > > merged for the last merge window. > > > > The following drivers in current net-next remain unconverted: > > > > drivers/net/ethernet/mediatek/mtk_eth_soc.c > > drivers/net/dsa/ocelot/felix.c > > drivers/net/dsa/qca/ar9331.c > > drivers/net/dsa/bcm_sf2.c > > drivers/net/dsa/b53/b53_common.c > > > > These can be easily identified by grepping for conditionals where the > > expression matches the "MLO_PAUSE_.X" regexp. > > > > I have an untested patch that I will be sending out today for > > mtk_eth_soc.c, but the four DSA ones definitely require their authors > > or maintainers to either make the changes, or assist with that since > > their code is not straight forward. > > > > Essentially, if you are listed in this email's To: header, then you > > are listed as a maintainer for one of the affected drivers, and I am > > requesting assistance from you for this task please. > > > > Thanks. > > > > Russell. > > > > If forcing MAC speed is to be moved in mac_link_up(), and if (as you > requested in the mdio-lynx-pcs thread) configuring the PCS is to be > moved in pcs_link_up() and pcs_config() respectively, then what > remains to be done in mac_config()? Hopefully very little, but I suspect there will still be a need for some kind of interface to configure the MAC interface type at the MAC. Note that I have said many many many times that using state->{speed, duplex,pause} in mac_config() when in in-band mode is unreliable, yet still people insist on using them. There _are_ and always _have been_ paths in phylink where these members will be passed with an unresolved state, and they will corrupt the link settings when that happens. I know that phylink was deficient in its handling of a split PCS, but I have worked to correct that. That job still is not complete, because because I'm held up by these drivers that have not yet converted. I've already waited a kernel cycle, despite having the next series of phylink patches ready and waiting since early February. I'm getting to the point of wishing that phylink did not have users except my own.
Le 2020-06-21 à 13:02, Russell King - ARM Linux admin a écrit : > On Sun, Jun 21, 2020 at 10:37:43PM +0300, Vladimir Oltean wrote: >> Hi Russell, >> >> On Sun, 21 Jun 2020 at 17:34, Russell King - ARM Linux admin >> <linux@armlinux.org.uk> wrote: >>> >>> All, >>> >>> This is now almost four months old, but I see that I didn't copy the >>> message to everyone who should've been, especially for the five >>> remaining drivers. >>> >>> I had asked for input from maintainers to help me convert their >>> phylink-using drivers to the new style where mac_link_up() performs >>> the speed, duplex and pause setup rather than mac_config(). So far, >>> I have had very little assistance with this, and it is now standing >>> in the way of further changes to phylink, particularly with proper >>> PCS support. You are effectively blocking this work; I can't break >>> your code as that will cause a kernel regression. >>> >>> This is one of the reasons why there were not many phylink patches >>> merged for the last merge window. >>> >>> The following drivers in current net-next remain unconverted: >>> >>> drivers/net/ethernet/mediatek/mtk_eth_soc.c >>> drivers/net/dsa/ocelot/felix.c >>> drivers/net/dsa/qca/ar9331.c >>> drivers/net/dsa/bcm_sf2.c >>> drivers/net/dsa/b53/b53_common.c >>> >>> These can be easily identified by grepping for conditionals where the >>> expression matches the "MLO_PAUSE_.X" regexp. >>> >>> I have an untested patch that I will be sending out today for >>> mtk_eth_soc.c, but the four DSA ones definitely require their authors >>> or maintainers to either make the changes, or assist with that since >>> their code is not straight forward. >>> >>> Essentially, if you are listed in this email's To: header, then you >>> are listed as a maintainer for one of the affected drivers, and I am >>> requesting assistance from you for this task please. >>> >>> Thanks. >>> >>> Russell. >>> >> >> If forcing MAC speed is to be moved in mac_link_up(), and if (as you >> requested in the mdio-lynx-pcs thread) configuring the PCS is to be >> moved in pcs_link_up() and pcs_config() respectively, then what >> remains to be done in mac_config()? > > Hopefully very little, but I suspect there will still be a need for > some kind of interface to configure the MAC interface type at the MAC. > > Note that I have said many many many times that using state->{speed, > duplex,pause} in mac_config() when in in-band mode is unreliable, yet > still people insist on using them. There _are_ and always _have been_ > paths in phylink where these members will be passed with an unresolved > state, and they will corrupt the link settings when that happens. > > I know that phylink was deficient in its handling of a split PCS, but > I have worked to correct that. That job still is not complete, because > because I'm held up by these drivers that have not yet converted. I've > already waited a kernel cycle, despite having the next series of > phylink patches ready and waiting since early February. > > I'm getting to the point of wishing that phylink did not have users > except my own. You have to realize that most people are not capable of working at your pace either because they just do not have your intellect or because their day job is not supporting a switch driver 100% of the time. The other thing to realize is that PHYLINK has seen a fair amount of churn since its introduction which makes it really hard to follow what is needed, what was bogus, what ended up working when it should not etc. You have clearly worked on improving both code and documentation, there is however inertia for people to catch up. So please stop with this attitude, just send the patches if it breaks it will take many cycles for people to realize it and that is on them, not you, they do not get to complain at all and should have been more reactive since you warned them. Thank you