Re: [net-next,v23,5/7] net: mtip: Add mtip_switch_{rx|tx} functions to the L2 switch driver

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mtip: Add mtip_switch_{rx|tx} functions to the L2 switch driver

This patch provides mtip_switch_tx and mtip_switch_rx functions code for
MTIP L2 switch.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> index 4c64681602d6..61a624f3df2b 100644
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> @@ -246,6 +246,39 @@ struct mtip_port_info *mtip_portinfofifo_read(struct switch_enet_private *fep)
>  	return info;
>  }
>
> +static void mtip_atable_get_entry_port_number(struct switch_enet_private *fep,
> +					      unsigned char *mac_addr, u8 *port)
> +{
> +	int block_index, block_index_end, entry;
> +	u32 mac_addr_lo, mac_addr_hi;
> +	u32 read_lo, read_hi;
> +
> +	mac_addr_lo = (u32)((mac_addr[3] << 24) | (mac_addr[2] << 16) |
> +			    (mac_addr[1] << 8) | mac_addr[0]);
> +	mac_addr_hi = (u32)((mac_addr[5] << 8) | (mac_addr[4]));
> +
> +	block_index = GET_BLOCK_PTR(crc8_calc(mac_addr));
> +	block_index_end = block_index + ATABLE_ENTRY_PER_SLOT;
> +
> +	/* now search all the entries in the selected block */
> +	for (entry = block_index; entry < block_index_end; entry++) {
> +		mtip_read_atable(fep, entry, &read_lo, &read_hi);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can this create a torn read of the address table entry?

mtip_read_atable() performs two separate readl() operations to read a
64-bit address table entry:

    *read_lo = readl(&atable_base[index].lo);
    *read_hi = readl(&atable_base[index].hi);

If another thread holding fep->learn_lock updates the same entry between
these two reads, the lookup would see an inconsistent state (old low word
with new high word, or vice versa).

Looking at the call chain:

    Thread 1 (no lock):
    mtip_switch_rx()->mtip_atable_get_entry_port_number()->mtip_read_atable()

    Thread 2 (lock held):
    mtip_mgnt_timer()->mtip_atable_dynamicms_learn_migration()->
    mtip_update_atable_dynamic1()->mtip_write_atable()

The race could cause a MAC address lookup to fail or match the wrong entry,
delivering packets to the wrong port.

All other functions that read or write the address table acquire
fep->learn_lock before calling mtip_read_atable() or mtip_write_atable().

Should this function also hold the lock during the lookup loop?

> +		*port = MTIP_PORT_FORWARDING_INIT;
> +
> +		if (read_lo == mac_addr_lo &&
> +		    ((read_hi & 0x0000FFFF) ==
> +		     (mac_addr_hi & 0x0000FFFF))) {
> +			/* found the correct address */
> +			if ((read_hi & (1 << 16)) && (!(read_hi & (1 << 17))))
> +				*port = FIELD_GET(AT_PORT_MASK, read_hi);
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(&fep->pdev->dev, "%s: MAC: %pM PORT: 0x%x\n", __func__,
> +		mac_addr, *port);
> +}

[ ... ]