From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hu-out-0506.google.com (hu-out-0506.google.com [72.14.214.231]) by ozlabs.org (Postfix) with ESMTP id 4FF5FDDF36 for ; Fri, 27 Jun 2008 16:55:01 +1000 (EST) Received: by hu-out-0506.google.com with SMTP id 34so921493hue.9 for ; Thu, 26 Jun 2008 23:54:59 -0700 (PDT) Message-ID: <1946a170806262354k582ad86asc9a1a8383aad45a4@mail.gmail.com> Date: Fri, 27 Jun 2008 12:24:59 +0530 From: SathyaNarayanan To: benh@kernel.crashing.org Subject: Re: [PATCH] ibm_newemac: Fixes kernel crashes when speed of cable connected changes In-Reply-To: <1214263216.8011.276.camel@pasglop> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_10107_9885088.1214549699185" References: <1214225694-25815-1-git-send-email-sr@denx.de> <1214263216.8011.276.camel@pasglop> Cc: linuxppc-dev@ozlabs.org, Stefan Roese , netdev@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , ------=_Part_10107_9885088.1214549699185 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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 > > > > 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 > > Signed-off-by: Stefan Roese > > --- > > 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) && > > ------=_Part_10107_9885088.1214549699185 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline 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) &&


------=_Part_10107_9885088.1214549699185--