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

[Asmaa Mnebhi <Asmaa@mellanox.com>]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, July 31, 2020 3:55 PM
> To: Asmaa Mnebhi <Asmaa@mellanox.com>
> Cc: David Thompson <dthompson@mellanox.com>;
> netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; Jiri
> Pirko <jiri@mellanox.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.

My apologies! 

> 
> > > +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?

Ah ok! There is a HW entity (called YU) within the BlueField which is connected to the PHY device.
I think the YU is what you are calling "backdoor" here. The YU contains several registers which control reads/writes
To the PHY. So it is like an extra layer for reading MDIO registers. One of the YU registers is the gateway register (aka GW or
MLXBF_GIGE_MDIO_GW_OFFSET in the code). If the GW register's LOCK bit is not cleared, we cannot write anything to the actual PHY MDIO registers.
Did I answer your question?

> 
> > > +         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?

I think I misunderstood your comment. Did you ask why we are polling here? Or that we shouldn't be returning -EBUSY?
> 
>      Andrew