netdev - Re: [PATCH net-next] Add Mellanox BlueField Gigabit Ethernet driver

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

Message-ID: <20200731195458.GA1843538@lunn.ch>
Date:   Fri, 31 Jul 2020 21:54:58 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Asmaa Mnebhi <Asmaa@...lanox.com>
Cc:     David Thompson <dthompson@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>, Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next] Add Mellanox BlueField Gigabit Ethernet driver

On Fri, Jul 31, 2020 at 06:54:04PM +0000, Asmaa Mnebhi wrote:

Hi Asmaa

Please don't send HTML obfusticated emails to mailing lists.

> > +static int mlxbf_gige_mdio_read(struct mii_bus *bus, int phy_add, int
> 
> > +phy_reg) {
> 
> > +         struct mlxbf_gige *priv = bus->priv;
> 
> > +         u32 cmd;
> 
> > +         u32 ret;
> 
> > +
> 
> > +         /* If the lock is held by something else, drop the request.
> 
> > +         * If the lock is cleared, that means the busy bit was cleared.
> 
> > +         */
> 
>  
> 
> How can this happen? The mdio core has a mutex which prevents parallel access?
> 
>  
> 
> This is a HW Lock. It is an actual register. So another HW entity can be
> holding that lock and reading/changing the values in the HW registers.

You have not explains how that can happen? Is there something in the
driver i missed which takes a backdoor to read/write MDIO
transactions?

> > +         ret = mlxbf_gige_mdio_poll_bit(priv, MLXBF_GIGE_MDIO_GW_LOCK_MASK);
> 
> > +         if (ret)
> 
> > +                       return -EBUSY;
> 
>  
> 
> PHY drivers are not going to like that. They are not going to retry. What is
> likely to happen is that phylib moves into the ERROR state, and the PHY driver
> grinds to a halt.
> 
>  
> 
> This is a fairly quick HW transaction. So I don’t think it would cause and
> issue for the PHY drivers. In this case, we use the micrel KSZ9031. We haven’t
> seen issues.

So you have happy to debug hard to find and reproduce issues when it
does happen? Or would you like to spend a little bit of time now and
just prevent it happening at all?

     Andrew