[U-Boot] [u-boot] [PATCH] [2/2] [v1.2] mxc_fec: avoid free() calls to already freed pointers.

[U-Boot] [u-boot] [PATCH] [2/2] [v1.2] mxc_fec: avoid free() calls to already freed pointers.

[javier Martin]

Sometimes, inside NetLoop, eth_halt() is called before eth_init() has
been called. This is harmless except for free() calls to pointers
which have not been allocated yet. This patch adds two states to
distinguish when it is necessary to call free() and when it is not.

This has been tested in i.MX27 Litekit board and eldk-4.2 toolchains.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
--
 drivers/net/fec_mxc.c |    9 +++++++--
 drivers/net/fec_mxc.h |   10 ++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index bd83a24..9e9ef99 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -55,6 +55,7 @@ struct fec_priv gfec = {
 	.tbd_base  = NULL,
 	.tbd_index = 0,
 	.bd        = NULL,
+	.status	   = FEC_HALT_STATUS,
 };

 /*
@@ -453,6 +454,7 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
 		miiphy_restart_aneg(dev);

 	fec_open(dev);
+	fec->status = FEC_INIT_STATUS;
 	return 0;
 }

@@ -491,8 +493,11 @@ static void fec_halt(struct eth_device *dev)
 	writel(0, &fec->eth->ecntrl);
 	fec->rbd_index = 0;
 	fec->tbd_index = 0;
-	free(fec->rdb_ptr);
-	free(fec->base_ptr);
+	if (fec->status == FEC_INIT_STATUS) {
+		free(fec->rdb_ptr);
+		free(fec->base_ptr);
+	}
+	fec->status = FEC_HALT_STATUS;
 	debug("eth_halt: done\n");
 }

diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
index 6cb1bfc..266b5d3 100644
--- a/drivers/net/fec_mxc.h
+++ b/drivers/net/fec_mxc.h
@@ -245,9 +245,19 @@ struct fec_priv {
 	bd_t *bd;
 	void *rdb_ptr;
 	void *base_ptr;
+	int status;			/* whether fec is halted or not* */
 };

 /**
+ * @brief Possible values for status
+ *
+ * fec_halt() changes the status to FEC_HALT_STATUS and fec_init()
+ * changes the status to FEC_INIT_STATUS
+ */
+#define FEC_HALT_STATUS 0
+#define FEC_INIT_STATUS 1
+
+/**
  * @brief Numbers of buffer descriptors for receiving
  *
  * The number defines the stocked memory buffers for the receiving task.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

[U-Boot] [u-boot] [PATCH] [2/2] [v1.2] mxc_fec: avoid free() calls to already freed pointers.

[Wolfgang Denk]

Dear javier Martin,

In message <eedb5540910192248j2c8f45edq4f36763737dcc692@mail.gmail.com> you wrote:
> Sometimes, inside NetLoop, eth_halt() is called before eth_init() has
> been called. This is harmless except for free() calls to pointers
> which have not been allocated yet. This patch adds two states to
> distinguish when it is necessary to call free() and when it is not.
> 
> This has been tested in i.MX27 Litekit board and eldk-4.2 toolchains.

Thanks.

>  /**
> + * @brief Possible values for status
> + *
> + * fec_halt() changes the status to FEC_HALT_STATUS and fec_init()
> + * changes the status to FEC_INIT_STATUS
> + */
> +#define FEC_HALT_STATUS 0
> +#define FEC_INIT_STATUS 1

It would seem more logical to me if you swapped names around, i. e.
please use FEC_STATUS_INIT and FEC_STATUS_HALT.

Also, we might consider using an enum here?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Dear Lord: I just want *one* one-armed manager so  I  never  have  to
hear "On the other hand", again.

[U-Boot] [u-boot] [PATCH] [2/2] [v1.2] mxc_fec: avoid free() calls to already freed pointers.

[javier Martin]

>
>> ?/**
>> + * @brief Possible values for status
>> + *
>> + * fec_halt() changes the status to FEC_HALT_STATUS and fec_init()
>> + * changes the status to FEC_INIT_STATUS
>> + */
>> +#define FEC_HALT_STATUS 0
>> +#define FEC_INIT_STATUS 1
>
> It would seem more logical to me if you swapped names around, i. e.
> please use FEC_STATUS_INIT and FEC_STATUS_HALT.
>
> Also, we might consider using an enum here?


No problem, let me fix it and resend.

Thank you for the comments.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

[U-Boot] [u-boot] [PATCH] [2/2] [v1.2] mxc_fec: avoid free() calls to already freed pointers.

[Ben Warren]

Javier,


On Mon, Oct 19, 2009 at 10:48 PM, javier Martin <
javier.martin@vista-silicon.com> wrote:

> Sometimes, inside NetLoop, eth_halt() is called before eth_init() has
> been called. This is harmless except for free() calls to pointers
> which have not been allocated yet. This patch adds two states to
> distinguish when it is necessary to call free() and when it is not.
>
> IMHO we should aim to make this driver less complicated, not more.  Do we
really need to malloc and free each time this interface is turned on or off?
 Why not just initialize the pointers to NULL, and malloc() only the first
time the init() function is called?  You could then get rid of the free()
calls in the halt function.

U-boot is very transient in nature.  Once we launch a kernel all the memory
essentially gets freed anyway.

regards,
Ben

[U-Boot] [u-boot] [PATCH] [2/2] [v1.2] mxc_fec: avoid free() calls to already freed pointers.

[Wolfgang Denk]

Dear Ben Warren,

In message <f8328f7c0910251228p4d5bd05j811b32553f37d8ef@mail.gmail.com> you wrote:
> 
> > IMHO we should aim to make this driver less complicated, not more.  Do we
> really need to malloc and free each time this interface is turned on or off?
>  Why not just initialize the pointers to NULL, and malloc() only the first
> time the init() function is called?  You could then get rid of the free()
> calls in the halt function.
> 
> U-boot is very transient in nature.  Once we launch a kernel all the memory
> essentially gets freed anyway.

Full ACK from me.  And we don't use that many different Ethernet
drivers in parallel either that the RAM footprint would hurt.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Genitiv ins Wasser, weil's Dativ ist!