| 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: <f030e42f-6655-451c-9453-5026d27c0980@quicinc.com>
Date: Tue, 11 Jun 2024 11:22:32 -0700
From: "Abhishek Chauhan (ABC)" <quic_abchauha@...cinc.com>
To: Andrew Halaney <ahalaney@...hat.com>,
"Russell King (Oracle)"
<rmk+kernel@...linux.org.uk>,
Sneh Shah <quic_snehshah@...cinc.com>
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 5/8] net: stmmac: dwmac4: convert
sgmii/rgmii "pcs" to phylink
On 6/5/2024 3:35 PM, Andrew Halaney wrote:
> On Fri, May 31, 2024 at 12:26:35PM GMT, Russell King (Oracle) wrote:
>> Convert dwmac4 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>
>> ---
>> .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 102 ++++++++++++------
>> 1 file changed, 72 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
>> index dbd9f93b2460..cb99cb69c52b 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
>> @@ -14,6 +14,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 "dwmac4.h"
>> @@ -758,42 +759,76 @@ static void dwmac4_ctrl_ane(void __iomem *ioaddr, bool ane, bool srgmi_ral,
>> dwmac_ctrl_ane(ioaddr, GMAC_PCS_BASE, ane, srgmi_ral, loopback);
>> }
>>
>> -static void dwmac4_get_adv_lp(void __iomem *ioaddr, struct rgmii_adv *adv)
>> +static int dwmac4_mii_pcs_validate(struct phylink_pcs *pcs,
>> + unsigned long *supported,
>> + const struct phylink_link_state *state)
>> {
>> - dwmac_get_adv_lp(ioaddr, GMAC_PCS_BASE, adv);
>> + /* Only support in-band */
>> + if (!test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, state->advertising))
>> + return -EINVAL;
>> +
>> + return 0;
>> }
>>
>> -/* RGMII or SMII interface */
>> -static void dwmac4_phystatus(void __iomem *ioaddr, struct stmmac_extra_stats *x)
>> +static int dwmac4_mii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>> + phy_interface_t interface,
>> + const unsigned long *advertising,
>> + bool permit_pause_to_mac)
>> {
>> + 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 dwmac4_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 clk_spd;
>> u32 status;
>>
>> - status = readl(ioaddr + GMAC_PHYIF_CONTROL_STATUS);
>> - x->irq_rgmii_n++;
>> + status = readl(hw->pcsr + GMAC_PHYIF_CONTROL_STATUS);
>> +
>> + state->link = !!(status & GMAC_PHYIF_CTRLSTATUS_LNKSTS);
>> + if (!state->link)
>> + return;
>>
>> - /* Check the link status */
>> - if (status & GMAC_PHYIF_CTRLSTATUS_LNKSTS) {
>> - int speed_value;
>> + clk_spd = FIELD_GET(GMAC_PHYIF_CTRLSTATUS_SPEED, status);
>> + if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_125)
>> + state->speed = SPEED_1000;
>> + else if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_25)
>> + state->speed = SPEED_100;
>> + else if (clk_spd == GMAC_PHYIF_CTRLSTATUS_SPEED_2_5)
>> + state->speed = SPEED_10;
>> +
>> + /* FIXME: Is this even correct?
>> + * GMAC_PHYIF_CTRLSTATUS_TC = BIT(0)
>> + * GMAC_PHYIF_CTRLSTATUS_LNKMOD = BIT(16)
>> + * GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK = 1
>> + *
>> + * The result is, we test bit 0 for the duplex setting.
>> + */
>> + state->duplex = status & GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK ?
>> + DUPLEX_FULL : DUPLEX_HALF;
>
> My gut feeling is that this is/was wrong, and the LNKMOD_MASK expects you've
> shifted / got the field out etc...
>
> Sneh, Abhishek, can you confirm this for us? I'd appreciate it.
>
The status needs to be checked against Bit 16 which is GMAC_PHYIF_CTRLSTATUS_LNKMOD to determine the Duplex.
In the above logic the status is checked against GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK (value 1) which actually determines
the link is up/down
//Correct logic
state->duplex = status & GMAC_PHYIF_CTRLSTATUS_LNKMOD ?
DUPLEX_FULL : DUPLEX_HALF;
Bit 16 LNKMOD Link Mode
This bit indicates the current mode of operation of the link.
0x0: HDUPLX
0x1: FDUPLX
Bit 1 LUD Link Up or Down
This bit indicates whether the link is up or down during transmission of configuration in the RGMII, SGMII, or SMII interface.
0x0: LINKDOWN
0x1: LINKUP
> I need to run for the evening soon, but tested taking sa8775p-ride,
> making it have managed = "in-band-status", and remove the
> HAS_INTEGRATED_PCS stuff... and then all of a sudden the link acts as if
> its half duplex (but works otherwise I think? need to test more
> throughly):
>
> [ 11.458385] qcom-ethqos 23040000.ethernet end0: Link is Up - 1Gbps/Half - flow control rx/tx
>
> So I think it is probably wrong, and if I understand
> correctly most of the special treatment for the qualcomm driver can be
> dropped? I know it was mentioned that all the 2.5 Gpbs stuff is a
> Qualcomm addition (and that you're chasing down some answers with the
> hardware team), but hopefully you can confirm the register's bitfields
> for us here!
>
>
>>
>> - x->pcs_link = 1;
>> + dwmac_pcs_get_state(hw, state, GMAC_PCS_BASE);
>> +}
>>
>> - speed_value = ((status & GMAC_PHYIF_CTRLSTATUS_SPEED) >>
>> - GMAC_PHYIF_CTRLSTATUS_SPEED_SHIFT);
>> - if (speed_value == GMAC_PHYIF_CTRLSTATUS_SPEED_125)
>> - x->pcs_speed = SPEED_1000;
>> - else if (speed_value == GMAC_PHYIF_CTRLSTATUS_SPEED_25)
>> - x->pcs_speed = SPEED_100;
>> - else
>> - x->pcs_speed = SPEED_10;
>> +static const struct phylink_pcs_ops dwmac4_mii_pcs_ops = {
>> + .pcs_validate = dwmac4_mii_pcs_validate,
>> + .pcs_config = dwmac4_mii_pcs_config,
>> + .pcs_get_state = dwmac4_mii_pcs_get_state,
>> +};
>>
>> - x->pcs_duplex = (status & GMAC_PHYIF_CTRLSTATUS_LNKMOD_MASK);
>> +static struct phylink_pcs *
>> +dwmac4_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;
>>
>> - 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");
>> - }
>> + return NULL;
>> }
>>
>> static int dwmac4_irq_mtl_status(struct stmmac_priv *priv,
>> @@ -867,8 +902,12 @@ static int dwmac4_irq_status(struct mac_device_info *hw,
>> }
>>
>> dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
>> - if (intr_status & PCS_RGSMIIIS_IRQ)
>> - dwmac4_phystatus(ioaddr, x);
>> + if (intr_status & PCS_RGSMIIIS_IRQ) {
>> + /* TODO Dummy-read to clear the IRQ status */
>> + readl(ioaddr + GMAC_PHYIF_CONTROL_STATUS);
>> + phylink_pcs_change(&hw->mac_pcs, false);
>
> I'll just highlight it here, but same question as the dwmac1000 change.
> We can discuss that question there, and if anything changes apply it
> here too. It is probably fine and I'm fussing over nothing.
>
>> + x->irq_rgmii_n++;
>> + }
>>
>> return ret;
>> }
>> @@ -1186,6 +1225,7 @@ static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw)
>> const struct stmmac_ops dwmac4_ops = {
>> .core_init = dwmac4_core_init,
>> .update_caps = dwmac4_update_caps,
>> + .phylink_select_pcs = dwmac4_phylink_select_pcs,
>> .set_mac = stmmac_set_mac,
>> .rx_ipc = dwmac4_rx_ipc_enable,
>> .rx_queue_enable = dwmac4_rx_queue_enable,
>> @@ -1210,7 +1250,6 @@ const struct stmmac_ops dwmac4_ops = {
>> .set_eee_timer = dwmac4_set_eee_timer,
>> .set_eee_pls = dwmac4_set_eee_pls,
>> .pcs_ctrl_ane = dwmac4_ctrl_ane,
>> - .pcs_get_adv_lp = dwmac4_get_adv_lp,
>> .debug = dwmac4_debug,
>> .set_filter = dwmac4_set_filter,
>> .set_mac_loopback = dwmac4_set_mac_loopback,
>> @@ -1230,6 +1269,7 @@ const struct stmmac_ops dwmac4_ops = {
>> const struct stmmac_ops dwmac410_ops = {
>> .core_init = dwmac4_core_init,
>> .update_caps = dwmac4_update_caps,
>> + .phylink_select_pcs = dwmac4_phylink_select_pcs,
>> .set_mac = stmmac_dwmac4_set_mac,
>> .rx_ipc = dwmac4_rx_ipc_enable,
>> .rx_queue_enable = dwmac4_rx_queue_enable,
>> @@ -1254,7 +1294,6 @@ const struct stmmac_ops dwmac410_ops = {
>> .set_eee_timer = dwmac4_set_eee_timer,
>> .set_eee_pls = dwmac4_set_eee_pls,
>> .pcs_ctrl_ane = dwmac4_ctrl_ane,
>> - .pcs_get_adv_lp = dwmac4_get_adv_lp,
>> .debug = dwmac4_debug,
>> .set_filter = dwmac4_set_filter,
>> .flex_pps_config = dwmac5_flex_pps_config,
>> @@ -1278,6 +1317,7 @@ const struct stmmac_ops dwmac410_ops = {
>> const struct stmmac_ops dwmac510_ops = {
>> .core_init = dwmac4_core_init,
>> .update_caps = dwmac4_update_caps,
>> + .phylink_select_pcs = dwmac4_phylink_select_pcs,
>> .set_mac = stmmac_dwmac4_set_mac,
>> .rx_ipc = dwmac4_rx_ipc_enable,
>> .rx_queue_enable = dwmac4_rx_queue_enable,
>> @@ -1302,7 +1342,6 @@ const struct stmmac_ops dwmac510_ops = {
>> .set_eee_timer = dwmac4_set_eee_timer,
>> .set_eee_pls = dwmac4_set_eee_pls,
>> .pcs_ctrl_ane = dwmac4_ctrl_ane,
>> - .pcs_get_adv_lp = dwmac4_get_adv_lp,
>> .debug = dwmac4_debug,
>> .set_filter = dwmac4_set_filter,
>> .safety_feat_config = dwmac5_safety_feat_config,
>> @@ -1391,5 +1430,8 @@ int dwmac4_setup(struct stmmac_priv *priv)
>> mac->mii.clk_csr_mask = GENMASK(11, 8);
>> mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr);
>>
>> + mac->mac_pcs.ops = &dwmac4_mii_pcs_ops;
>> + mac->mac_pcs.neg_mode = true;
>> +
>> return 0;
>> }
>> --
>> 2.30.2
>>
>
Powered by blists - more mailing lists