Message-ID: <6952fd42-f0b1-4509-a91e-b2e43436a873@lunn.ch>
Date: Tue, 26 Aug 2025 14:15:43 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Yangfl <mmyangfl@...il.com>
Cc: Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org,
	"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

> > > +static int yt921x_fdb_wait(struct yt921x_priv *priv, u32 *valp)
> > > +{
> > > +     struct device *dev = to_device(priv);
> > > +     u32 val;
> > > +     int res;
> > > +
> > > +     res = yt921x_smi_read(priv, YT921X_FDB_RESULT, &val);
> > > +     if (res)
> > > +             return res;
> > > +     if ((val & YT921X_FDB_RESULT_DONE) == 0) {
> > > +             yt921x_smi_release(priv);
> >
> > yuck, why is it ok to release the SMI lock here? What's the point of the
> > lock in the first place?
> >
> 
> It's the bus lock, not the driver lock. We need to release the bus
> lock when we want to sleep.

Why do you need to release the lock when you sleep? It is not a
spinlock, you can sleep while holding it. How do you prevent other fdb
operations running in parallel if you don't hold the lock?

> > > +static int
> > > +yt921x_dsa_get_eeprom(struct dsa_switch *ds, struct ethtool_eeprom *eeprom,
> > > +                   u8 *data)
> > > +{
> > > +     struct yt921x_priv *priv = to_yt921x_priv(ds);
> > > +     unsigned int i = 0;
> > > +     int res;
> > > +
> > > +     yt921x_smi_acquire(priv);
> > > +     do {
> > > +             res = yt921x_edata_wait(priv);
> > > +             if (res)
> > > +                     break;
> > > +             for (; i < eeprom->len; i++) {
> > > +                     unsigned int offset = eeprom->offset + i;
> > > +
> > > +                     res = yt921x_edata_read_cont(priv, offset, &data[i]);
> > > +                     if (res)
> > > +                             break;
> > > +             }
> > > +     } while (0);
> > > +     yt921x_smi_release(priv);
> > > +
> > > +     eeprom->len = i;
> > > +     return res;
> >
> > What is contained in this EEPROM?
> >
> 
> No description, sorry.

Even if you don't have a detailed description, can you tell us what it
can be used for?

The Marvell switches have a little application specific language which
allows you to write values into any register, and poll waiting for
events. It allows you to setup some aspects of the switch without
software. Using it is not a good idea, because DSA has no idea what
the EEPROM has done, and it could invalidate the assumptions about
reset default values.

In theory it is also possible to put code in the EEPROM for the Z80.
But i've never seen that does, probably because nobody combines DSA
with using the Z80.

> > I guess you got this snippet from mv88e6xxx_mdios_register(), which is
> > different from your case because it is an old driver which has to
> > support older device trees, before conventions changed.
> >
> > Please only register the internal MDIO bus if it is present in the
> > device tree. This simplifies the driver and dt-bindings by having a
> > single way of describing ports with internal PHYs. Also, remove the
> > ds->user_mii_bus assignment after you do that.
> >
> 
> How to access internal PHYs without registering the internal MDIO bus?

phy-handle in each port nodes pointing to the PHY. This is standard
Ethernet DT. It just makes it more verbose.

> > > +     /* chip */
> > > +     .get_eeprom_len         = yt921x_dsa_get_eeprom_len,
> > > +     .get_eeprom             = yt921x_dsa_get_eeprom,
> > > +     .preferred_default_local_cpu_port       = yt921x_dsa_preferred_default_local_cpu_port,
> > > +     .conduit_state_change   = yt921x_dsa_conduit_state_change,
> > > +     .setup                  = yt921x_dsa_setup,
> >
> > No STP state handling?
> >
> 
> Support for (M)STP was suggested for a future series in previous reviews.

It is a pretty important feature to have, otherwise your network dies
in a broadcast storm when there are loops. I personally would of added
STP before VLANS. So please don't wait too long with this feature.

> > > +/******** debug ********/
> > > +
> > > +static ssize_t
> > > +reg_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +     struct yt921x_priv *priv = dev_get_drvdata(dev);
> > > +
> > > +     if (!priv->reg_valid)
> > > +             return sprintf(buf, "0x%x: -\n", priv->reg_addr);
> > > +
> > > +     return sprintf(buf, "0x%x: 0x%08x\n", priv->reg_addr, priv->reg_val);
> > > +}
> > > +
> > > +/* Convenience sysfs attribute to read/write switch internal registers, since
> > > + * user-space tools cannot gain exclusive access to the device, which is
> > > + * required for any register operations.
> > > + */
> > > +static ssize_t
> > > +reg_store(struct device *dev, struct device_attribute *attr,
> > > +       const char *buf, size_t count)
> >
> > NACK for exposing a sysfs which allows user space to modify switch
> > registers without the driver updating its internal state.
> >
> > For reading -- also not so sure. There are many higher-level debug
> > interfaces which you should prefer implementing before resorting to raw
> > register reads. If there's any latching register in hardware, user space
> > can mess it up. What use case do you have?
> >
> 
> It is not possible to debug the device without a register sysfs, see
> previous replies for why we need to lock the bus (but not necessarily
> the MDIO bus). And if they are to modify switch registers, that's what
> they want.
> 
> So it's also kind of development leftover, but if anyone wants to
> improve the driver, they would come up with pretty the same debug
> solution as this, thus I consider it useful in the future. If that
> effort is still not appreciated, it might be wrapped inside a #ifdef
> DEBUG block so it will not be enabled to end users.

Write access in any form is definitely NACK.

For read access, please look at mv88e6xxx devlink regions, and
https://github.com/lunn/mv88e6xxx_dump

	Andrew

Powered by blists - more mailing lists