Re: [PATCH net-next v2 05/10] net: dsa: realtek: rtl8365mb: describe the ports of each chip

[netdev-bot+sashiko@kernel.org]

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] This patch lowers RTL8365MB_MAX_NUM_PORTS from 11 to 8.

--

net: dsa: realtek: rtl8365mb: describe the ports of each chip

This commit adds a per-chip port_mask to rtl8365mb_chip_info. It reports
fls(port_mask) to DSA as the port count and rejects devicetree ports that
the chip does not have. It also lowers RTL8365MB_MAX_NUM_PORTS from 11 to
8.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index da06f952a053d..1e6cdc61f6bd3 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -115,7 +115,7 @@
>  #define RTL8365MB_PHYADDRMAX		7
>  #define RTL8365MB_NUM_PHYREGS		32
>  #define RTL8365MB_PHYREGMAX		(RTL8365MB_NUM_PHYREGS - 1)
> -#define RTL8365MB_MAX_NUM_PORTS		11
> +#define RTL8365MB_MAX_NUM_PORTS		8

[Severity: Low]
This isn't a bug, but is "family maximum" the right description for this
value?

The commit message says:

  Lower the family maximum to 8 to match.

The constant also still sits under this heading:

  /* Family-specific data and limits */

The file header comment still describes the family as having more ports
than that, and it lists the RTL8370MB as a member:

 * In the same family of chips, some carry up to 8 user ports and up to 2
 * extension ports. Where possible this driver tries to make things generic, but
 ...
 *  - RTL8370MB

Some of the family-generic register handling was also written for port
indexes above 7. Examples are the 11-bit RTL8365MB_PORT_MASK, the EXT bit
for the trap port in CPU_CTRL, and the L2 flush code in rtl8365mb_l2.c:

	/* Now issue the flush command and wait for its completion. There are
	 * two registers for this purpose, and which one to use depends on the
	 * port number. The _EXT register is for ports 8 or higher.
	 */
	if (port < 8) {

With this change, 8 looks like the largest port count among the chips the
driver supports, not a limit for the whole family. The BUILD_BUG_ON in
PORT_MASK() reads the same way. So does the later static_assert in this
series that ties RTL8365MB_MAX_NUM_PORTS to RTL8365MB_QOS_MAX_PORTS.

At the end of the series, the header comment and the placement of the
constant are still unchanged.

Would it be clearer to rename the constant to something that marks it as
a driver-supported maximum? Moving it out of the family-limits group and
updating the comment and commit message would also work.

>  /* Valid for the whole family except RTL8370B, which has 4160 entries.
>   * RTL8370B is mentioned in vendor code but it might not even belong
>   * to the same RTL8367C family.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-rtl8365mb-dcb-ingress-v2-0-f8e6fbdf8c99%40pengutronix.de