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

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: motorcomm: Split SMI module

This moves the yt921x SMI register accessors out of chip.c into a new
smi.c/smi.h pair and adds smi.o to the module objects.  The helpers gain
external linkage, and the thin wrappers around them become static inline in
the new header.

A few comments and questions below.  The chip.c and Makefile hunks are
snipped since the comments all land on the new files.

[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?

[ ... ]

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

[ ... ]

[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?

[ ... ]

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

+
+/* yt921x_regs_*() are exported for linkage only. Do not use them directly. Use
+ * yt921x_reg64_*() or yt921x_reg96_*() instead.
+ */
+
+int yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
+		     unsigned int num_regs);

[Severity: Low]
Two things about this contract.

The comment above says "Here we require full read and write operations over
these registers", and this one says to use yt921x_reg64_*() or
yt921x_reg96_*() instead of the raw helpers.  But the wrapper set is write
side only: yt921x_reg64_write(), yt921x_reg64_update_bits(),
yt921x_reg64_clear_bits() and yt921x_reg96_write().  There is no
yt921x_reg64_read() or yt921x_reg96_read(), so a module that wants the
required full read of something like VLANn_CTRL has no permitted call.
Should a read wrapper be added along with the split?

Second, yt921x_regs_read() is declared here "for linkage only", but its only
callers are yt921x_regs_update_bits() and yt921x_regs_clear_bits() in smi.c
itself, both at the time of this commit and at the end of the series.  Can it
stay static in smi.c rather than becoming a global symbol?

[ ... ]