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

[Andrew Lunn <andrew@lunn.ch>]

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