Re: [PATCH] net: macb: free resources on failure path of at91ether_open()

[Claudiu.Beznea]

Please ignore this one!
I'll send a v2.

On 24.06.2020 10:26, Claudiu Beznea wrote:
> DMA buffers were not freed on failure path of at91ether_open().
> Along with changes for freeing the DMA buffers the enable/disable
> interrupt instructions were moved to at91ether_start()/at91ether_stop()
> functions and the operations on at91ether_stop() were done in
> their reverse order (compared with how is done in at91ether_start()):
> before this patch the operation order on interface open path
> was as follows:
> 1/ alloc DMA buffers
> 2/ enable tx, rx
> 3/ enable interrupts
> and the order on interface close path was as follows:
> 1/ disable tx, rx
> 2/ disable interrupts
> 3/ free dma buffers.
> 
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 116 +++++++++++++++++++------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5705359a3612..52582e8ed90e 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3762,15 +3762,9 @@ static int macb_init(struct platform_device *pdev)
>  
>  static struct sifive_fu540_macb_mgmt *mgmt;
>  
> -/* Initialize and start the Receiver and Transmit subsystems */
> -static int at91ether_start(struct net_device *dev)
> +static int at91ether_alloc_coherent(struct macb *lp)
>  {
> -	struct macb *lp = netdev_priv(dev);
>  	struct macb_queue *q = &lp->queues[0];
> -	struct macb_dma_desc *desc;
> -	dma_addr_t addr;
> -	u32 ctl;
> -	int i;
>  
>  	q->rx_ring = dma_alloc_coherent(&lp->pdev->dev,
>  					 (AT91ETHER_MAX_RX_DESCR *
> @@ -3792,6 +3786,43 @@ static int at91ether_start(struct net_device *dev)
>  		return -ENOMEM;
>  	}
>  
> +	return 0;
> +}
> +
> +static void at91ether_free_coherent(struct macb *lp)
> +{
> +	struct macb_queue *q = &lp->queues[0];
> +
> +	if (q->rx_ring) {
> +		dma_free_coherent(&lp->pdev->dev,
> +				  AT91ETHER_MAX_RX_DESCR *
> +				  macb_dma_desc_get_size(lp),
> +				  q->rx_ring, q->rx_ring_dma);
> +		q->rx_ring = NULL;
> +	}
> +
> +	if (q->rx_buffers) {
> +		dma_free_coherent(&lp->pdev->dev,
> +				  AT91ETHER_MAX_RX_DESCR *
> +				  AT91ETHER_MAX_RBUFF_SZ,
> +				  q->rx_buffers, q->rx_buffers_dma);
> +		q->rx_buffers = NULL;
> +	}
> +}
> +
> +/* Initialize and start the Receiver and Transmit subsystems */
> +static int at91ether_start(struct macb *lp)
> +{
> +	struct macb_queue *q = &lp->queues[0];
> +	struct macb_dma_desc *desc;
> +	dma_addr_t addr;
> +	u32 ctl;
> +	int i, ret;
> +
> +	ret = at91ether_alloc_coherent(lp);
> +	if (ret)
> +		return ret;
> +
>  	addr = q->rx_buffers_dma;
>  	for (i = 0; i < AT91ETHER_MAX_RX_DESCR; i++) {
>  		desc = macb_rx_desc(q, i);
> @@ -3813,9 +3844,39 @@ static int at91ether_start(struct net_device *dev)
>  	ctl = macb_readl(lp, NCR);
>  	macb_writel(lp, NCR, ctl | MACB_BIT(RE) | MACB_BIT(TE));
>  
> +	/* Enable MAC interrupts */
> +	macb_writel(lp, IER, MACB_BIT(RCOMP)	|
> +			     MACB_BIT(RXUBR)	|
> +			     MACB_BIT(ISR_TUND)	|
> +			     MACB_BIT(ISR_RLE)	|
> +			     MACB_BIT(TCOMP)	|
> +			     MACB_BIT(ISR_ROVR)	|
> +			     MACB_BIT(HRESP));
> +
>  	return 0;
>  }
>  
> +static void at91ether_stop(struct macb *lp)
> +{
> +	u32 ctl;
> +
> +	/* Disable MAC interrupts */
> +	macb_writel(lp, IDR, MACB_BIT(RCOMP)	|
> +			     MACB_BIT(RXUBR)	|
> +			     MACB_BIT(ISR_TUND)	|
> +			     MACB_BIT(ISR_RLE)	|
> +			     MACB_BIT(TCOMP)	|
> +			     MACB_BIT(ISR_ROVR) |
> +			     MACB_BIT(HRESP));
> +
> +	/* Disable Receiver and Transmitter */
> +	ctl = macb_readl(lp, NCR);
> +	macb_writel(lp, NCR, ctl & ~(MACB_BIT(TE) | MACB_BIT(RE)));
> +
> +	/* Free resources. */
> +	at91ether_free_coherent(lp);
> +}
> +
>  /* Open the ethernet interface */
>  static int at91ether_open(struct net_device *dev)
>  {
> @@ -3835,27 +3896,20 @@ static int at91ether_open(struct net_device *dev)
>  
>  	macb_set_hwaddr(lp);
>  
> -	ret = at91ether_start(dev);
> +	ret = at91ether_start(lp);
>  	if (ret)
>  		goto pm_exit;
>  
> -	/* Enable MAC interrupts */
> -	macb_writel(lp, IER, MACB_BIT(RCOMP)	|
> -			     MACB_BIT(RXUBR)	|
> -			     MACB_BIT(ISR_TUND)	|
> -			     MACB_BIT(ISR_RLE)	|
> -			     MACB_BIT(TCOMP)	|
> -			     MACB_BIT(ISR_ROVR)	|
> -			     MACB_BIT(HRESP));
> -
>  	ret = macb_phylink_connect(lp);
>  	if (ret)
> -		goto pm_exit;
> +		goto stop;
>  
>  	netif_start_queue(dev);
>  
>  	return 0;
>  
> +stop:
> +	at91ether_stop(lp);
>  pm_exit:
>  	pm_runtime_put_sync(&lp->pdev->dev);
>  	return ret;
> @@ -3865,37 +3919,13 @@ static int at91ether_open(struct net_device *dev)
>  static int at91ether_close(struct net_device *dev)
>  {
>  	struct macb *lp = netdev_priv(dev);
> -	struct macb_queue *q = &lp->queues[0];
> -	u32 ctl;
> -
> -	/* Disable Receiver and Transmitter */
> -	ctl = macb_readl(lp, NCR);
> -	macb_writel(lp, NCR, ctl & ~(MACB_BIT(TE) | MACB_BIT(RE)));
> -
> -	/* Disable MAC interrupts */
> -	macb_writel(lp, IDR, MACB_BIT(RCOMP)	|
> -			     MACB_BIT(RXUBR)	|
> -			     MACB_BIT(ISR_TUND)	|
> -			     MACB_BIT(ISR_RLE)	|
> -			     MACB_BIT(TCOMP)	|
> -			     MACB_BIT(ISR_ROVR) |
> -			     MACB_BIT(HRESP));
>  
>  	netif_stop_queue(dev);
>  
>  	phylink_stop(lp->phylink);
>  	phylink_disconnect_phy(lp->phylink);
>  
> -	dma_free_coherent(&lp->pdev->dev,
> -			  AT91ETHER_MAX_RX_DESCR *
> -			  macb_dma_desc_get_size(lp),
> -			  q->rx_ring, q->rx_ring_dma);
> -	q->rx_ring = NULL;
> -
> -	dma_free_coherent(&lp->pdev->dev,
> -			  AT91ETHER_MAX_RX_DESCR * AT91ETHER_MAX_RBUFF_SZ,
> -			  q->rx_buffers, q->rx_buffers_dma);
> -	q->rx_buffers = NULL;
> +	at91ether_stop(lp);
>  
>  	return pm_runtime_put(&lp->pdev->dev);
>  }
>