netdev - Re: [PATCH net-next v6 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAXyoMN1f-z4sMgugnXy=6ComkfX6vGhGSzwbC0kMSJNG6aQ3Q@mail.gmail.com>
Date: Sun, 24 Aug 2025 17:31:03 +0800
From: Yangfl <mmyangfl@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>, 
	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>, 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 4:42 PM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> Hi,
>
> Thanks for fixing the major phylink implementation errors. There are
> further issues that need addressing.
>
> On Sun, Aug 24, 2025 at 08:51:11AM +0800, David Yang wrote:
> > +/******** hardware definitions ********/
>
> ...
>
> > +#define YT921X_REG_END                       0x400000  /* as long as reg space is below this */
>
> Please consider moving the above register definitions to, e.g.
> drivers/net/dsa/yt921x-hw.h Also consider whether some of the below
> should also be moved there.
>
> > +#define YT921X_TAG_LEN                       8
> > +
> > +#define YT921X_EDATA_EXTMODE         0xfb
> > +#define YT921X_EDATA_LEN             0x100
> > +
> > +#define YT921X_FDB_NUM               4096
> >
> > +enum yt921x_fdb_entry_status {
> > +     YT921X_FDB_ENTRY_STATUS_INVALID = 0,
> > +     YT921X_FDB_ENTRY_STATUS_MIN_TIME = 1,
> > +     YT921X_FDB_ENTRY_STATUS_MOVE_AGING_MAX_TIME = 3,
> > +     YT921X_FDB_ENTRY_STATUS_MAX_TIME = 5,
> > +     YT921X_FDB_ENTRY_STATUS_PENDING = 6,
> > +     YT921X_FDB_ENTRY_STATUS_STATIC = 7,
> > +};
> > +
> > +#define YT921X_PVID_DEFAULT          1
> > +
> > +#define YT921X_FRAME_SIZE_MAX                0x2400  /* 9216 */
> > +
> > +#define YT921X_RST_DELAY_US          10000
> > +
> > +struct yt921x_mib_desc {
> > +     unsigned int size;
> > +     unsigned int offset;
> > +     const char *name;
> > +};
>
> Maybe consider moving the struct definitions (of which there are
> several) to drivers/net/dsa/yt921x.h ?

I checked other drivers under dsa and most of them use a single header
for both register and struct definitions, so it might be a more common
practice?

>
> > +/******** eee ********/
> > +
> > +static int
> > +yt921x_set_eee(struct yt921x_priv *priv, int port, struct ethtool_keee *e)
> > +{
> > +     struct device *dev = to_device(priv);
> > +     bool enable = e->eee_enabled;
> > +     u16 new_mask;
> > +     int res;
> > +
> > +     dev_dbg(dev, "%s: port %d, enable %d\n", __func__, port, enable);
> > +
> > +     /* Enable / disable global EEE */
> > +     new_mask = priv->eee_ports_mask;
> > +     new_mask &= ~BIT(port);
> > +     new_mask |= !enable ? 0 : BIT(port);
> > +
> > +     if (!!new_mask != !!priv->eee_ports_mask) {
> > +             dev_dbg(dev, "%s: toggle %d\n", __func__, !!new_mask);
> > +
> > +             res = yt921x_smi_toggle_bits(priv, YT921X_PON_STRAP_FUNC,
> > +                                          YT921X_PON_STRAP_EEE, !!new_mask);
> > +             if (res)
> > +                     return res;
> > +             res = yt921x_smi_toggle_bits(priv, YT921X_PON_STRAP_VAL,
> > +                                          YT921X_PON_STRAP_EEE, !!new_mask);
> > +             if (res)
> > +                     return res;
>
> Here, if EEE is completely disabled, you clear the YT921X_PON_STRAP_EEE
> bit...
>
> > +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;
>
> ... and if this bit is clear, you report that EEE is unsupported by the
> device - which means the device has no hardware for EEE support, and
> the ethtool EEE operations will be blocked and return -EOPNOTSUPP. This
> means that once all ports have EEE disabled, EEE can not be re-enabled
> except through hardware reset.
>
> Please see the code in net/dsa/user.c::dsa_user_set_eee().

priv->pon_strap_cap is read from a different readonly register, and
cached globally at startup (yt921x_detect) to reduce SMI ops. It
should not affect reported EEE status.