Re: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes

[Jeff Garzik <jgarzik@xxxxxxxxx>]

Ravinandan Arakali wrote:
> Hi,
> Attached is the second patch in this submission. It contains the following
> software bug fixes.
>
> 1. In free_rx_buffers clearing out RxDs not owned by Xena.
>
> 2. In alarm_intr_handler, when a serr error occurs, schedule a task to reset
> the card rather than stopping Tx queue.
>
> 3. In s2io_close freeing IRQ before calling s2io_reset also added a new call
> to flush queued tasks. This is not done if the s2io_close itself is called
> from a queued task like s2io_restart_nic.
>
> 4. read_eeprom function has been changed such that data to be returned is
> sent as an input argument and the return value represents a pass/fail. The
> previous implementation as Randy had pointed out was error prone as on
> failure it returned -1 which can be interpreted as all ff's, so any data
> area which contained ff's in the eeprom was likely to be treated as an
> error.
>
> 5. Added a flag "task_flag" to track if the call to s2io_close is coming
> from the s2io_restart_nic function or from the ifconfig <I/F> down called by
> user.
>
> 6. Moved register_netdev call from just after setting entry points to the
> end of the s2io_init_nic function.
>
> 7. In s2io.h field added a new member into the s2io_nic structure called
> "task_flag".
>
>
> Signed-off-by: Raghavendra Koushik <raghavendra.koushik@xxxxxxxx>


Comments:

1) the following code looks wrong:

>                 DBG_PRINT(ERR_DBG, "%s: Device indicates ", dev->name);
>                 DBG_PRINT(ERR_DBG, "serious error!!\n");
> -               netif_stop_queue(dev);
> +               schedule_work(&nic->rst_timer_task);

Typically you want to stop the queue _and_ schedule_work().  Otherwise, 
the net stack will continually call your ->hard_start_xmit() hook with 
new skbs, _during_ your rst_timer_task.

2) can this 'if' test be removed?  there is no harm in unconditionally 
calling flush_scheduled_work()

> +       /* Flush all scheduled tasks */
> +       if (sp->task_flag == 1) {
> +               DBG_PRINT(INFO_DBG, "%s: Calling close from a task\n",
> +                         dev->name);
> +       } else {
> +               flush_scheduled_work();
> +       }

3) I do not think that s2io_close should be called from anywhere except 
the net stack...

The following code should not be needed, unless I am missing something:
>         nic_t *sp = dev->priv;
>  
> +       sp->task_flag = 1;
>         s2io_close(dev);
> +       sp->task_flag = 0;
>         sp->device_close_flag = TRUE;


4) the following code is correct, but should additional propagate the 
register_netdev() error code back to its caller, upon failure:

>         sp->rx_csum = 1;        /* Rx chksum verify enabled by default */
>  
> +       if (register_netdev(dev)) {
> +               DBG_PRINT(ERR_DBG, "Device registration failed\n");
> +               goto register_failed;
> +       }
> +