netdev - Re: [PATCH net-next v8 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <47ec4174-ca82-42f6-be52-f28e75c9f007@lunn.ch>
Date: Fri, 12 Sep 2025 17:38:07 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Yangfl <mmyangfl@...il.com>
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 v8 3/3] net: dsa: yt921x: Add support for
 Motorcomm YT921x

> > > +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;
> >
> > -ENODEV.
> >
> 
> mdio-tools complains a lot when returning an error code.

It should. There is no device there, so its an error.

> Also that is
> what dsa_user_phy_write() returns for a non-existing port.

I would say that is not ideal. If you are trying to write to a PHY
which does not exist, you want to know about it.

> > > +     unsigned long delay = YT921X_STATS_INTERVAL_JIFFIES;
> > > +     struct device *dev = to_device(priv);
> > > +     struct yt921x_mib *mib = &pp->mib;
> > > +     struct yt921x_mib_raw raw;
> > > +     int port = pp->index;
> > > +     int res;
> > > +
> > > +     yt921x_reg_lock(priv);
> > > +     res = yt921x_mib_read(priv, port, &raw);
> > > +     yt921x_reg_unlock(priv);
> > > +
> > > +     if (res) {
> > > +             dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",
> > > +                     port, res);
> > > +             delay *= 4;
> > > +             goto end;
> > > +     }
> > > +
> > > +     spin_lock(&pp->stats_lock);
> > > +
> > > +     /* Handle overflow of 32bit MIBs */
> > > +     for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> > > +             const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> > > +             u32 *rawp = (u32 *)((u8 *)&raw + desc->offset);

One thing which might helper is make yt921x_mib_raw and a yt921x_mib
union. List all the fields, but also have a u32 array. You might also
consider a u64 array, so you don't need to construct a u64 from two
u32. But you need to check alignment.

> > > +static bool yt921x_dsa_support_eee(struct dsa_switch *ds, int port)
> > > +{
> > > +     struct yt921x_priv *priv = to_yt921x_priv(ds);
> > > +
> > > +     return (priv->pon_strap_cap & YT921X_PON_STRAP_EEE) != 0;
> >
> > What does the strapping actually tell you?
> >
> 
> Whether EEE capability is present.

This is really a strapping? A resistor pulling the pin high/low?

The silicon itself should know if it has EEE support or not. Strapping
is normally used for configuration, what are the reset defaults. So i
suspect this strapping says the design of the board would like EEE
on/off by default, not that the silicon supports EEE. And the user
should have the choice to enable/disable EEE, independent of the board
designs defaults.

> > What does a while (0) loop bring you here?
> >
> 
> break statement instead of goto err.

Which you only seem to do once, here, and nowhere else. And look at
other drivers, how many use a while (0) loop like this?

goto err is the standard way to do this.

	Andrew