Re: [PATCH net-next 1/2] net: pcs: rzn1-miic: Make usage of miic_port_max consistent

[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] The commit message describes this patch as a consistency cleanup
  ("Make usage of miic_port_max consistent").

Pre-existing issues:
- [Medium] In the reviewed commit, miic_parse_dt() reads the per-
  converter DT "reg" into `int port` and never checks it against the…

--

net: pcs: rzn1-miic: Make usage of miic_port_max consistent

This patch makes miic_port_max always mean a port count rather than the
last port number. It adds a MIIC_PORT_END() helper for the last valid port
and uses it for the range check in miic_create(). The per-SoC values now
come from the index_to_string tables.

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index cb74861e823c8..62daaf76e0525 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -59,6 +59,8 @@
>  
>  #define MIIC_MAX_NUM_RSTS		2
>  
> +#define MIIC_PORT_END(x) ((x)->miic_port_start + (x)->miic_port_max - 1)
> +
>  /**
>   * struct modctrl_match - Matching table entry for  convctrl configuration
>   *			  See section 8.2.1 of manual.

[ ... ]

> @@ -482,7 +484,7 @@ struct phylink_pcs *miic_create(struct device *dev, struct device_node *np)
>  
>  	miic = platform_get_drvdata(pdev);
>  	of_data = miic->of_data;
> -	if (port > of_data->miic_port_max || port < of_data->miic_port_start) {
> +	if (port > MIIC_PORT_END(of_data) || port < of_data->miic_port_start) {
>  		put_device(&pdev->dev);
>  		return ERR_PTR(-EINVAL);
>  	}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Now that
MIIC_PORT_END() exists, should miic_parse_dt() use it as well?

miic_parse_dt() reads each converter's reg into int port and uses it with
no range check:

drivers/net/pcs/pcs-rzn1-miic.c:miic_parse_dt() {
    ...
	for_each_available_child_of_node(np, conv) {
		if (of_property_read_u32(conv, "reg", &port))
			continue;
		...
		/* Adjust for 0 based index */
		dt_val[port + !miic->of_data->miic_port_start] = conf;
		...
		miic_configure_phy_link(miic, conf, port, active_low);
	}
    ...
}

dt_val is allocated with conf_conv_count entries (6 on RZ/N1, 5 on
RZ/T2H). On RZ/T2H, reg = 4 writes dt_val[5], one byte past the end of the
allocation. Larger or negative reg values write further out of bounds.

miic_configure_phy_link() also builds BIT(port + shift) from the same
unchecked value, and the result is written to MIIC_PHY_LINK.

The miic_probe()->miic_parse_dt() path runs before miic_create() sees the
port, so the new check here does not cover it. Only a malformed DT can
reach this, since the binding restricts reg per SoC.

A later patch in this series, "net: pcs: rzn1-miic: Validate dtb
configuration values", appears to add a port < miic_port_start ||
port > MIIC_PORT_END() check to miic_parse_dt() and fail probe with
-EINVAL. That would resolve this.

[ ... ]

> @@ -838,7 +840,7 @@ static struct miic_of_data rzt2h_miic_of_data = {
>  	.index_to_string = rzt2h_index_to_string,
>  	.index_to_string_count = ARRAY_SIZE(rzt2h_index_to_string),
>  	.miic_port_start = 0,
> -	.miic_port_max = 4,
> +	.miic_port_max = ARRAY_SIZE(rzt2h_index_to_string) - 1,
>  	.sw_mode_mask = GENMASK(2, 0),
>  	.reset_ids = rzt2h_reset_ids,
>  	.reset_count = ARRAY_SIZE(rzt2h_reset_ids),

[Severity: Low]
The subject describes this as a consistency cleanup:

  net: pcs: rzn1-miic: Make usage of miic_port_max consistent

Isn't this really a functional fix that only affects RZ/T2H and RZ/N2H?

The value here is still 4, but its meaning has changed. Before this patch,
miic_init_hw() already used it as a count:

	for (port = 0; port < miic->of_data->miic_port_max; port++) {

miic_create(), on the other hand, used it as the last valid port:

	if (port > of_data->miic_port_max || port < of_data->miic_port_start) {

So on RZ/T2H, a PCS node with reg = 4 was accepted, even though
rzt2h_index_to_string only lists CONV0..CONV3. That produced
miic_port->port = 4. The phylink ops then wrote MIIC_CONVCTRL(4) and
toggled bit 4 of CONVRST for a converter that does not exist.

After this patch, MIIC_PORT_END() is 0 + 4 - 1 = 3 on RZ/T2H, so
miic_create() now rejects reg = 4 with -EINVAL.

Nothing changes on RZ/N1. ARRAY_SIZE(index_to_string) - 1 is still 5, and
MIIC_PORT_END() is 1 + 5 - 1 = 5.

Could the commit message name the affected SoC, describe this change in
behaviour, and note that RZ/N1 is unaffected?

Should it also carry a Fixes: tag? The mismatch appears to come from
08f89e42121d ("net: pcs: rzn1-miic: Add RZ/T2H MIIC support"). That commit
added .miic_port_max = 4, while the check from c112520de041 ("net: pcs:
rzn1-miic: move port range handling into SoC data") treated the value as
the last port.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-miic-validate-dtb-v1-0-3a6db9bb75ec%40reliablecontrols.com