Messages in this thread | | | Date | Tue, 25 Jan 2022 16:54:59 +0200 | From | Vladimir Oltean <> | Subject | Re: [RFC PATCH v7 10/16] net: dsa: qca8k: add support for mgmt read/write in Ethernet packet |
| |
On Mon, Jan 24, 2022 at 05:48:32PM +0100, Ansuel Smith wrote: > > > +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.
| |