Message-ID: <CAAXyoMP-Z8aYTSZwqJpDYRVcYQ9fzEgmDuAbQd=UEGp+o5Fdjg@mail.gmail.com>
Date: Mon, 25 Aug 2025 00:38:20 +0800
From: Yangfl <mmyangfl@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Simon Horman <horms@...nel.org>, Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 3/3] net: dsa: yt921x: Add support for
 Motorcomm YT921x

On Sun, Aug 24, 2025 at 11:26 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +#define yt921x_port_is_internal(port) ((port) < 8)
> > +#define yt921x_port_is_external(port) (8 <= (port) && (port) < 9)
>
> > +#define yt921x_info_port_is_internal(info, port) \
> > +     ((info)->internal_mask & BIT(port))
> > +#define yt921x_info_port_is_external(info, port) \
> > +     ((info)->external_mask & BIT(port))
>
> Do we really need two sets of macros?
>
> And is there a third state? Can a port be not internal and not
> external?
>
> Maybe the code can just use !yt921x_info_port_is_internal(info, port)
>
>         Andrew

They are used in phylink_get_caps(), since I don't want to declare a
port which we know it does not exist on some chips. But the info_* set
might be inlined and removed since it is not used elsewhere.

Port 10 is dedicated to the internal MCU. Although I could not use it,
anyone familiar with the chips would know it's Port 10 that is neither
internal nor external.

On Sun, Aug 24, 2025 at 11:34 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +static void yt921x_smi_acquire(struct yt921x_priv *priv)
> > +{
> > +     if (priv->smi_ops->acquire)
> > +             priv->smi_ops->acquire(priv->smi_ctx);
> > +}
> > +
> > +static void yt921x_smi_release(struct yt921x_priv *priv)
> > +{
> > +     if (priv->smi_ops->release)
> > +             priv->smi_ops->release(priv->smi_ctx);
> > +}
>
> What happens if priv->smi_ops->acquire and priv->smi_ops->release are
> not implemented? Very likely, it will mostly work, but have subtle bug
> which are going to be hard to observe and find.
>
> You want bugs to be obvious, so they are quick and easy to find. The
> best way to make the bug of missing locking obvious is to jump through
> a NULL pointer and get an Opps. The stack trace will make it obvious
> what has happened.
>
> > +
> > +static int yt921x_smi_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
> > +{
> > +     return priv->smi_ops->read(priv->smi_ctx, reg, valp);
> > +}
> > +
> > +static int yt921x_smi_read_burst(struct yt921x_priv *priv, u32 reg, u32 *valp)
> > +{
> > +     int res;
> > +
> > +     yt921x_smi_acquire(priv);
> > +     res = yt921x_smi_read(priv, reg, valp);
> > +     yt921x_smi_release(priv);
>
> I don't understand the name _burst here? Why is it called
> that. Looking at other drivers, _u32 would be more common, especially
> if you have functions to read a _u16, _u8 etc.
>
>    Andrew

They are locked wrappers for their unlocked counterparts. I'd like to
name the unlocked versions __yt921x_smi_read just like __mdiobus_read,
but that was turned down in the previous version, so I have to give
the locked versions a stranger marker since we use unlocked versions
more often.

On Sun, Aug 24, 2025 at 11:51 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +static int
> > +yt921x_intif_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
> > +{
> > +     struct device *dev = to_device(priv);
> > +     u32 mask;
> > +     u32 ctrl;
> > +     u32 val;
> > +     int res;
> > +
> > +     res = yt921x_intif_wait(priv);
> > +     if (res)
> > +             return res;
> > +
> > +     mask = YT921X_MBUS_CTRL_PORT_M | YT921X_MBUS_CTRL_REG_M |
> > +            YT921X_MBUS_CTRL_OP_M;
> > +     ctrl = YT921X_MBUS_CTRL_PORT(port) | YT921X_MBUS_CTRL_REG(reg) |
> > +            YT921X_MBUS_CTRL_READ;
> > +     res = yt921x_smi_update_bits(priv, YT921X_INT_MBUS_CTRL, mask, ctrl);
> > +     if (res)
> > +             return res;
> > +     res = yt921x_smi_write(priv, YT921X_INT_MBUS_OP, YT921X_MBUS_OP_START);
> > +     if (res)
> > +             return res;
> > +
> > +     res = yt921x_intif_wait(priv);
> > +     if (res)
> > +             return res;
> > +     res = yt921x_smi_read(priv, YT921X_INT_MBUS_DIN, &val);
> > +     if (res)
> > +             return res;
> > +
> > +     if ((u16)val != val)
> > +             dev_err(dev,
> > +                     "%s: port %d, reg 0x%x: Expected u16, got 0x%08x\n",
> > +                     __func__, port, reg, val);
> > +     *valp = (u16)val;
> > +     return 0;
> > +}
>
> ...
>
> > +static int yt921x_mbus_int_read(struct mii_bus *mbus, int port, int reg)
> > +{
> > +     struct yt921x_priv *priv = mbus->priv;
> > +     u16 val;
> > +     int res;
> > +
> > +     if (port >= YT921X_PORT_NUM)
> > +             return 0xffff;
> > +
> > +     yt921x_smi_acquire(priv);
> > +     res = yt921x_intif_read(priv, port, reg, &val);
> > +     yt921x_smi_release(priv);
> > +
> > +     if (res)
> > +             return res;
> > +     return val;
> > +}
> > +
> > +static int
> > +yt921x_mbus_int_write(struct mii_bus *mbus, int port, int reg, u16 data)
> > +{
> > +     struct yt921x_priv *priv = mbus->priv;
> > +     int res;
> > +
> > +     if (port >= YT921X_PORT_NUM)
> > +             return 0;
> > +
> > +     yt921x_smi_acquire(priv);
> > +     res = yt921x_intif_write(priv, port, reg, data);
> > +     yt921x_smi_release(priv);
> > +
> > +     return res;
> > +}
>
> Going back to comment from Russell in an older version:
>
> > > I'm also concerned about the SMI locking, which looks to me like you
> > > haven't realised that the MDIO bus layer has locking which guarantees
> > > that all invocations of the MDIO bus read* and write* methods are
> > > serialised.
> >
> > The device takes two sequential u16 MDIO r/w into one op on its
> > internal 32b regs, so we need to serialise SMI ops to avoid race
> > conditions. Strictly speaking only locking the target phyaddr is
> > needed, but I think it won't hurt to lock the MDIO bus as long as I
> > don't perform busy wait while holding the bus lock.
>
> You comment is partially correct, but also wrong. As you can see here,
> you hold the lock for a number of read/writes, not just one u32 write
> split into two MDIO bus transactions.
>
> They way you currently do locking is error prone.
>
> 1) Are you sure you actually hold the lock on all paths?
>
> 2) Are you sure you hold the lock long enough for all code which
>    requires multiple reads/writes?
>
> The mv88e6xxx driver does things differently:
>
> Every function assigned to struct dsa_switch_ops first takes the lock,
> does what needs doing, and then releases the lock just before the
> return.
>
> The lowest level read/write function does a mutex_is_locked() to test
> that the lock is held. If it is not, it prints an error message.
>
> The first part makes it easy to see the lock is held, and it makes it
> clear all operations the driver is doing is covered by the lock, there
> is no need worry about two threads racing.
>
> The second part makes bugs about missing locks obvious, an error
> message is printed.
>
> Please reconsider your locking. Also, please think about, do you need
> a different lock for MDIO, I2C and SPI? Do you need the current
> acquire/release abstract?
>
>         Andrew

That's exactly what I've done: every exposed virtual function
yt921x_dsa_* and yt921x_mbus_* is guarded by an acquire/release pair
(except for few exceptions). But I might add a check for locking
status in mdio_read and mdio_write.

The driver itself does not need an explicit lock (so long as dsa
framework does not call two conflicting methods on the same port), and
holding two locks, driver lock and bus lock makes things even worse,
thus I left the acquire/release method to SMI implementations. If the
I2C / SPI / GPIO bitbang / etc interface supports native atomic
transactions, it can choose not to implement acquire/release and
leaves them NULL.

Powered by blists - more mailing lists