Re: [v20 02/13] rtase: Implement the .ndo_open function

[Simon Horman <horms@kernel.org>]

On Mon, Jun 17, 2024 at 01:28:51PM +0000, Justin Lai wrote:
> 
> > >> How do you think about to increase the application of scope-based resource
> > management?
> > >> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/clean
> > >> up.h#L8
> > >
> > > Due to our tx and rx each having multiple queues that need to allocate
> > > descriptors, if any one of the queues fails to allocate,
> > > rtase_alloc_desc() will return an error. Therefore, using 'goto'
> > > here rather than directly returning seems to be reasonable.
> > 
> > Some goto chains can be replaced by further usage of advanced cleanup
> > techniques, can't they?
> > 
> > Regards,
> > Markus
> 
> rtase_alloc_desc() is used to allocate DMA memory. 
> I'd like to ask if it's better to keep our current method?

Hi Justin,

It may be the case that the techniques recently added by cleanup.h could be
used here. But, OTOH, it is the case that using goto for unwinding errors
is in keeping with current Networking driver best practices.

Regardless of the above, I would suggest that if an error occurs in
rtase_alloc_desc() then it release any resources it has allocated. Assuming
the elements of tp->tx_ring and tp->rx_ring are initialised to NULL when
rtase_alloc_desc is called, it looks like that can be achieved by
rtase_alloc_desc() calling rtase_free_desc().

Something like the following (completely untested!).
Please also note that there is probably no need to call netdev_err on
error, as the memory core should already log on error.

static int rtase_alloc_desc(struct rtase_private *tp)
{
	struct pci_dev *pdev = tp->pdev;
	u32 i;

	/* rx and tx descriptors needs 256 bytes alignment.
	 * dma_alloc_coherent provides more.
	 */
	for (i = 0; i < tp->func_tx_queue_num; i++) {
		tp->tx_ring[i].desc =
				dma_alloc_coherent(&pdev->dev,
						   RTASE_TX_RING_DESC_SIZE,
						   &tp->tx_ring[i].phy_addr,
						   GFP_KERNEL);
		if (!tp->tx_ring[i].desc)
			goto err;
	}

	for (i = 0; i < tp->func_rx_queue_num; i++) {
		tp->rx_ring[i].desc =
				dma_alloc_coherent(&pdev->dev,
						   RTASE_RX_RING_DESC_SIZE,
						   &tp->rx_ring[i].phy_addr,
						   GFP_KERNEL);
		if (!tp->rx_ring[i].desc)
			goto err;
		}
	}

	return 0;

err:
	rtase_free_desc(tp)
	return -ENOMEM;
}

And then rtase_alloc_desc can be called like this in rtase_open():

static int rtase_open(struct net_device *dev)
{
	struct rtase_private *tp = netdev_priv(dev);
	const struct pci_dev *pdev = tp->pdev;
	struct rtase_int_vector *ivec;
	u16 i = 0, j;
	int ret;

	ivec = &tp->int_vector[0];
	tp->rx_buf_sz = RTASE_RX_BUF_SIZE;

	ret = rtase_alloc_desc(tp);
	if (ret)
		return ret;

	ret = rtase_init_ring(dev);
	if (ret)
		goto err_free_all_allocated_mem;

...

err_free_all_allocated_mem:
	rtase_free_desc(tp);

	return ret;
}

This is would be in keeping with my understanding of best practices for
Networking drivers: that callers don't have to worry about cleaning up
resources allocated by functions that return an error.


I would also suggest reading Markus's advice with due care,
as it is not always aligned with best practice for Networking code.