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

[Jakub Kicinski <kuba@kernel.org>]

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.

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

[ ... ]

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

[ ... ]

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

[ ... ]

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

[ ... ]