Message-ID: <CAAXyoMPLRHfSUGboC4SO+gBD0TdHq19fNs7AK3W2ZQnHT48gyA@mail.gmail.com>
Date: Fri, 12 Sep 2025 22:27:58 +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 v8 3/3] net: dsa: yt921x: Add support for
 Motorcomm YT921x

On Fri, Sep 12, 2025 at 8:56 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +static void yt921x_reg_mdio_verify(u32 reg, u16 val, bool lo)
> > +{
> > +     const char *desc;
> > +
> > +     switch (val) {
> > +     case 0xfade:
> > +             desc = "which is likely from a non-existent register";
> > +             break;
> > +     case 0xdead:
> > +             desc = "which is likely a data race condition";
> > +             break;
>
> Where did these two values come from? Are they documented in the datasheet?
>

I don't have the comprehensive datasheet and it (along with other
parts) is not mentioned in the documents I have, so I dug them out by
testing.

> > +     default:
> > +             return;
> > +     }
> > +
> > +     /* Skip registers which are likely to have any valid values */
> > +     switch (reg) {
> > +     case YT921X_MAC_ADDR_HI2:
> > +     case YT921X_MAC_ADDR_LO4:
> > +     case YT921X_FDB_OUT0:
> > +     case YT921X_FDB_OUT1:
> > +             return;
> > +     }
> > +
> > +     pr_warn("%s: Read 0x%x at 0x%x %s32, %s; "
> > +             "consider reporting a bug if this happens again\n",
> > +             __func__, val, reg, lo ? "lo" : "hi", desc);
>
> You probably have a warning from checkpatch about pr_warn. Ideally you
> want to give an indication which device has triggered this, making use
> of a struct device. You might want to include that in context.
>

I don't want to introduce priv struct layout into register
implementations, which makes unnecessary dependency between functions.
The warning will not appear if no bugs exist, and it can still be
easily identified by its function name.

> > +static int
> > +yt921x_intif_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
> > +{
> > +     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;
>
> You don't treat this as an error, you don't return -EIO or -EPROTO etc.
> So maybe this should be dev_info() or dev_dbg().
>
> > +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. Also that is
what dsa_user_phy_write() returns for a non-existing port.

> > +yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
> > +{
> > +     struct device *dev = to_device(priv);
> > +     struct mii_bus *mbus;
> > +     int res;
> > +
> > +     if (!mnp)
> > +             res = devm_mdiobus_register(dev, mbus);
> > +     else
> > +             res = devm_of_mdiobus_register(dev, mbus, mnp);
>
> You can call devm_of_mdiobus_register() with a NULL pointer for the
> OF, and it will do the correct thing.
>
> > +static int yt921x_extif_wait(struct yt921x_priv *priv)
> > +{
> > +     u32 val;
> > +     int res;
> > +
> > +     res = yt921x_reg_read(priv, YT921X_EXT_MBUS_OP, &val);
> > +     if (res)
> > +             return res;
> > +     if ((val & YT921X_MBUS_OP_START) != 0) {
> > +             res = read_poll_timeout(yt921x_reg_read, res,
> > +                                     (val & YT921X_MBUS_OP_START) == 0,
> > +                                     YT921X_POLL_SLEEP_US,
> > +                                     YT921X_POLL_TIMEOUT_US,
> > +                                     true, priv, YT921X_EXT_MBUS_OP, &val);
> > +             if (res)
> > +                     return res;
> > +     }
> > +
> > +     return 0;
>
> In mv88e6xxx, we have the generic mv88e6xxx_wait_mask() and on top of
> that mv88e6xxx_wait_bit(). That allows us to have register specific
> wait functions as one liners. Please consider something similar.
>
> > +static int yt921x_mib_read(struct yt921x_priv *priv, int port, void *data)
> > +{
>
> As far as i can see, data is always a pointer to struct
> yt921x_mib_raw. I would be better to not have the void in the middle.
> It also makes it clearer what assumption you are making about the
> layout of that structure.
>
> > +     unsigned char *buf = data;
> > +     int res = 0;
> > +
> > +     for (size_t i = 0; i < sizeof(struct yt921x_mib_raw);
> > +          i += sizeof(u32)) {
> > +             res = yt921x_reg_read(priv, YT921X_MIBn_DATA0(port) + i,
> > +                                   (u32 *)&buf[i]);
> > +             if (res)
> > +                     break;
> > +     }
> > +     return res;
> > +}
> > +
> > +static void yt921x_poll_mib(struct work_struct *work)
> > +{
> > +     struct yt921x_port *pp = container_of_const(work, struct yt921x_port,
> > +                                                 mib_read.work);
> > +     struct yt921x_priv *priv = (void *)(pp - pp->index) -
> > +                                offsetof(struct yt921x_priv, ports);
>
> Can you make container_of() work for this?
>

Impossible, also see ar9331_sw_port_to_priv().

> > +     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);
> > +             u64 *valp = &((u64 *)mib)[i];
> > +             u64 newval;
> > +
> > +             if (desc->size > 1) {
> > +                     newval = ((u64)rawp[0] << 32) | rawp[1];
> > +             } else {
> > +                     newval = (*valp & ~(u64)U32_MAX) | *rawp;
> > +                     if (*rawp < (u32)*valp)
> > +                             newval += (u64)1 << 32;
> > +             }
>
> There are way too many casts here. Think about your types, and how you
> can remove some of these casts. In general, casts are bad, and should
> be avoided where possible.
>

Some casts are necessary for shifting operations, otherwise an error
will pop out for shift overflow.

Others are just synonyms of val & 0xffffffff. I'd consider casts are
neater than magic numbers.

> > +static void
> > +yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
> > +{
> > +     struct yt921x_priv *priv = to_yt921x_priv(ds);
> > +     struct yt921x_port *pp = &priv->ports[port];
> > +     struct yt921x_mib *mib = &pp->mib;
> > +     size_t j;
> > +
> > +     spin_lock(&pp->stats_lock);
> > +
> > +     j = 0;
> > +     for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> > +             const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> > +
> > +             if (!desc->unstructured)
> > +                     continue;
> > +
> > +             data[j] = ((u64 *)mib)[i];
> > +             j++;
> > +     }
> >
>
> ethtool APIs are called in a context where you can block. So it would
> be good to updated the statistics first before copying them. You just
> need to think about your locking in case the worker is running.
>
> > +static int yt921x_dsa_get_sset_count(struct dsa_switch *ds, int port, int sset)
> > +{
> > +     int cnt;
> > +
> > +     if (sset != ETH_SS_STATS)
> > +             return 0;
> > +
> > +     cnt = 0;
>
> Please do the zeroing above when you declare the local variable.
>
> > +     for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> > +             const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> > +
> > +             if (desc->unstructured)
> > +                     cnt++;
> > +     }
> > +
> > +     return cnt;
> > +}
>
> > +static int
> > +yt921x_set_eee(struct yt921x_priv *priv, int port, struct ethtool_keee *e)
> > +{
>
> > +     /* Enable / disable port EEE */
> > +     res = yt921x_reg_toggle_bits(priv, YT921X_EEE_CTRL,
> > +                                  YT921X_EEE_CTRL_ENn(port), enable);
> > +     if (res)
> > +             return res;
> > +     res = yt921x_reg_toggle_bits(priv, YT921X_EEEn_VAL(port),
> > +                                  YT921X_EEE_VAL_DATA, enable);
>
> How do these two different registers differ? Why are there two of
> them? Maybe add a comment to explain this.
>

Datasheet gives no explanation here too, so this is a carbon copy of the sample
code. I'm also confused too.

> > +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.

> > +static int
> > +yt921x_dsa_port_mirror_add(struct dsa_switch *ds, int port,
> > +                        struct dsa_mall_mirror_tc_entry *mirror,
> > +                        bool ingress, struct netlink_ext_ack *extack)
> > +{
> > +     struct yt921x_priv *priv = to_yt921x_priv(ds);
> > +     u32 ctrl;
> > +     u32 val;
> > +     int res;
> > +
> > +     yt921x_reg_lock(priv);
> > +     do {
> > +             u32 srcs;
> > +             u32 dst;
> > +
> > +             if (ingress)
> > +                     srcs = YT921X_MIRROR_IGR_PORTn(port);
> > +             else
> > +                     srcs = YT921X_MIRROR_EGR_PORTn(port);
> > +             dst = YT921X_MIRROR_PORT(mirror->to_local_port);
> > +
> > +             res = yt921x_reg_read(priv, YT921X_MIRROR, &val);
> > +             if (res)
> > +                     break;
> > +
> > +             /* other mirror tasks & different dst port -> conflict */
> > +             if ((val & ~srcs & (YT921X_MIRROR_EGR_PORTS_M |
> > +                                 YT921X_MIRROR_IGR_PORTS_M)) != 0 &&
> > +                 (val & YT921X_MIRROR_PORT_M) != dst) {
> > +                     NL_SET_ERR_MSG_MOD(extack,
> > +                                        "Sniffer port is already configured,"
> > +                                        " delete existing rules & retry");
> > +                     res = -EBUSY;
> > +                     break;
> > +             }
> > +
> > +             ctrl = val & ~YT921X_MIRROR_PORT_M;
> > +             ctrl |= srcs;
> > +             ctrl |= dst;
> > +
> > +             if (ctrl != val)
> > +                     res = yt921x_reg_write(priv, YT921X_MIRROR, ctrl);
> > +     } while (0);
>
> What does a while (0) loop bring you here?
>

break statement instead of goto err.

>
>     Andrew
>
> ---
> pw-bot: cr

Powered by blists - more mailing lists