On Mon, Jan 24, 2022 at 05:48:32PM +0100, Ansuel Smith wrote:

quoted

quoted

+static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
+{
+	struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	if (!skb)
+		return -ENOMEM;
+
+	mutex_lock(&mgmt_hdr_data->mutex);
+
+	/* Recheck mgmt_master under lock to make sure it's operational */
+	if (!priv->mgmt_master)

mutex_unlock and kfree_skb

Also, why "recheck under lock"? Why not check just under lock?

Tell me if the logic is wrong.
We use the mgmt_master (outside lock) to understand if the eth mgmt is
available. Then to make sure it's actually usable when the operation is
actually done (and to prevent any panic if the master is dropped or for
whatever reason mgmt_master is not available anymore) we do the check
another time under lock.

It's really just to save extra lock when mgmt_master is not available.
The check under lock is to handle case when the mgmt_master is removed
while a mgmt eth is pending (corner case but still worth checking).

If you have suggestions on how to handle this corner case without
introducing an extra lock in the read/write function, I would really
appreaciate it.
Now that I think about it, considering eth mgmt will be the main way and
mdio as a fallback... wonder if the extra lock is acceptable anyway.
In the near future ipq40xx will use qca8k, but will have his own regmap
functions so we they won't be affected by these extra locking.

Don't know what is worst. Extra locking when mgmt_master is not
avaialable or double check. (I assume for a cleaner code the extra lock
is preferred)

I don't think there's a hidden bug in the code (other than the one I
mentioned, which is that you don't unlock or free resources at all on
the error path, which is quite severe), but also, I don't know if this
is such a performance-sensitive operation to justify a gratuitous
"optimization".
As you mention, if the Ethernet management will be the main I/O access
method, then the common case will take a single lock. You aren't really
saving much.