[PATCH] ibm_newemac: Fixes kernel crashes when speed of cable connected changes

[PATCH] ibm_newemac: Fixes kernel crashes when speed of cable connected changes

[Stefan Roese]

From: Sathya Narayanan <sathyan@teamf1.com>

The descriptor pointers were not initialized to NIL values, so it was
poiniting to some random addresses which was completely invalid. This
fix takes care of initializing the descriptor to NIL values and clearing
the valid descriptors on clean ring operation.

Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
Signed-off-by: Stefan Roese <sr@denx.de>
---
 drivers/net/ibm_newemac/core.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index 5d2108c..6dfc2c9 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -1025,7 +1025,7 @@ static void emac_clean_tx_ring(struct emac_instance *dev)
 	int i;
 
 	for (i = 0; i < NUM_TX_BUFF; ++i) {
-		if (dev->tx_skb[i]) {
+		if (dev->tx_skb[i] && dev->tx_desc[i].data_ptr) {
 			dev_kfree_skb(dev->tx_skb[i]);
 			dev->tx_skb[i] = NULL;
 			if (dev->tx_desc[i].ctrl & MAL_TX_CTRL_READY)
@@ -2719,6 +2719,10 @@ static int __devinit emac_probe(struct of_device *ofdev,
 	/* Clean rings */
 	memset(dev->tx_desc, 0, NUM_TX_BUFF * sizeof(struct mal_descriptor));
 	memset(dev->rx_desc, 0, NUM_RX_BUFF * sizeof(struct mal_descriptor));
+	for (i = 0; i <= NUM_TX_BUFF; i++)
+		dev->tx_skb[i] = NULL;
+	for (i = 0; i <= NUM_RX_BUFF; i++)
+		dev->rx_skb[i] = NULL;
 
 	/* Attach to ZMII, if needed */
 	if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII) &&
-- 
1.5.6

Re: [PATCH] ibm_newemac: Fixes kernel crashes when speed of cable connected changes

[Benjamin Herrenschmidt]

On Mon, 2008-06-23 at 14:54 +0200, Stefan Roese wrote:
> From: Sathya Narayanan <sathyan@teamf1.com>
> 
> The descriptor pointers were not initialized to NIL values, so it was
> poiniting to some random addresses which was completely invalid. This
> fix takes care of initializing the descriptor to NIL values and clearing
> the valid descriptors on clean ring operation.
> 
> Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  drivers/net/ibm_newemac/core.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
> index 5d2108c..6dfc2c9 100644
> --- a/drivers/net/ibm_newemac/core.c
> +++ b/drivers/net/ibm_newemac/core.c
> @@ -1025,7 +1025,7 @@ static void emac_clean_tx_ring(struct emac_instance *dev)
>  	int i;
>  
>  	for (i = 0; i < NUM_TX_BUFF; ++i) {
> -		if (dev->tx_skb[i]) {
> +		if (dev->tx_skb[i] && dev->tx_desc[i].data_ptr) {

Why changing the test above ?

>  			dev_kfree_skb(dev->tx_skb[i]);
>  			dev->tx_skb[i] = NULL;
>  			if (dev->tx_desc[i].ctrl & MAL_TX_CTRL_READY)
> @@ -2719,6 +2719,10 @@ static int __devinit emac_probe(struct of_device *ofdev,
>  	/* Clean rings */
>  	memset(dev->tx_desc, 0, NUM_TX_BUFF * sizeof(struct mal_descriptor));
>  	memset(dev->rx_desc, 0, NUM_RX_BUFF * sizeof(struct mal_descriptor));
> +	for (i = 0; i <= NUM_TX_BUFF; i++)
> +		dev->tx_skb[i] = NULL;
> +	for (i = 0; i <= NUM_RX_BUFF; i++)
> +		dev->rx_skb[i] = NULL;

Why not use memset here too ?

>  	/* Attach to ZMII, if needed */
>  	if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII) &&

Re: [PATCH] ibm_newemac: Fixes kernel crashes when speed of cable connected changes

[SathyaNarayanan]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

Hi benh,

               Please find my comments inline.

Thanks and regards,
SathyaNarayanan

On Tue, Jun 24, 2008 at 4:50 AM, Benjamin Herrenschmidt <
benh@kernel.crashing.org> wrote:

> On Mon, 2008-06-23 at 14:54 +0200, Stefan Roese wrote:
> > From: Sathya Narayanan <sathyan@teamf1.com>
> >
> > The descriptor pointers were not initialized to NIL values, so it was
> > poiniting to some random addresses which was completely invalid. This
> > fix takes care of initializing the descriptor to NIL values and clearing
> > the valid descriptors on clean ring operation.
> >
> > Signed-off-by: Sathya Narayanan <sathyan@teamf1.com>
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > ---
> >  drivers/net/ibm_newemac/core.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/ibm_newemac/core.c
> b/drivers/net/ibm_newemac/core.c
> > index 5d2108c..6dfc2c9 100644
> > --- a/drivers/net/ibm_newemac/core.c
> > +++ b/drivers/net/ibm_newemac/core.c
> > @@ -1025,7 +1025,7 @@ static void emac_clean_tx_ring(struct emac_instance
> *dev)
> >       int i;
> >
> >       for (i = 0; i < NUM_TX_BUFF; ++i) {
> > -             if (dev->tx_skb[i]) {
> > +             if (dev->tx_skb[i] && dev->tx_desc[i].data_ptr) {
>
> Why changing the test above ?


   The reason for changing this condition is ,  In any of the case if the
dev->tx_skb is not containing valid address, Then while clearing it you may
be resulted in "address voilations". This additional condition ensures that
we are clearing the valid skbs.
Further this condition is not in general data flow, So this additional
condition should not have any impact on performance.

>
>
> >                       dev_kfree_skb(dev->tx_skb[i]);
> >                       dev->tx_skb[i] = NULL;
> >                       if (dev->tx_desc[i].ctrl & MAL_TX_CTRL_READY)
> > @@ -2719,6 +2719,10 @@ static int __devinit emac_probe(struct of_device
> *ofdev,
> >       /* Clean rings */
> >       memset(dev->tx_desc, 0, NUM_TX_BUFF * sizeof(struct
> mal_descriptor));
> >       memset(dev->rx_desc, 0, NUM_RX_BUFF * sizeof(struct
> mal_descriptor));
> > +     for (i = 0; i <= NUM_TX_BUFF; i++)
> > +             dev->tx_skb[i] = NULL;
> > +     for (i = 0; i <= NUM_RX_BUFF; i++)
> > +             dev->rx_skb[i] = NULL;
>
> Why not use memset here too ?

    Yes, It was valid to use memset here. I can send the modified patch for
it.

>
>
> >       /* Attach to ZMII, if needed */
> >       if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII) &&
>
>

[-- Attachment #2: Type: text/html, Size: 4135 bytes --]

Re: [PATCH] ibm_newemac: Fixes kernel crashes when speed of cable connected changes

[Benjamin Herrenschmidt]

>         
>         >       for (i = 0; i < NUM_TX_BUFF; ++i) {
>         > -             if (dev->tx_skb[i]) {
>         > +             if (dev->tx_skb[i] &&
>         dev->tx_desc[i].data_ptr) {
>         
>         
>         Why changing the test above ?
> 
>    The reason for changing this condition is ,  In any of the case if
> the dev->tx_skb is not containing valid address, Then while clearing
> it you may be resulted in "address voilations". This additional
> condition ensures that we are clearing the valid skbs.
> Further this condition is not in general data flow, So this additional
> condition should not have any impact on performance.

Do you see -any- case where tx_skb[i] and dev->tx_desc[i].data_ptr would
be out of sync ? If that's the case, shouldn't we cleanup instead of
leaving some kind of stale entry in the ring ?

In addition, in pure theory, data_ptr == 0 is a valid DMA address :-) So
I think that part of the patch shouldn't be there.

>         
>         >                       dev_kfree_skb(dev->tx_skb[i]);
>         >                       dev->tx_skb[i] = NULL;
>         >                       if (dev->tx_desc[i].ctrl &
>         MAL_TX_CTRL_READY)
>         > @@ -2719,6 +2719,10 @@ static int __devinit
>         emac_probe(struct of_device *ofdev,
>         >       /* Clean rings */
>         >       memset(dev->tx_desc, 0, NUM_TX_BUFF * sizeof(struct
>         mal_descriptor));
>         >       memset(dev->rx_desc, 0, NUM_RX_BUFF * sizeof(struct
>         mal_descriptor));
>         > +     for (i = 0; i <= NUM_TX_BUFF; i++)
>         > +             dev->tx_skb[i] = NULL;
>         > +     for (i = 0; i <= NUM_RX_BUFF; i++)
>         > +             dev->rx_skb[i] = NULL;
>         
>         
>         Why not use memset here too ?
>     Yes, It was valid to use memset here. I can send the modified
> patch for it. 

Please do, thanks.

Cheers,
Ben.