linux-kernel - Re: [net-next PATCH v4 09/11] net: macb: Move most of mac_config to mac

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <e545e5c2-daeb-4cd1-b9f8-e5d28e6250c8@linux.dev>
Date: Tue, 13 May 2025 11:49:43 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: "Karumanchi, Vineeth" <vineeth.karumanchi@....com>,
 netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Russell King <linux@...linux.org.uk>
Cc: upstream@...oha.com, Simon Horman <horms@...nel.org>,
 Heiner Kallweit <hkallweit1@...il.com>,
 Kory Maincent <kory.maincent@...tlin.com>, linux-kernel@...r.kernel.org,
 Christian Marangi <ansuelsmth@...il.com>,
 Claudiu Beznea <claudiu.beznea@...rochip.com>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>
Subject: Re: [net-next PATCH v4 09/11] net: macb: Move most of mac_config to
 mac_prepare

On 5/13/25 11:29, Karumanchi, Vineeth wrote:
> Hi Sean,
> 
> Sorry for the delayed response.
> 
> We are working on MACB with two internal PCS's (10G-BASER, 1000-BASEX) supporting 1G, 2.5G, 5G, and 10G with AN disabled.
> 
> I have sent an initial RFC : https://lore.kernel.org/netdev/20241009053946.3198805-1-vineeth.karumanchi@amd.com/
> 
> Currently, we are working on integrating the MAC in fixed-link and phy-mode.
> 
> Please see my inline comments.
> 
> On 5/12/2025 9:44 PM, Sean Anderson wrote:
>> mac_prepare is called every time the interface is changed, so we can do
>> all of our configuration there, instead of in mac_config. This will be
>> useful for the next patch where we will set the PCS bit based on whether
>> we are using our internal PCS. No functional change intended.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
>> ---
>>
> 
> <...>
> 
>> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>> +               phy_interface_t interface,
>> +               const unsigned long *advertising,
>> +               bool permit_pause_to_mac)
>> +{
>> +    struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
>> +    bool changed = false;
>> +    unsigned long flags;
>> +    u32 old, new;
>> +
>> +    spin_lock_irqsave(&bp->lock, flags);
>> +    old = new = gem_readl(bp, NCFGR);
>> +    new |= GEM_BIT(SGMIIEN);
> 
> This bit represents the AN feature, can we make it conditional to facilitate IP's with AN disabled.

To clarify, this bit enables SGMII timings for AN (as opposed to
1000Base-X). If you don't have AN enabled at 1G, then this bit affects
nothing.

1000Base-X is not currently supported by the built-in PCS. Therefore,
this bit should be set unconditionally at 1G speeds. This patch aims to
avoid functional changes so I have not made it conditional. Making this
bit conditional would be appropriate for a patch adding support for
1000Base-X using the internal PCS.

>> +    if (old != new) {
>> +        changed = true;
>> +        gem_writel(bp, NCFGR, new);
>> +    }
> 
> <..>
> 
>>     static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
>> @@ -589,45 +661,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
>>                      bool permit_pause_to_mac)
>>   {
>>       struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>> +    unsigned long flags;
>> +    bool changed;
>> +    u16 old, new;
>>   -    gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
>> -           GEM_BIT(SIGNAL_OK));
>> +    spin_lock_irqsave(&bp->lock, flags);
>> +    if (macb_pcs_config_an(bp, neg_mode, interface, advertising))
>> +        changed = true;
>>   -    return 0;
>> -}
>> +    old = new = gem_readl(bp, USX_CONTROL);
>> +    new |= GEM_BIT(SIGNAL_OK);
>> +    if (old != new) {
>> +        changed = true;
>> +        gem_writel(bp, USX_CONTROL, new);
>> +    }
>>   -static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>> -                   struct phylink_link_state *state)
>> -{
>> -    state->link = 0;
>> -}
>> +    old = new = gem_readl(bp, USX_CONTROL);
>> +    new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
>> +    new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
>> +    new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
>> +    new |= GEM_BIT(TX_EN);
>> +    if (old != new) {
>> +        changed = true;
>> +        gem_writel(bp, USX_CONTROL, new);
>> +    }
> 
> The above speed/rate configuration was moved from macb_usx_pcs_link_up() where speed is an argument, which can be leveraged to configure multiple speeds.
> 
> Can we achieve configuring for multiple speeds from macb_usx_pcs_config() in fixed-link and phy-mode ?

Form what I can tell, the USX PCS is only used for 10G interfaces. If
you want to add support for using it at other link speeds, then yes some
of these register writes should be moved to link_up. For the moment it
doesn't matter where they happen.

--Sean