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;
+ }
+
|