Re: [PATCH net-next v16 3/5] net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message for "net: dsa: motorcomm: use fixed-width
  GENMASK_U*() helpers" does not match the diff, in three ways.

--

net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

This commit changes two GENMASK() uses in the YT921x DSA driver to
GENMASK_U32(): the FILTER_UNK value in yt921x_chip_setup_dsa(), and the
mii_bus phy_mask in yt921x_mbus_int_init(). The commit message describes
it as a type-only cleanup with no functional change.

> Some masks assigned to u16 and u32 fields are defined with
> GENMASK(),which is an unsigned long constant and wider than
> the field type on 64-bit builds. Convert the YT921X_MBUS_CTRL_*
> masks used by mdio_bus.c to fixed-width helpers.
>
> This is a type cleanup only, with no functional change. Other
> GENMASK() uses in the driver are intentionally left untouched.

[Severity: Medium]
Does this description match the diff?

The patch doesn't modify drivers/net/dsa/motorcomm/mdio_bus.h, and the
YT921X_MBUS_CTRL_* masks there still use plain GENMASK():

#define  YT921X_MBUS_CTRL_PORT_M		GENMASK(25, 21)
#define  YT921X_MBUS_CTRL_REG_M			GENMASK(20, 16)
#define  YT921X_MBUS_CTRL_TYPE_M		GENMASK(11, 8)  /* wild guess */
#define  YT921X_MBUS_CTRL_OP_M			GENMASK(3, 2)  /* wild guess */

They are still combined into u32 locals in yt921x_intif_read(),
yt921x_intif_write(), yt921x_extif_read() and yt921x_extif_write(). For
example:

	mask = YT921X_MBUS_CTRL_PORT_M | YT921X_MBUS_CTRL_REG_M |
	       YT921X_MBUS_CTRL_OP_M;

The only change in mdio_bus.c is the phy_mask expression in
yt921x_mbus_int_init(), and that is not a YT921X_MBUS_CTRL_* mask:

	mbus->phy_mask = ~GENMASK_U32(max_ports - 1, 0);

Later patches in the series don't seem to convert these masks either.

The message also says other GENMASK() uses are left untouched, but the
patch changes the open-coded value in yt921x_chip_setup_dsa() in chip.c:

	ctrl = GENMASK_U32(10, 0);
	res = yt921x_reg_write(priv, YT921X_FILTER_UNK_UCAST, ctrl);

The message doesn't mention this change. The named mask for the same
field, YT921X_FILTER_PORTS_M in chip.h, is still GENMASK(10, 0).

Both changed sites are u32, so the reference to "u16 and u32 fields"
doesn't match the diff either.

The message may describe a different version of this patch. Could it be
rewritten to describe the two conversions that are actually made? The
other option would be to convert the mdio_bus.h masks as described, and
then drop or explain the chip.c hunk.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020111115.3072180-1-kyle.switch%40motor-comm.com