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

[SathyaNarayanan <sathyan@teamf1.com>]

[-- 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 --]