[PATCH v2 1/2] net: fec: use managed DMA API functions to allocate BD ring

[PATCH v2 1/2] net: fec: use managed DMA API functions to allocate BD ring

[Lucas Stach]

So it gets freed when the device is going away.
This fixes a DMA memory leak on driver probe() fail and driver
remove().

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
v2: Fix indentation of second line to fix alignment with opening bracket.
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 349365d85b92..a7f1bdf718f8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3142,8 +3142,8 @@ static int fec_enet_init(struct net_device *ndev)
 			fep->bufdesc_size;
 
 	/* Allocate memory for buffer descriptors. */
-	cbd_base = dma_alloc_coherent(NULL, bd_size, &bd_dma,
-				      GFP_KERNEL);
+	cbd_base = dmam_alloc_coherent(&fep->pdev->dev, bd_size, &bd_dma,
+				       GFP_KERNEL);
 	if (!cbd_base) {
 		return -ENOMEM;
 	}
-- 
2.1.4

[PATCH v2 2/2] net: fec: introduce fec_ptp_stop and use in probe fail path

[Lucas Stach]

This function frees resources and cancels delayed work item that
have been initialized in fec_ptp_init().

Use this to do proper error handling if something goes wrong in
probe function after fec_ptp_init has been called.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c |  5 ++---
 drivers/net/ethernet/freescale/fec_ptp.c  | 10 ++++++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 1eee73cccdf5..99d33e2d35e6 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -562,6 +562,7 @@ struct fec_enet_private {
 };
 
 void fec_ptp_init(struct platform_device *pdev);
+void fec_ptp_stop(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
 int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index a7f1bdf718f8..32e3807c650e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3494,6 +3494,7 @@ failed_register:
 failed_mii_init:
 failed_irq:
 failed_init:
+	fec_ptp_stop(pdev);
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 failed_regulator:
@@ -3515,14 +3516,12 @@ fec_drv_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	cancel_delayed_work_sync(&fep->time_keep);
 	cancel_work_sync(&fep->tx_timeout_work);
+	fec_ptp_stop(pdev);
 	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
-	if (fep->ptp_clock)
-		ptp_clock_unregister(fep->ptp_clock);
 	of_node_put(fep->phy_node);
 	free_netdev(ndev);
 
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index a15663ad7f5e..f457a23d0bfb 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -604,6 +604,16 @@ void fec_ptp_init(struct platform_device *pdev)
 	schedule_delayed_work(&fep->time_keep, HZ);
 }
 
+void fec_ptp_stop(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	cancel_delayed_work_sync(&fep->time_keep);
+	if (fep->ptp_clock)
+		ptp_clock_unregister(fep->ptp_clock);
+}
+
 /**
  * fec_ptp_check_pps_event
  * @fep: the fec_enet_private structure handle
-- 
2.1.4

RE: [PATCH v2 1/2] net: fec: use managed DMA API functions to allocate BD ring

[Duan Andy]

From: Lucas Stach <l.stach@pengutronix.de> Sent: Tuesday, July 21, 2015 11:11 PM
> To: David S. Miller
> Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
> kernel@pengutronix.de; patchwork-lst@pengutronix.de
> Subject: [PATCH v2 1/2] net: fec: use managed DMA API functions to
> allocate BD ring
> 
> So it gets freed when the device is going away.
> This fixes a DMA memory leak on driver probe() fail and driver remove().
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> v2: Fix indentation of second line to fix alignment with opening bracket.
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 349365d85b92..a7f1bdf718f8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3142,8 +3142,8 @@ static int fec_enet_init(struct net_device *ndev)
>  			fep->bufdesc_size;
> 
>  	/* Allocate memory for buffer descriptors. */
> -	cbd_base = dma_alloc_coherent(NULL, bd_size, &bd_dma,
> -				      GFP_KERNEL);
> +	cbd_base = dmam_alloc_coherent(&fep->pdev->dev, bd_size, &bd_dma,
> +				       GFP_KERNEL);
>  	if (!cbd_base) {
>  		return -ENOMEM;
>  	}
> --

Can you also replace the below position with dma_alloc_coherent() ?
                txq->tso_hdrs = dma_alloc_coherent(NULL,
                                        txq->tx_ring_size * TSO_HEADER_SIZE,
                                        &txq->tso_hdrs_dma,
                                        GFP_KERNEL);


Regards,
Andy

RE: [PATCH v2 2/2] net: fec: introduce fec_ptp_stop and use in probe fail path

[Duan Andy]

From: Lucas Stach <l.stach@pengutronix.de> Sent: Tuesday, July 21, 2015 11:11 PM
> To: David S. Miller
> Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
> kernel@pengutronix.de; patchwork-lst@pengutronix.de
> Subject: [PATCH v2 2/2] net: fec: introduce fec_ptp_stop and use in probe
> fail path
> 
> This function frees resources and cancels delayed work item that have
> been initialized in fec_ptp_init().
> 
> Use this to do proper error handling if something goes wrong in probe
> function after fec_ptp_init has been called.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  1 +
>  drivers/net/ethernet/freescale/fec_main.c |  5 ++---
> drivers/net/ethernet/freescale/fec_ptp.c  | 10 ++++++++++
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 1eee73cccdf5..99d33e2d35e6 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -562,6 +562,7 @@ struct fec_enet_private {  };
> 
>  void fec_ptp_init(struct platform_device *pdev);
> +void fec_ptp_stop(struct platform_device *pdev);
>  void fec_ptp_start_cyclecounter(struct net_device *ndev);  int
> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);  int
> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); diff --git
> a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index a7f1bdf718f8..32e3807c650e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3494,6 +3494,7 @@ failed_register:
>  failed_mii_init:
>  failed_irq:
>  failed_init:
> +	fec_ptp_stop(pdev);
>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);
>  failed_regulator:
> @@ -3515,14 +3516,12 @@ fec_drv_remove(struct platform_device *pdev)
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> 
> -	cancel_delayed_work_sync(&fep->time_keep);
>  	cancel_work_sync(&fep->tx_timeout_work);
> +	fec_ptp_stop(pdev);
>  	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);
>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);
> -	if (fep->ptp_clock)
> -		ptp_clock_unregister(fep->ptp_clock);
>  	of_node_put(fep->phy_node);
>  	free_netdev(ndev);
> 
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index a15663ad7f5e..f457a23d0bfb 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -604,6 +604,16 @@ void fec_ptp_init(struct platform_device *pdev)
>  	schedule_delayed_work(&fep->time_keep, HZ);  }
> 
> +void fec_ptp_stop(struct platform_device *pdev) {
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +
> +	cancel_delayed_work_sync(&fep->time_keep);
> +	if (fep->ptp_clock)
> +		ptp_clock_unregister(fep->ptp_clock);
> +}
> +
>  /**
>   * fec_ptp_check_pps_event
>   * @fep: the fec_enet_private structure handle
> --
> 2.1.4

Acked-by: Fugang Duan <B38611@freescale.com>

Re: [PATCH v2 1/2] net: fec: use managed DMA API functions to allocate BD ring

[Lucas Stach]

Am Mittwoch, den 22.07.2015, 01:55 +0000 schrieb Duan Andy:
> From: Lucas Stach <l.stach@pengutronix.de> Sent: Tuesday, July 21, 2015 11:11 PM
> > To: David S. Miller
> > Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
> > kernel@pengutronix.de; patchwork-lst@pengutronix.de
> > Subject: [PATCH v2 1/2] net: fec: use managed DMA API functions to
> > allocate BD ring
> > 
> > So it gets freed when the device is going away.
> > This fixes a DMA memory leak on driver probe() fail and driver remove().
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > v2: Fix indentation of second line to fix alignment with opening bracket.
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 349365d85b92..a7f1bdf718f8 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3142,8 +3142,8 @@ static int fec_enet_init(struct net_device *ndev)
> >  			fep->bufdesc_size;
> > 
> >  	/* Allocate memory for buffer descriptors. */
> > -	cbd_base = dma_alloc_coherent(NULL, bd_size, &bd_dma,
> > -				      GFP_KERNEL);
> > +	cbd_base = dmam_alloc_coherent(&fep->pdev->dev, bd_size, &bd_dma,
> > +				       GFP_KERNEL);
> >  	if (!cbd_base) {
> >  		return -ENOMEM;
> >  	}
> > --
> 
> Can you also replace the below position with dma_alloc_coherent() ?
>                 txq->tso_hdrs = dma_alloc_coherent(NULL,
>                                         txq->tx_ring_size * TSO_HEADER_SIZE,
>                                         &txq->tso_hdrs_dma,
>                                         GFP_KERNEL);
> 
> 
No, that one has an explicit free functions.
So using managed function would result in a double free in paths. We
might want to move those over to devm eventually to make the error
handling more robust, but that's clearly out of the scope of this patch.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |