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

[David Yang]

On Wed, Aug 19, 2026 at 1:10 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > 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.

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

> [ ... ]
>
> > 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.

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