Re: [PATCH 18/24] net: ethernet: zte: Add ZX279128S Ethernet switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] mm: unmanaged DMA arena and rhashtable memory leak on probe error paths
- [High] hardware: incorrect sequence of command and data writes in SBRG indirect accesses
- [High] hardware: out-of-bounds indirect table access in zx_sbrg_fdb_del loop
- [High] lib: unhandled rhashtable_init error leaves ppe_flows in invalid state
- [High] net: ethernet: zte: hardware VLAN and FDB tables are written with stale data
- [High] mm: dma_alloc_coherent 16MB memory leak on probe error paths
- [High] net: zte: active notifiers reschedule work after hardware reset in shutdown
- [Medium] net: permanent leak of valid BMU buffer index 0 in zx_eth_rx
- [Medium] notifier: incomplete cleanup of network notifiers on partial registration failure
- [Medium] mm: memory and DMA leaks on probe error paths
- [Low] cleanup: mixing __free() scope-based cleanup with goto error handling
- [Low] cleanup: mixed goto and __free() in zx_eth_probe()
--

commit 119d6d4afd1aad4ec6720d33caee938510d72bf0
Author: Navid Ghahremani <ghahramani.navid@gmail.com>

net: ethernet: zte: Add ZX279128S Ethernet switch driver

This commit introduces a new driver for the five-port gigabit switch
of the ZTE zx279128s SoC. It includes phylink support, switchdev
bridge forwarding, hardware IPv4 flow offload, and MAC statistics.

> diff --git a/drivers/net/ethernet/zte/zx279128s-main.c b/drivers/net/ethernet/zte/zx279128s-main.c
> index 0000000000000..3b9834d63f81c
> --- /dev/null
> +++ b/drivers/net/ethernet/zte/zx279128s-main.c
[ ... ]
> @@ -731,6 +731,14 @@ static int zx_eth_rx(struct zx_eth_adapter *adapter, int budget)
>  				continue;
>  			}
>  			adapter->rx_synced[q] = true;
>  
>  			if (bp == 0) {
>  				adapter->rx_nobuf_cnt++;
>  				zx_rx_consume(desc, rel);
>  				adapter->rx_cur_idx[q]++;
>  				work_done++;
>  				avail--;
>  				continue;
>  			}

[Severity: Medium]
Does this code permanently leak buffer index 0? The BMU free list is
initialized with index 0 as a valid buffer, but this treats bp == 0 as a
sentinel value. If the hardware legitimately uses buffer 0 for a received
frame, it seems we skip the frame and bypass zx_bmu_free_bp(), permanently
losing the buffer.

[ ... ]
> @@ -1213,6 +1213,7 @@ static int zx_eth_probe(struct platform_device *pdev)
>  	struct device_node *ports_np __free(device_node) =
>  		of_get_child_by_name(dev->of_node, "ethernet-ports");

[Severity: Low]
This isn't a bug, but does mixing scope-based cleanup helpers with goto
jumps in the same function violate the cleanup guidelines? zx_eth_probe()
utilizes both __free(device_node) and traditional goto error handling,
which the core documentation recommends avoiding to prevent out-of-order
execution issues during unwinding.

[ ... ]
> @@ -1245,6 +1246,8 @@ static int zx_eth_probe(struct platform_device *pdev)
>  	adapter->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(adapter->base))
>  		return PTR_ERR(adapter->base);

[Severity: Medium]
Does this early return leak the rhashtable? rhashtable_init() was called
earlier in this function, but this return bypasses rhashtable_destroy().

[ ... ]
> @@ -1358,16 +1361,16 @@ static int zx_eth_probe(struct platform_device *pdev)
>  
>  err_napi:
>  	netif_napi_del(&adapter->napi);
>  err_disconnect:
>  	for (i = 0; i < ZX_NUM_PORTS; i++) {

[Severity: High]
Do the error paths in this function permanently leak the 16 MiB contiguous
DMA arena and the initialized rhashtable? Because dma_alloc_coherent() is
not managed by devres, returning here without calling dma_free_coherent()
and rhashtable_destroy() leaves these resources stranded whenever the probe
fails.

[ ... ]
> @@ -1441,6 +1444,9 @@ static void zx_eth_shutdown(struct platform_device *pdev)
>  {
[ ... ]
>  	cancel_work_sync(&adapter->sw_work);
>  	cancel_delayed_work_sync(&adapter->mib_work);
>  
>  	zx_hw_reset(adapter);
>  }

[Severity: High]
Can active notifiers reschedule the workqueue after the hardware has been
reset? zx_eth_shutdown() calls cancel_work_sync() and zx_hw_reset(), but
leaves the global notifiers registered. If another network device changes
state during the system teardown, the notifiers can catch the event and
call schedule_work(&adapter->sw_work). The worker thread would then run
zx_sw_work() and access MMIO registers while the hardware is in reset,
potentially causing bus faults or hangs.

> diff --git a/drivers/net/ethernet/zte/zx279128s-ppe.c b/drivers/net/ethernet/zte/zx279128s-ppe.c
> index 0000000000000..d9c6c1d249dbd
> --- /dev/null
> +++ b/drivers/net/ethernet/zte/zx279128s-ppe.c
[ ... ]
> @@ -673,8 +673,9 @@ static void zx_ppe_stop(struct zx_eth_adapter *adapter)
>  {
[ ... ]
>  	rhashtable_free_and_destroy(&adapter->ppe_flows, zx_ppe_flush, adapter);
>  	rhashtable_init(&adapter->ppe_flows, &zx_ppe_ht_params);

[Severity: High]
Does ignoring the return value of rhashtable_init() risk a kernel panic?
The initialization dynamically allocates memory and can fail. If it fails,
adapter->ppe_flows is left broken, and a subsequent offload request calling
rhashtable_insert_fast() on it would cause a crash.

> diff --git a/drivers/net/ethernet/zte/zx279128s-switch.c b/drivers/net/ethernet/zte/zx279128s-switch.c
> index 0000000000000..91df8de4556de
> --- /dev/null
> +++ b/drivers/net/ethernet/zte/zx279128s-switch.c
[ ... ]
> @@ -59,10 +59,10 @@ static void zx_sbrg_fdb_del(struct zx_eth_adapter *adapter, const u8 *mac)
>  {
>  	static const int sizes[4] = { 1024, 256, 512, 512 };
>  	void __iomem *base = adapter->base;
>  	u32 hi = mac[0] << 16 | mac[1] << 8 | mac[2];
>  	u32 lo = mac[3] << 24 | mac[4] << 16 | mac[5] << 8;
>  	int n = sizes[readl(base + ZX_SBRG_TABLE_SEL) & 3];
>  	int ram, i;
>  
>  	for (ram = 0; ram < 4; ram++) {
>  		for (i = 0; i < n; i++) {

[Severity: High]
Does this loop read out of bounds on the smaller RAM banks? The loop limit
n is calculated once based on the currently selected SBRG table. Since the
table sizes vary, if n is set to 1024 on the first bank, the loop will
attempt to read up to index 1023 on bank 1, which only has 256 entries.

[ ... ]
>  			if (zx_sbrg_ind(adapter, ram << 22 | i))
>  				return;
>  			writel(0, base + ZX_SBRG_IND_DATA(2));
>  			writel(0, base + ZX_SBRG_IND_DATA(1));
>  			writel(0, base + ZX_SBRG_IND_DATA(0));
>  			adapter->sw_fdb_deleted++;

[Severity: High]
Is this the correct sequence for indirect hardware access? zx_sbrg_ind()
issues the command to the ZX_SBRG_IND_CMD register and polls for completion
before the ZX_SBRG_IND_DATA registers are populated. If the hardware
triggers on the command write, it uses stale data. If it triggers on
DATA(0), zx_sbrg_ind() completes immediately, and the driver fails to wait
for the data write to finish before moving on.

[ ... ]
> @@ -142,6 +142,6 @@ static void zx_sw_apply(struct zx_eth_adapter *adapter)
>  	writel(0, base + ZX_SBRG_LEARN);
>  	if (zx_sbrg_ind(adapter, ZX_SBRG_MEM_VLAN << 22 | 1))
>  		dev_warn(adapter->dev, "bridge VLAN table access timed out\n");
>  	else
>  		writel(members ? vlan | ZX_SBRG_VLAN_VALID : 0,
>  		       base + ZX_SBRG_IND_DATA(0));

[Severity: High]
Similar to the FDB deletion, is it safe to issue the ZX_SBRG_IND_CMD write
and poll for completion via zx_sbrg_ind() before populating
ZX_SBRG_IND_DATA(0)? This risks either writing stale data to the VLAN table
or initiating a write that is never waited on.

[ ... ]
> @@ -328,6 +328,6 @@ void zx_sw_init(struct zx_eth_adapter *adapter)
>  {
>  	adapter->sw_netdev_nb.notifier_call = zx_sw_netdev_event;
>  	adapter->sw_switchdev_nb.notifier_call = zx_sw_switchdev_event;
>  	adapter->sw_switchdev_blocking_nb.notifier_call =
>  		zx_sw_switchdev_blocking_event;
>  	adapter->sw_ready =
>  		!register_netdevice_notifier(&adapter->sw_netdev_nb) &&
>  		!register_switchdev_notifier(&adapter->sw_switchdev_nb) &&
>  		!register_switchdev_blocking_notifier(&adapter->sw_switchdev_blocking_nb);

[Severity: Medium]
Can a partial registration failure leak notifiers? Because the logical AND
operator uses short-circuit evaluation, if register_netdevice_notifier()
succeeds but register_switchdev_notifier() fails, the sequence stops. The
function marks sw_ready = false but leaves the first notifier fully active
without calling unregister_netdevice_notifier() to clean it up.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010130024.177503-1-ghahramani.navid@gmail.com?part=18