Re: [PATCH net-next v10 2/4] net: dsa: motorcomm: Split SMI module

On Wed, Aug 19, 2026 at 1:10 AM Jakub Kicinski [off-list ref] wrote:

quoted

diff --git a/drivers/net/dsa/motorcomm/smi.c b/drivers/net/dsa/motorcomm/smi.c
new file mode 100644
index 0000000000000..164c25ef494a1
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/smi.c

@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#include <linux/iopoll.h>
+
+#include "chip.h"
+#include "smi.h"
+
+#define YT921X_POLL_SLEEP_US 10000
+#define YT921X_POLL_TIMEOUT_US       100000
+
+int yt921x_reg_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
+{
+     WARN_ON(!mutex_is_locked(&priv->reg_lock));
+
+     return priv->reg_ops->read(priv->reg_ctx, reg, valp);
+}
+
+int yt921x_reg_write(struct yt921x_priv *priv, u32 reg, u32 val)
+{
+     WARN_ON(!mutex_is_locked(&priv->reg_lock));
+
+     return priv->reg_ops->write(priv->reg_ctx, reg, val);
+}

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, since these
two lines are moved verbatim out of chip.c.

Now that yt921x_reg_read() and yt921x_reg_write() are a cross-file (and per
the commit message cross-module) API, would lockdep_assert_held() be the
better assertion here?

mutex_is_locked(&priv->reg_lock) only reports that some task owns the mutex,
not that the current one does, so a new caller that forgets to take
priv->reg_lock passes the check whenever another thread happens to hold it.

Would WARN_ON_ONCE() also be preferable to WARN_ON() on a path that runs for
every single register access?

It would be better to address it in another commit. Also it's the AI
who suggests not to include such changes.

quoted

+int
+yt921x_regs_update_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
+                     const u32 *vals, unsigned int num_regs)
+{
+     bool changed = false;
+     u32 vs[4];
+     int res;
+
+     if (WARN_ON_ONCE(num_regs > ARRAY_SIZE(vs)))
+             return -EINVAL;

[Severity: Low]
The chip.c versions guarded the vs[4] stack buffer at compile time:

        BUILD_BUG_ON(num_regs > ARRAY_SIZE(vs));

and that becomes a runtime warning here and in yt921x_regs_clear_bits().
Is the reason that num_regs is no longer a constant expression inside the
callee once the function has external linkage?

No overflow looks reachable today: the only callers go through the
yt921x_reg64_*()/yt921x_reg96_*() wrappers in smi.h with a literal 2 or 3.

Would it be worth naming this conversion in the commit message instead of
just "Minor changes are applied to fix build errors"?  The lengths of the
caller supplied masks[] and vals[] arrays also remain unchecked, which is
worth a word since the helpers are now callable from other files.

It just isn't worth it to do fancy checks to these simple helpers with
tons of macro hacks.

[ ... ]

quoted

diff --git a/drivers/net/dsa/motorcomm/smi.h b/drivers/net/dsa/motorcomm/smi.h
new file mode 100644
index 0000000000000..2995e91f3b3d1
--- /dev/null
+++ b/drivers/net/dsa/motorcomm/smi.h

@@ -0,0 +1,92 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2026 David Yang
+ */
+
+#ifndef _YT_SMI_H
+#define _YT_SMI_H

[Severity: Low]
This isn't a bug, but the guard name here is _YT_SMI_H while the sibling
private header in the same directory uses __YT921X_H.  Was the difference
intended?

We are going to use the _YT prefix for guard macros instead.

quoted

+static inline void update_ctrls_unaligned(u32 *lo, u32 *hi, u64 mask, u64 val)
+{
+     *lo &= ~lower_32_bits(mask);
+     *hi &= ~upper_32_bits(mask);
+     *lo |= lower_32_bits(val);
+     *hi |= upper_32_bits(val);
+}

[Severity: Low]
Related to the above, and also not a bug: this helper is the only symbol in
the header without the yt921x_ prefix that everything else uses, and it is
now visible to every file that includes smi.h.

Its only users are the two METER control word calls in chip.c:

        update_ctrls_unaligned(&ctrls[0], &ctrls[1],
                               YT921X_METER_CTRLab_EBS_M, ...);

so could it stay a file local static in chip.c, or get a yt921x_ prefix if
it really needs to move?

Irrelevant to this commit.