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

[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: use fixed-width GENMASK_U*() helpers

This patch replaces two GENMASK() uses in the motorcomm DSA driver with
GENMASK_U32(). One is in yt921x_chip_setup_dsa() in chip.c and the other
is in yt921x_mbus_int_init() in mdio_bus.c. 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 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 seem to touch
the YT921X_MBUS_CTRL_* masks at all. After this patch they are still
plain GENMASK() in drivers/net/dsa/motorcomm/mdio_bus.h:

#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 still feed the u32 mask variables 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;

No later patch in the series converts them either.

The two lines the patch does change are not MBUS_CTRL masks. In
yt921x_chip_setup_dsa(), the mask written to YT921X_FILTER_UNK_UCAST
and YT921X_FILTER_UNK_MCAST is changed:

	ctrl = GENMASK_U32(10, 0);

That seems to contradict "Other GENMASK() uses in the driver are
intentionally left untouched".

The other change is the phy_mask calculation in yt921x_mbus_int_init():

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

Also, the message refers to "masks assigned to u16 and u32 fields",
but none of the changed lines involve a u16 field.

Both changed lines give the same values as before, so runtime behaviour
does not change. Only the description is off.

Could the message be reworded to describe the FILTER_UNK and phy_mask
changes, with the "left untouched" sentence dropped? The other option
is to convert the YT921X_MBUS_CTRL_* masks in mdio_bus.h as the
message says.