Re: [PATCH 7/8] net: mv643xx: use platform_device_set_fwnode()

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

Pre-existing issues:
- [High] The driver permanently leaks dynamically allocated platform devices, their resources, platform data, and fwnode references during teardown.
- [High] DMA mappings are leaked and the TX ring state becomes corrupted if descriptor mapping fails during TSO processing.
--

--- Patch [7]: [PATCH 7/8] net: mv643xx: use platform_device_set_fwnode() ---
commit d4dc0688026958e8569e1e6212a8ec7a37b4fb45
Author: Bartosz Golaszewski <[email protected]>

net: mv643xx: use platform_device_set_fwnode()
    
Prefer the higher-level platform_device_set_fwnode() over the
OF-specific platform_device_set_of_node() for dynamically allocated
platform devices.

> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 9caa1e47c174c..2f2d6cce8d852 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
[ ... ]

While reviewing this patch, I noticed a couple of pre-existing regressions in
this file:

> static int txq_submit_tso(struct tx_queue *txq, struct sk_buff *skb,
> 			  struct net_device *dev)
> {
[ ... ]
> 	/* clear TX_END status */
> 	mp->work_tx_end &= ~(1 << txq->index);
> 
> 	/* ensure all descriptors are written before poking hardware */
> 	wmb();
> 	txq_enable(txq);
> 	txq->tx_desc_count += desc_count;
> 	return 0;
> err_release:
> 	/* TODO: Release all used data descriptors; header descriptors must not
> 	 * be DMA-unmapped.
> 	 */
> 	return ret;
> }

[Severity: High]
This is a pre-existing issue, but does this err_release path leak DMA memory
mappings and permanently corrupt the transmit ring state?

If txq_put_data_tso() fails (e.g., due to a dma_map_single() failure), the
code jumps to err_release and returns the error without rolling back already
configured descriptors.

Since txq->tx_desc_count is not incremented for the already processed
descriptors, the subsequent txq_reclaim() loop will never clean them up. The
txq->tx_curr_desc index has already advanced, which seems like it would cause
the ring pointers to lose synchronization.

[ ... ]
> static void mv643xx_eth_shared_of_remove(struct platform_device *pdev)
> {
> 	struct mv643xx_eth_platform_data *pd;
> 	int n;
> 
> 	for (n = 0; n < 3; n++) {
> 		if (!port_platdev[n])
> 			continue;
> 		pd = dev_get_platdata(&port_platdev[n]->dev);
> 		if (pd)
> 			of_node_put(pd->phy_node);
> 		platform_device_del(port_platdev[n]);
> 		port_platdev[n] = NULL;
> 	}
> }

[Severity: High]
This isn't a bug introduced by this patch, but does this loop permanently leak
dynamically allocated platform devices and their resources?

The code calls platform_device_del() to remove the device from the hierarchy,
but it appears to be missing a call to platform_device_put() to drop the
initial reference count.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-pdev-set-fwnode-instead-of-of-node-v1-0-2dee93f42c54@oss.qualcomm.com?part=7