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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: dsa: motorcomm: use fixed-width
  GENMASK_U*() helpers" does not describe the diff correctly.

--

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

This patch replaces several GENMASK() uses in the motorcomm DSA driver
with GENMASK_U16() and GENMASK_U32(). It touches the internal_mask
entries in yt921x_infos[], the ctrl value in yt921x_chip_setup_dsa()
and the phy_mask computation in yt921x_mbus_int_init(). It is described
as a type 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 affected
> internal_mask entries in chip.c and the YT921X_MBUS_CTRL_*
> masks used by mdio_bus.c to fixed-width helpers.

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

The YT921X_MBUS_CTRL_* masks are defined in
drivers/net/dsa/motorcomm/mdio_bus.h. This patch does not touch that
file, and the masks still use plain GENMASK() after the patch:

#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 */

The only change in mdio_bus.c is the mii_bus phy_mask in
yt921x_mbus_int_init(). That mask is computed from a runtime value and
is not one of the YT921X_MBUS_CTRL_* masks:

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

In chip.c, the changes also go beyond the internal_mask entries.
yt921x_chip_setup_dsa() converts the local register value written to
YT921X_FILTER_UNK_UCAST and YT921X_FILTER_UNK_MCAST:

-	ctrl = GENMASK(10, 0);
+	ctrl = GENMASK_U32(10, 0);

Could the message be reworded to describe the phy_mask and ctrl
changes? Alternatively, should the mdio_bus.h conversion be added as
the message describes?

The description and the diff disagree, so was the Reviewed-by tag
perhaps given on an earlier revision with different content?

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