Message-ID: <20200213133831.GM25745@shell.armlinux.org.uk>
Date:   Thu, 13 Feb 2020 13:38:31 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     netdev@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Felix Fietkau <nbd@...nwrt.org>,
        John Crispin <john@...ozen.org>,
        Mark Lee <Mark-MC.Lee@...iatek.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Jose Abreu <joabreu@...opsys.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Radhey Shyam Pandey <radhey.shyam.pandey@...inx.com>,
        Ioana Radulescu <ruxandra.radulescu@....com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Heads up: phylink changes for next merge window

Hi,

During the next round of development changes, I wish to make some
changes to phylink which will affect almost every user out there,
as it affects the interfaces to the MAC drivers.

The reason behind the change is to allow us to support situations
where the MAC is not closely coupled with its associated PCS, such
as is found in mvneta and mvpp2.  This is necessary to support
already existing hardware properly, such as Marvell DSA and Xilinx
AXI ethernet drivers, and there seems to be a growing need for this.

What I'm proposing to do is to move the MAC setup for the negotiated
speed, duplex and pause settings to the mac_link_up() method, out of
the existing mac_config() method.  I have already converted the
axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
know enough about the hardware to do the conversion myself.

The other benefit this has is that mac_config() called with
SPEED_UNKNOWN and DUPLEX_UNKNOWN has been a pain point for some;
mac_link_up() should never be called with speed or duplex set to these
- if it is, something is wrong with phylib or the mac_pcs_get_state()
implementation.  Hence, this pain point should be gone with this
proposal.

Once net-next has opened, and when the pre-requisit patches have been
reviewed, I'll post the series that includes these conversions.

The queued work can be found in my "phy" branch, viewable at:

  http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy

which I maintain as a re-sortable queue of pending patches; it is
based on v5.5 but I track most commits that impact on the pending
patches through a merge window.  Essentially, "net: phylink: allow
in-band AN for USXGMII" and preceding patches are in 5.6-rc1.

The initial plan will be to send:

- Batch 1:
   net: linkmode: make linkmode_test_bit() take const pointer
   net: add helpers to resolve negotiated flow control
   net: add linkmode helper for setting flow control advertisement
   net: phylink: remove pause mode ethtool setting for fixed links
   net: phylink: ensure manual flow control is selected appropriately
   net: phylink: use phylib resolved flow control modes
   net: phylink: resolve fixed link flow control
   net: phylink: allow ethtool -A to change flow control advertisement
   net: phylink: improve initial mac configuration
   net: phylink: clarify flow control settings in documentation
- Batch 2:
   net: phy: marvell10g: read copper results from CSSR1
   net: phy: marvell: don't interpret PHY status unless resolved
   net: phy: add resolved pause support
   net: phy: marvell*: add support for hw resolved pause modes
- Maybe some others from there, and then a series which starts the ball
  rollong on this:
   net: mii: convert mii_lpa_to_ethtool_lpa_x() to linkmode variant
   net: mii: add linkmode_adv_to_mii_adv_x()
   net: phylink: propagate resolved link config via mac_link_up()
   net: dsa: propagate resolved link config via mac_link_up()
   net: mv88e6xxx: use resolved link config in mac_link_up()
   net: axienet: use resolved link config in mac_link_up()
   net: dpaa2-mac: use resolved link config in mac_link_up()
   net: macb: use resolved link config in mac_link_up()
   net: mvneta: use resolved link config in mac_link_up()
   net: mvpp2: use resolved link config in mac_link_up()

When all drivers have been converted, I propose to make phylink stop
calling mac_config() just before a link-up event for the MLO_AN_PHY
and MLO_AN_FIXED cases, unless a major reconfiguration of the MAC is
required (e.g. the PHY_INTERFACE_MODE has changed.)  Hence why all
existing drivers will need to be converted before this step.

I have a Coccinelle script that finds unconverted drivers, which I've
attached, runnable with:

spatch -D report --very-quiet --sp-file phylink-config.cocci drivers/net

The script will warn about the use of deprecated phylink_link_state
members, and error about use of ->link which has never been guaranteed
to be correct (there are code paths where it is definitely not correct,
particularly through the ethtool code paths, so it's use by drivers is
already buggy - bcm_sf2 seems to be the only user.)

I'm considering adding this coccinelle script to scripts/coccinelle so
that 0-day can catch any future inappropriate uses, although I would
like to eventually revise mac_config() sometime in the future so that it's
not possible for this to happen in the first place.

The remaining files are:

drivers/net/dsa/b53/b53_common.c
drivers/net/dsa/bcm_sf2.c
drivers/net/dsa/mt7530.c
drivers/net/dsa/sja1105/sja1105_main.c
drivers/net/ethernet/mediatek/mtk_eth_soc.c
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

Russell.

-- 
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

View attachment "phylink-config.cocci" of type "text/plain" (1635 bytes)

Powered by blists - more mailing lists