netdev - Re: [PATCH net-next 1/2] net: pcs: xpcs: remove double-read of link state when using AN

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]

Date:   Thu, 16 Mar 2023 10:44:48 +0100
From:   Steen Hegelund <steen.hegelund@...rochip.com>
To:     "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Jonathan McDowell <noodles@...th.li>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        Vladimir Oltean <olteanv@...il.com>
CC:     Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: pcs: xpcs: remove double-read of link
 state when using AN

Hi Russell,


On Wed, 2023-03-15 at 14:46 +0000, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Phylink does not want the current state of the link when reading the
> PCS link state - it wants the latched state. Don't double-read the
> MII status register. Phylink will re-read as necessary to capture
> transient link-down events as of dbae3388ea9c ("net: phylink: Force
> retrigger in case of latched link-fail indicator").
> 
> The above referenced commit is a dependency for this change, and thus
> this change should not be backported to any kernel that does not
> contain the above referenced commit.
> 
> Fixes: fcb26bd2b6ca ("net: phy: Add Synopsys DesignWare XPCS MDIO module")
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> ---
>  drivers/net/pcs/pcs-xpcs.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index bc428a816719..04a685353041 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -321,7 +321,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs,
>         return 0;
>  }
> 
> -static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an)
> +static int xpcs_read_link_c73(struct dw_xpcs *xpcs)
>  {
>         bool link = true;
>         int ret;
> @@ -333,15 +333,6 @@ static int xpcs_read_link_c73(struct dw_xpcs *xpcs, bool an)
>         if (!(ret & MDIO_STAT1_LSTATUS))
>                 link = false;
> 
> -       if (an) {
> -               ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1);
> -               if (ret < 0)
> -                       return ret;
> -
> -               if (!(ret & MDIO_STAT1_LSTATUS))
> -                       link = false;
> -       }
> -
>         return link;
>  }
> 
> @@ -935,7 +926,7 @@ static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
>         int ret;
> 
>         /* Link needs to be read first ... */
> -       state->link = xpcs_read_link_c73(xpcs, state->an_enabled) > 0 ? 1 : 0;
> +       state->link = xpcs_read_link_c73(xpcs) > 0 ? 1 : 0;

Couldn't you just say:

	state->link = xpcs_read_link_c73(xpcs) > 0;

That should be a boolean, right?

> 
>         /* ... and then we check the faults. */
>         ret = xpcs_read_fault_c73(xpcs, state);
> --
> 2.30.2
> 

BR
Steen