Re: [PATCH net-next] net: lan969x: correct RGMII port mapping index

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] net: lan969x: the patch does not achieve its stated purpose —
  the new DEVRGMII instance index it returns is never paired with…

--

net: lan969x: correct RGMII port mapping index

This adds an RGMII case to lan969x_port_dev_mapping() so that ports 28 and
29 return DEVRGMII instance indices 0 and 1 instead of the physical port
number. The helper is installed as sparx5_ops::get_port_dev_index and is
read through sparx5_port_dev_index().

> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> index f3a9c71bea36a..bf5320c7a6ca8 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> @@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
>  
>  static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
>  {
> +	if (lan969x_port_is_rgmii(port)) {
> +		switch (port) {
> +		case 28:
> +			return 0;
> +		case 29:
> +			return 1;
> +		}
> +	}
> +

[Severity: High]

Can this new index ever reach DEVRGMII register space?  The value returned
by lan969x_port_dev_mapping() is only read through sparx5_port_dev_index()
and is always combined with a target coming from sparx5_to_high_dev() or
sparx5_to_pcs_dev(), and neither of those has an RGMII case:

drivers/net/ethernet/microchip/sparx5/sparx5_port.h:
static inline u32 sparx5_to_high_dev(struct sparx5 *sparx5, int port)
{
	const struct sparx5_ops *ops = sparx5->data->ops;

	if (ops->is_port_5g(port))
		return TARGET_DEV5G;
	if (ops->is_port_10g(port))
		return TARGET_DEV10G;
	return TARGET_DEV25G;
}

So for lan969x ports 28 and 29 the target stays TARGET_DEV25G (or
TARGET_PCS25G_BR), never TARGET_DEVRGMII.

The base then comes from an unchecked array index:

drivers/net/ethernet/microchip/sparx5/sparx5_main.h:spx5_inst_get() {
	return sparx5->regs[id + tinst];
}

With the new index that is regs[TARGET_DEV25G + 0] and
regs[TARGET_DEV25G + 1].  The lan969x iomap in lan969x_main_iomap[] has no
TARGET_DEV25G or TARGET_PCS25G_BR entries at all, so both slots are NULL.
Before the patch, index 28 resolved to regs[TARGET_DEV25G + 28], which
lands on a mapped (though unrelated) DEV2G5 base.

Does that mean the commit message claim

    So, check if requested port index is an RGMII port and return the correct
    DEVRGMII index.

is not reflected in any code path, since no consumer pairs this index with
TARGET_DEVRGMII?

The real DEVRGMII and HSIO_WRAP accesses compute the instance locally and do
not use this helper:

drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c:
#define RGMII_PORT_IDX(port) ((port)->portno - LAN969X_RGMII_PORT_START_IDX)

used by lan969x_rgmii_port_device_config(), lan969x_rgmii_tx_clk_config(),
lan969x_rgmii_delay_config() and lan969x_rgmii_gpio_config().  Should the
two sources of the same index be folded into one helper instead?

There is also a latent NULL __iomem base introduced here.  In
sparx5_port_init() the devinst and pcsinst pointers are computed for RGMII
ports too:

	devinst = spx5_inst_get(sparx5, devhigh, pix);
	pcsinst = spx5_inst_get(sparx5, pcs, pix);
	...
	if (ops->is_port_rgmii(port->portno))
		return 0; /* RGMII device - nothing more to configure */

They are now NULL rather than a mapped base, and only the is_port_rgmii()
early return keeps them from being dereferenced.  The other consumers of the
pair are gated by unrelated conditions today: sparx5_port_max_tags_set()
runs only after that early return, sparx5_port_disable() and
sparx5_port_pcs_high_set() are reached via sparx5_pcs_config() where
sparx5_phylink_mac_select_pcs() returns NULL for RGMII interface modes, and
the ethtool device stat paths are gated by
sparx5_is_baser(port->conf.portmode).  Would it be safer to also teach the
target selection side about RGMII, for instance returning TARGET_DEVRGMII
for is_port_rgmii() ports, so the index and the target stay consistent?

If this is intended as preparatory work for a later change, could the commit
message say so and name the dependent patch?  A Fixes: tag also seems to be
missing for the addressing bug the changelog describes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922125112.2111256-1-robert.marko%40sartura.hr