Re: [PATCH net-next v2 4/7] net: xilinx: axienet: Add 10G/25G (XXV) ethernet support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver advertises runtime speed switching capabilities for the 10G/25G MAC but fails to implement the logic to actually switch the MAC speed, causing a speed mismatch if the negotiated speed differs from the MAC's default.
- [High] XXV MAC falsely advertises Pause frame capabilities but does not implement flow control programming.
--

--- Patch [4]: [PATCH net-next v2 4/7] net: xilinx: axienet: Add 10G/25G (XXV) ethernet support ---
commit 8e3220d85fee82772624219e408ac95ba32403d4
Author: Suraj Gupta <[email protected]>

net: xilinx: axienet: Add 10G/25G (XXV) ethernet support

The AXI Ethernet driver currently supports only the AXI 1G Ethernet IP.
This commit adds support for the Xilinx XXV Ethernet IP which implements
a 10G/25G Ethernet MAC. It adds the XXV-specific logic in new files and
describes the MAC through an axienet_10g25g_config.

[1]: https://docs.amd.com/r/en-US/pg210-25g-ethernet

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c b/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c
> new file mode 100644
> index 0000000000000..46b48a773c15a
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c
> @@ -0,0 +1,329 @@

[ ... ]

> +static void axienet_xxv_phylink_set_capabilities(struct axienet_local *lp,
> +						 struct phylink_config *cfg)
> +{
> +	u32 core_speed;
> +	bool rtsw;
> +
> +	core_speed = axienet_ior(lp, XXV_STAT_CORE_SPEED_OFFSET);
> +	/* Bit[1:0]: 00=25G, 01=10G, 10=runtime-switchable 25G,
> +	 * 11=runtime-switchable 10G. A runtime-switchable core can operate at
> +	 * either speed, so advertise both; a fixed core advertises only its
> +	 * configured speed.
> +	 */
> +	rtsw = core_speed & XXV_STAT_CORE_SPEED_RTSW_MASK;
> +
> +	if (rtsw || (core_speed & XXV_STAT_CORE_SPEED_10G_MASK)) {
> +		cfg->mac_capabilities |= MAC_10000FD;
> +		__set_bit(PHY_INTERFACE_MODE_10GBASER,
> +			  cfg->supported_interfaces);
> +	}
> +
> +	if (rtsw || !(core_speed & XXV_STAT_CORE_SPEED_10G_MASK)) {
> +		cfg->mac_capabilities |= MAC_25000FD;
> +		__set_bit(PHY_INTERFACE_MODE_25GBASER,
> +			  cfg->supported_interfaces);
> +	}
> +}

[Severity: High]
The driver advertises both 10G and 25G capabilities for runtime switchable
cores here, creating the expectation that it will support speed switching.

However, the axienet_10g25g_config structure below omits the .mac_link_up
callback. If phylink negotiates a speed that differs from the MAC's default,
how is the new speed written to the hardware via
XXV_SWITCH_CORE_SPEED_OFFSET to prevent a mismatch?

[ ... ]

> +const struct axienet_config axienet_10g25g_config = {
> +	.sw_padding = true,
> +	.internal_pcs = true,
> +	.regs_n = XXV_REGS_N,
> +	.clk_init = axienet_10g25g_clk_init,
> +	.setoptions = axienet_xxv_setoptions,
> +	.probe_init = axienet_xxv_probe_init,
> +	.gt_reset = axienet_xxv_gt_reset,
> +	.mac_init = axienet_xxv_mac_init,
> +	.get_regs = axienet_xxv_get_regs,
> +	.phylink_set_caps = axienet_xxv_phylink_set_capabilities,
> +	.pcs_ops = &axienet_xxv_pcs_ops,
> +};

[Severity: High]
The shared axienet_probe() function unconditionally adds MAC_SYM_PAUSE and
MAC_ASYM_PAUSE to the mac_capabilities for all configurations, including the
XXV MAC.

Without a .mac_link_up callback provided here to program the hardware flow
control registers (like XXV_CONFIG_TX_FLOW_CTRL1_OFFSET), could this lead to
a silent mismatch of flow control configuration if pause frames are negotiated
with the link partner?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4