| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6qpcartwgkgdmtxwj4puxn2exbpiv6t6fxv2b3kecu6ezzdogs@yii3j3xtougr>
Date: Wed, 5 Jun 2024 16:59:14 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Cc: Serge Semin <fancer.lancer@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH RFC net-next v2 3/8] net: stmmac: dwmac1000: convert
sgmii/rgmii "pcs" to phylink
On Wed, Jun 05, 2024 at 03:05:43PM GMT, Andrew Halaney wrote:
> On Fri, May 31, 2024 at 12:26:25PM GMT, Russell King (Oracle) wrote:
> > Convert dwmac1000 sgmii/rgmii "pcs" implementation to use a phylink_pcs
> > so we can eventually get rid of the exceptional paths that conflict
> > with phylink.
> >
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > ---
> > .../ethernet/stmicro/stmmac/dwmac1000_core.c | 113 ++++++++++++------
> > 1 file changed, 75 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> > index d413d76a8936..4a0572d5f865 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> > @@ -16,6 +16,7 @@
> > #include <linux/slab.h>
> > #include <linux/ethtool.h>
> > #include <linux/io.h>
> > +#include <linux/phylink.h>
> > #include "stmmac.h"
> > #include "stmmac_pcs.h"
> > #include "dwmac1000.h"
> > @@ -261,39 +262,6 @@ static void dwmac1000_pmt(struct mac_device_info *hw, unsigned long mode)
> > writel(pmt, ioaddr + GMAC_PMT);
> > }
> >
> > -/* RGMII or SMII interface */
> > -static void dwmac1000_rgsmii(void __iomem *ioaddr, struct stmmac_extra_stats *x)
> > -{
> > - u32 status;
> > -
> > - status = readl(ioaddr + GMAC_RGSMIIIS);
> > - x->irq_rgmii_n++;
> > -
> > - /* Check the link status */
> > - if (status & GMAC_RGSMIIIS_LNKSTS) {
> > - int speed_value;
> > -
> > - x->pcs_link = 1;
> > -
> > - speed_value = ((status & GMAC_RGSMIIIS_SPEED) >>
> > - GMAC_RGSMIIIS_SPEED_SHIFT);
> > - if (speed_value == GMAC_RGSMIIIS_SPEED_125)
> > - x->pcs_speed = SPEED_1000;
> > - else if (speed_value == GMAC_RGSMIIIS_SPEED_25)
> > - x->pcs_speed = SPEED_100;
> > - else
> > - x->pcs_speed = SPEED_10;
> > -
> > - x->pcs_duplex = (status & GMAC_RGSMIIIS_LNKMOD_MASK);
> > -
> > - pr_info("Link is Up - %d/%s\n", (int)x->pcs_speed,
> > - x->pcs_duplex ? "Full" : "Half");
> > - } else {
> > - x->pcs_link = 0;
> > - pr_info("Link is Down\n");
> > - }
> > -}
> > -
> > static int dwmac1000_irq_status(struct mac_device_info *hw,
> > struct stmmac_extra_stats *x)
> > {
> > @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
> >
> > dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
> >
> > - if (intr_status & PCS_RGSMIIIS_IRQ)
> > - dwmac1000_rgsmii(ioaddr, x);
> > + if (intr_status & PCS_RGSMIIIS_IRQ) {
> > + /* TODO Dummy-read to clear the IRQ status */
> > + readl(ioaddr + GMAC_RGSMIIIS);
>
> This seems to me that you're doing the TODO here? Maybe I'm
> misunderstanding... maybe not :)
>
> > + phylink_pcs_change(&hw->mac_pcs, false);
Continuing to read through this all, sorry for the double reply and
possibly dumb question. Should we be passing in false unconditionally
here?
Prior code had checked GMAC_RGSMIIIS & GMAC_RGSMIIIS_LNKSTS to determine
if the link was up/down, which seem logical to pass in here too. Reading
the kerneldoc for phylink_pcs_change() I'm not totally positive if
we're in the "otherwise pass false" state here or some other detail I'm
missing though (it seems that maybe we're just sort of kicking the can
to phylink_resolve() which ends up calling into the get_state()
callback?)
> > + x->irq_rgmii_n++;
> > + }
> >
> > return ret;
> > }
> > @@ -404,9 +376,71 @@ static void dwmac1000_ctrl_ane(void __iomem *ioaddr, bool ane, bool srgmi_ral,
> > dwmac_ctrl_ane(ioaddr, GMAC_PCS_BASE, ane, srgmi_ral, loopback);
> > }
> >
> > -static void dwmac1000_get_adv_lp(void __iomem *ioaddr, struct rgmii_adv *adv)
> > +static int dwmac1000_mii_pcs_validate(struct phylink_pcs *pcs,
> > + unsigned long *supported,
> > + const struct phylink_link_state *state)
> > +{
> > + /* Only support in-band */
> > + if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int dwmac1000_mii_pcs_config(struct phylink_pcs *pcs,
> > + unsigned int neg_mode,
> > + phy_interface_t interface,
> > + const unsigned long *advertising,
> > + bool permit_pause_to_mac)
> > {
> > - dwmac_get_adv_lp(ioaddr, GMAC_PCS_BASE, adv);
> > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
> > +
> > + return dwmac_pcs_config(hw, neg_mode, interface, advertising,
> > + GMAC_PCS_BASE);
> > +}
> > +
> > +static void dwmac1000_mii_pcs_get_state(struct phylink_pcs *pcs,
> > + struct phylink_link_state *state)
> > +{
> > + struct mac_device_info *hw = phylink_pcs_to_mac_dev_info(pcs);
> > + unsigned int spd_clk;
> > + u32 status;
> > +
> > + status = readl(hw->pcsr + GMAC_RGSMIIIS);
> > +
> > + state->link = status & GMAC_RGSMIIIS_LNKSTS;
> > + if (!state->link)
> > + return;
> > +
> > + spd_clk = FIELD_GET(GMAC_RGSMIIIS_SPEED, status);
> > + if (spd_clk == GMAC_RGSMIIIS_SPEED_125)
> > + state->speed = SPEED_1000;
> > + else if (spd_clk == GMAC_RGSMIIIS_SPEED_25)
> > + state->speed = SPEED_100;
> > + else if (spd_clk == GMAC_RGSMIIIS_SPEED_2_5)
> > + state->speed = SPEED_10;
> > +
> > + state->duplex = status & GMAC_RGSMIIIS_LNKMOD_MASK ?
> > + DUPLEX_FULL : DUPLEX_HALF;
> > +
> > + dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE);
> > +}
> > +
> > +static const struct phylink_pcs_ops dwmac1000_mii_pcs_ops = {
> > + .pcs_validate = dwmac1000_mii_pcs_validate,
> > + .pcs_config = dwmac1000_mii_pcs_config,
> > + .pcs_get_state = dwmac1000_mii_pcs_get_state,
> > +};
> > +
> > +static struct phylink_pcs *
> > +dwmac1000_phylink_select_pcs(struct stmmac_priv *priv,
> > + phy_interface_t interface)
> > +{
> > + if (priv->hw->pcs & STMMAC_PCS_RGMII ||
> > + priv->hw->pcs & STMMAC_PCS_SGMII)
> > + return &priv->hw->mac_pcs;
> > +
> > + return NULL;
> > }
> >
> > static void dwmac1000_debug(struct stmmac_priv *priv, void __iomem *ioaddr,
> > @@ -499,6 +533,7 @@ static void dwmac1000_set_mac_loopback(void __iomem *ioaddr, bool enable)
> >
> > const struct stmmac_ops dwmac1000_ops = {
> > .core_init = dwmac1000_core_init,
> > + .phylink_select_pcs = dwmac1000_phylink_select_pcs,
> > .set_mac = stmmac_set_mac,
> > .rx_ipc = dwmac1000_rx_ipc_enable,
> > .dump_regs = dwmac1000_dump_regs,
> > @@ -514,7 +549,6 @@ const struct stmmac_ops dwmac1000_ops = {
> > .set_eee_pls = dwmac1000_set_eee_pls,
> > .debug = dwmac1000_debug,
> > .pcs_ctrl_ane = dwmac1000_ctrl_ane,
> > - .pcs_get_adv_lp = dwmac1000_get_adv_lp,
> > .set_mac_loopback = dwmac1000_set_mac_loopback,
> > };
> >
> > @@ -549,5 +583,8 @@ int dwmac1000_setup(struct stmmac_priv *priv)
> > mac->mii.clk_csr_shift = 2;
> > mac->mii.clk_csr_mask = GENMASK(5, 2);
> >
> > + mac->mac_pcs.ops = &dwmac1000_mii_pcs_ops;
> > + mac->mac_pcs.neg_mode = true;
> > +
> > return 0;
> > }
> > --
> > 2.30.2
> >
Powered by blists - more mailing lists