Re: [net-next v24 6/7] net: mtip: Extend the L2 switch driver with management operations

Re: [net-next v24 6/7] net: mtip: Extend the L2 switch driver with management operations

[Jakub Kicinski]

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: mtip: Extend the L2 switch driver with management operations

This patch adds switch management helpers (port enable, blocking,
learning, broadcast, multicast, VLAN input/output, VLAN verification,
forced forwarding, port separation) and wires them into the switch
configuration and TX paths.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> index 212207861e13..76e503e128dd 100644
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> @@ -505,8 +505,35 @@ static void mtip_config_switch(struct switch_enet_private *fep)
>  
>  	writel(0, fep->hwp + ESW_BKLR);
>  
> +	/* Do NOT disable learning */
> +	mtip_port_learning_config(fep, 0, 0, 0);
> +	mtip_port_learning_config(fep, 1, 0, 0);
> +	mtip_port_learning_config(fep, 2, 0, 0);
> +
> +	/* Disable blocking */
> +	mtip_port_blocking_config(fep, 0, 0);
> +	mtip_port_blocking_config(fep, 1, 0);
> +	mtip_port_blocking_config(fep, 2, 0);
> +
>  	writel(MCF_ESW_IMR_TXF | MCF_ESW_IMR_RXF,
>  	       fep->hwp + ESW_IMR);
> +
> +	mtip_port_enable_config(fep, 0, 1, 1);
> +	mtip_port_enable_config(fep, 1, 1, 1);
> +	mtip_port_enable_config(fep, 2, 1, 1);
> +
> +	mtip_port_broadcast_config(fep, 0, 1);
> +	mtip_port_broadcast_config(fep, 1, 1);
> +	mtip_port_broadcast_config(fep, 2, 1);
> +
> +	/* Disable multicast receive on port 0 (MGNT) */
> +	mtip_port_multicast_config(fep, 0, 0);
> +	mtip_port_multicast_config(fep, 1, 1);
> +	mtip_port_multicast_config(fep, 2, 1);

[Low]
mtip_port_learning_config(), mtip_port_blocking_config(),
mtip_port_enable_config(), mtip_port_broadcast_config() and
mtip_port_multicast_config() all return int, but every return value here
is discarded and mtip_config_switch() itself stays void.

The arguments are compile-time constants today so validation cannot
fail, but if any of these helpers later acquire a real failure mode (for
example, hardware probing or capability gating) the switch would be
silently left in a partially configured state.  Should the return
values be checked, or should these helpers be void?

> +
> +	/* Setup VLANs to provide port separation */
> +	if (!fep->br_offload)
> +		mtip_switch_en_port_separation(fep);
>  }
>  
>  static netdev_tx_t mtip_start_xmit_port(struct sk_buff *skb,
> @@ -592,6 +619,10 @@ static netdev_tx_t mtip_start_xmit_port(struct sk_buff *skb,
>  
>  	skb_tx_timestamp(skb);
>  
> +	/* For port separation - force sending via specified port */
> +	if (!fep->br_offload && port != 0)
> +		mtip_forced_forward(fep, port, 1);
> +

[High]
This is the per-packet TX path, and mtip_forced_forward() is called for
every non-management transmit.  A few questions about that call:

The helper passes the netdev port index (1 or 2) in as the FD value, but
its body encodes that argument as a bitmap into MCF_ESW_P0FFEN_FD_MASK:

	reg |= FIELD_PREP(MCF_ESW_P0FFEN_FD_MASK, port & GENMASK(1, 0));

For port == 1 the FD field is 0b01, for port == 2 it is 0b10.  If FD is
a destination-port bitmap, does this mean port == 1 forwards to MGNT
port 0 and port == 2 forwards to the first user port?  Is the caller
passing the right value?

The function builds the register value from a fresh u32 reg = 0 and
unconditionally writes ESW_P0FFEN, so the else branch that masks off
MCF_ESW_P0FFEN_FEN never runs in practice (reg already has FEN clear).
Once this path runs once with enable == 1, who clears the FEN bit?

mtip_switch_dis_port_separation() resets PID/VRES/VIMEN/VIMSEL/VLANV/
VOMSEL but not ESW_P0FFEN.  Is forced-forward state expected to remain
asserted across reconfiguration?

>  	dev->stats.tx_bytes += skb->len;
>  	/* If this was the last BD in the ring,
>  	 * start at the beginning again.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c
> new file mode 100644
> index 000000000000..4c07861a6a3e
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c

[ ... ]

> +int mtip_vlan_input_process(struct switch_enet_private *fep,
> +			    int port, int mode, unsigned short port_vlanid,
> +			    int vlan_verify_en, int vlan_domain_num,
> +			    int vlan_domain_port)
> +{
> +	if (mode < 0 || mode > 3) {
> +		dev_err(&fep->pdev->dev,
> +			"%s: VLAN input processing mode (%d) not supported\n",
> +			__func__, mode);
> +		return -EINVAL;
> +	}
> +
> +	if (port < 0 || port > 2) {
> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not supported!\n",
> +			__func__, port);
> +		return -EINVAL;
> +	}
> +
> +	if (vlan_verify_en == 1 &&
> +	    (vlan_domain_num < 0 || vlan_domain_num > 32)) {
> +		dev_err(&fep->pdev->dev, "%s: Domain out of range\n", __func__);
> +		return -EINVAL;
> +	}

[Medium]
Is the upper bound here off by one?  ESW_VRES_BASE is 0x280 and
ESW_VRES(x) = 0x280 + 4*x, so vlan_domain_num == 32 produces offset
0x300, which is the ESW_DISCN register.  A call with
vlan_domain_num == 32 would write a VRES bitmap value into the
discard-stats register.  Should the test be > 31 (or >= 32)?

[Low]
The vlan_domain_port parameter is part of the function signature and
the public header but never referenced in the body, and all in-tree
callers pass 0.  Is it intended to control which VRES port bit is set
(currently hardcoded from port), or should the parameter be removed
until it has a use?

> +
> +	writel(FIELD_PREP(MCF_ESW_PID_VLANID_MASK, port_vlanid),
> +	       fep->hwp + ESW_PID(port));
> +	if (port == 0) {
> +		if (vlan_verify_en == 1)
> +			writel(FIELD_PREP(MCF_ESW_VRES_VLANID_MASK,
> +					  port_vlanid) | MCF_ESW_VRES_P0,
> +			       fep->hwp + ESW_VRES(vlan_domain_num));
> +
> +		writel(readl(fep->hwp + ESW_VIMEN) | MCF_ESW_VIMEN_EN0,
> +		       fep->hwp + ESW_VIMEN);
> +		writel(readl(fep->hwp + ESW_VIMSEL) |
> +		       FIELD_PREP(MCF_ESW_VIMSEL_IM0_MASK, mode),
> +		       fep->hwp + ESW_VIMSEL);

[Medium]
VIMSEL IMx and VOMSEL OMx are 2-bit fields, but the update here is
read | FIELD_PREP(MASK, mode) without first clearing the field.  Once a
non-zero mode has been programmed for a port, can a subsequent
reprogramming actually replace it?

For example, programming port 0 first with mode 1 (binary 01) and then
with mode 2 (binary 10) would yield 01 | 10 = 11 in IM0.

mtip_config_switch() runs from the switch-restart path
(mtip_open(), mtip_timeout_work(), mtip_adjust_link()) and does not
zero VIMSEL/VOMSEL beforehand, so a restart with a different mode would
hit this.  Should the helpers do reg &= ~MASK; reg |= FIELD_PREP(...)
as the sibling helpers (e.g. mtip_port_enable_config) do?

> +	} else if (port == 1) {

[ ... ]

> +int mtip_vlan_output_process(struct switch_enet_private *fep, int port,
> +			     int mode)
> +{
> +	if (port < 0 || port > 2) {
> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not supported!\n",
> +			__func__, port);
> +		return -EINVAL;
> +	}
> +
> +	if (port == 0) {
> +		writel(readl(fep->hwp + ESW_VOMSEL) |
> +		       FIELD_PREP(MCF_ESW_VOMSEL_OM0_MASK, mode),
> +		       fep->hwp + ESW_VOMSEL);

[Medium]
The same accumulating-OR pattern is used here, and there is no
validation of mode (the input variant has it).  Should mode be range
checked, and should the OMx field be cleared before being rewritten?

> +	} else if (port == 1) {

[ ... ]

> +int mtip_set_vlan_verification(struct switch_enet_private *fep, int port,
> +			       int vlan_domain_verify_en,
> +			       int vlan_discard_unknown_en)
> +{
> +	if (port < 0 || port > 2) {
> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not supported!\n",
> +			__func__, port);
> +		return -EINVAL;
> +	}
> +
> +	if (vlan_domain_verify_en == 1) {

[Low]
Both vlan_domain_verify_en and vlan_discard_unknown_en are tested for
exactly 1 and exactly 0; any other value silently performs no register
update yet the function still returns 0.

Would it be cleaner to declare these parameters bool (as several other
helpers in this file do) or to validate them and return -EINVAL on out
of range input?

> +		if (port == 0)
> +			writel(readl(fep->hwp + ESW_VLANV) | MCF_ESW_VLANV_VV0,
> +			       fep->hwp + ESW_VLANV);

[ ... ]

> +int mtip_port_multicast_config(struct switch_enet_private *fep,
> +			       int port, bool enable)
> +{
> +	u32 reg = 0;
> +
> +	if (port < 0 || port > 2) {
> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not supported\n",
> +			__func__, port);
> +		return -EINVAL;
> +	}
> +
> +	reg = readl(fep->hwp + ESW_DMCR);

[Medium]
A general question that applies to every helper added in this file:
each one performs a read-modify-write on a shared MMIO register
(ESW_BKLR, ESW_PER, ESW_DBCR, ESW_DMCR, ESW_VLANV, ESW_VIMEN,
ESW_VIMSEL, ESW_VOMSEL, ESW_VRES, ESW_IMR) without taking any lock and
without a documented locking precondition, and the helpers are exported
via mtipl2sw.h.

fep->hw_lock already exists for HW configuration but the helpers
neither take it nor call lockdep_assert_held() on it.
mtip_config_switch() itself issues three independent RMWs on the same
register for each helper (one per port).

Today the only caller is mtip_config_switch() during switch restart, so
the race is dormant.  If a future caller (sysfs/netlink/bridge offload
hook) appears, every shared-register update becomes a race.  Should
these helpers take fep->hw_lock internally, or assert it via
lockdep_assert_held()?

[ ... ]

> +int mtip_forced_forward(struct switch_enet_private *fep, int port, bool enable)
> +{
> +	u32 reg = 0;
> +
> +	if (port & ~GENMASK(1, 0)) {
> +		dev_err(&fep->pdev->dev,
> +			"%s: Forced forward for port(s): 0x%x not supported!\n",
> +			__func__, port);
> +		return -EINVAL;
> +	}
> +
> +	/* Enable Forced forwarding for port(s) */
> +	reg |= FIELD_PREP(MCF_ESW_P0FFEN_FD_MASK, port & GENMASK(1, 0));
> +
> +	if (enable)
> +		reg |= MCF_ESW_P0FFEN_FEN;
> +	else
> +		reg &= ~MCF_ESW_P0FFEN_FEN;
> +
> +	writel(reg, fep->hwp + ESW_P0FFEN);
> +	return 0;
> +}

[High]
Two more notes here in addition to the call-site question above:

The validation accepts port values 0..3 via port & ~GENMASK(1, 0) while
every sibling helper in this file rejects port > 2.  Which convention
is intended for the API?

The function unconditionally overwrites ESW_P0FFEN starting from
u32 reg = 0, instead of doing the readl/modify/writel that every other
helper here does.  Is that intentional, and if so what about other bits
that may live in this register?

> +
> +int mtip_port_learning_config(struct switch_enet_private *fep, int port,
> +			      bool disable, bool irq_adj)
> +{
> +	u32 reg = 0;
> +
> +	if (port < 0 || port > 2) {
> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not supported\n",
> +			__func__, port);
> +		return -EINVAL;
> +	}
> +
> +	reg = readl(fep->hwp + ESW_BKLR);
> +	if (disable) {
> +		if (irq_adj)
> +			writel(readl(fep->hwp + ESW_IMR) & ~MCF_ESW_IMR_LRN,
> +			       fep->hwp + ESW_IMR);

[Medium]
Can this RMW on ESW_IMR race with mtip_interrupt()?  The hardirq
handler does its own RMW on the same register with no lock:

	int_imask = readl(fep->hwp + ESW_IMR);
	int_imask &= ~MCF_ESW_IMR_RXF;
	writel(int_imask, fep->hwp + ESW_IMR);

If the hardirq fires between this helper's readl() and writel(), the
interrupt handler's RXF mask clear can be lost on writeback,
re-enabling RXF while NAPI is supposed to be servicing it.

Today mtip_config_switch() calls this helper with irq_adj = 0 so the
ESW_IMR side is dormant, but the function is exported for the explicit
purpose of toggling the LRN mask, so the documented use of the API
trips the race.  Should this take fep->hw_lock with spin_lock_irqsave()
(and have mtip_interrupt() take the same lock around its RMW)?

> +
[ ... ]