Date: Tue, 18 Jun 2024 07:51:19 +0000
From: Justin Lai <justinlai0215@...ltek.com>
To: Simon Horman <horms@...nel.org>
CC: Markus Elfring <Markus.Elfring@....de>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Eric
 Dumazet" <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, Andrew Lunn
	<andrew@...n.ch>,
        Hariprasad Kelam <hkelam@...vell.com>, Jiri Pirko
	<jiri@...nulli.us>,
        Larry Chiu <larry.chiu@...ltek.com>,
        Ping-Ke Shih
	<pkshih@...ltek.com>,
        Ratheesh Kannoth <rkannoth@...vell.com>
Subject: RE: [v20 02/13] rtase: Implement the .ndo_open function

> 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/c
> > > >> lean
> > > >> 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.

Hi Simon,
Thank you for your response. Based on your suggestion, if the descriptor
allocation of DMA memory fails, I will directly do error handling in
rtase_alloc_desc() using the 'goto' method. Moreover, in the error
handling section, I will call rtase_free_desc(tp) to free the already
allocated descriptor.

Powered by blists - more mailing lists