netdev - Re: [PATCH net-next 4/4] net: dsa: remove phylink

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c68ba566-debb-8c3a-f97d-535b3eca49fe@gmail.com>
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