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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 5 Nov 2022 18:04:32 -0700 From: Florian Fainelli <f.fainelli@...il.com> To: "Russell King (Oracle)" <linux@...linux.org.uk> Cc: Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org, Claudiu Manoil <claudiu.manoil@....com>, Alexandre Belloni <alexandre.belloni@...tlin.com>, UNGLinuxDriver@...rochip.com, Heiner Kallweit <hkallweit1@...il.com>, Sean Anderson <sean.anderson@...o.com>, Colin Foster <colin.foster@...advantage.com>, Andrew Lunn <andrew@...n.ch>, Vivien Didelot <vivien.didelot@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com> Subject: Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method On 11/4/2022 9:35 AM, Russell King (Oracle) wrote: > On Fri, Nov 04, 2022 at 08:40:56AM -0700, Florian Fainelli wrote: >> On 11/4/2022 4:35 AM, Russell King (Oracle) wrote: >>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c >>> index 18a3847bd82b..6676971128d1 100644 >>> --- a/drivers/net/dsa/bcm_sf2.c >>> +++ b/drivers/net/dsa/bcm_sf2.c >>> @@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, >>> __set_bit(PHY_INTERFACE_MODE_MII, interfaces); >>> __set_bit(PHY_INTERFACE_MODE_REVMII, interfaces); >>> __set_bit(PHY_INTERFACE_MODE_GMII, interfaces); >>> + >>> + /* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported? >>> + * See FIXME 2 and FIXME 3 below. >>> + */ >> >> They are supported, just not tested and still don't have hardware to test, I >> suppose you can include both modes for simplicity if they end up being >> broken, a fix would be submitted. > > Okay, that sounds like we can add them to the switch() in > bcm_sf2_sw_mac_config(). I assume id_mode_dis should be zero for > these other modes. Yes please. > >>> +static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port, >>> + unsigned long *supported, >>> + struct phylink_link_state *state) >>> +{ >>> + u32 caps; >>> + >>> + caps = dsa_to_port(ds, port)->pl_config.mac_capabilities; >>> + >>> + /* Pause modes are only programmed for these modes - see FIXME 3. >>> + * So, as pause modes are not configured for other modes, disable >>> + * support for them. If FIXME 3 is updated to allow the other RGMII >>> + * modes, these should be included here as well. >>> + */ >>> + if (!(state->interface == PHY_INTERFACE_MODE_RGMII || >>> + state->interface == PHY_INTERFACE_MODE_RGMII_TXID || >>> + state->interface == PHY_INTERFACE_MODE_MII || >>> + state->interface == PHY_INTERFACE_MODE_REVMII)) >>> + caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE); >> >> Can be programmed on all ports. > > If I understand you correctly, I think you mean that this can be > programmed on all ports that support the RGMII control register: > > if (phy_interface_mode_is_rgmii(interface) || > interface == PHY_INTERFACE_MODE_MII || > interface == PHY_INTERFACE_MODE_REVMII) { > reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port); > reg = reg_readl(priv, reg_rgmii_ctrl); > reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN); > > if (tx_pause) > reg |= TX_PAUSE_EN; > if (rx_pause) > reg |= RX_PAUSE_EN; > > reg_writel(priv, reg, reg_rgmii_ctrl); > } > > We seem to have several places in the code that make this decision - > bcm_sf2_sw_mac_link_set(). I'm guessing, looking at > bcm_sf2_reg_rgmii_cntrl(), that we _could_ use the device ID and port > number in bcm_sf2_sw_get_caps() to narrow down whether the pause > modes are supported - as ports that do not have the RGMII control > register can't have pause modes programmed? > > So for the BCM4908, it's just port 7, and for everything else it's > ports 0-2. > > That would mean something like: > > static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, > struct phylink_config *config) > { > ... > struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); > ... > config->mac_capabilities = MAC_10 | MAC_100 | MAC_1000; > > if (priv->type == BCM4908_DEVICE_ID ? port == 7 : > (port >= 0 && port < 3)) > config->mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE; > > may be sensible? Sorry, I should have been way more specific here, all ports support pause frames, however only a subset of the ports, those connected over RGMII require to program the reg_rgmii_ctrl offset with the advertised pause settings. > > It brings up another question: if the port supports this register, but > we aren't using one of the rgmii, mii or revmii modes, should we be > clearing the pause bits in this register if we're telling the system > that pause isn't supported, or does the hardware not look at this RGMII > register unless its in one of those three modes? It is the latter, the hardware does not look at this RGMII register unless it is one of the 4 possible RGMII modes, MII, or reverse MII. -- Florian
Powered by blists - more mailing lists