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

quoted

quoted

+static int mlxbf_gige_mdio_read(struct mii_bus *bus, int phy_add,
+int

quoted

+phy_reg) {

quoted

+         struct mlxbf_gige *priv = bus->priv;

quoted

+         u32 cmd;

quoted

+         u32 ret;

quoted

+

quoted

+         /* If the lock is held by something else, drop the request.

quoted

+         * If the lock is cleared, that means the busy bit was cleared.

quoted

+         */

How can this happen? The mdio core has a mutex which prevents parallel

access?

quoted

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?

quoted

quoted

+         ret = mlxbf_gige_mdio_poll_bit(priv,
+ MLXBF_GIGE_MDIO_GW_LOCK_MASK);

quoted

+         if (ret)

quoted

+                       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