S2io: sw bug fixes

PATCH In review Ravinandan Arakali 3 participants 4 messages Oct 14, 2004 – Oct 15, 2004

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Ravinandan Arakali

author

2004-10-14 01:13 UTC #0 lore raw

PATCH 2/8 S2io: sw bug fixes

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

Jeff Garzik

2004-10-14 14:44 UTC #1 lore raw

Quoted 24 lines · Ravinandan Arakali

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.

Quoted 8 lines

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

Comments:

1) the following code looks wrong:

Review

Quoted 4 lines

                DBG_PRINT(ERR_DBG, "%s: Device indicates ", dev->name);
                DBG_PRINT(ERR_DBG, "serious error!!\n");

(quoted diff) +1 -1

-               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()

Review

Quoted 7 lines

(quoted diff) +7 -0

+       /* 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:

Review

Quoted 7 lines

        nic_t *sp = dev->priv;

(quoted diff) +2 -0

+       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:

Quoted 7 lines

        sp->rx_csum = 1;        /* Rx chksum verify enabled by default */

(quoted diff) +5 -0

+       if (register_netdev(dev)) {
+               DBG_PRINT(ERR_DBG, "Device registration failed\n");
+               goto register_failed;
+       }
+

Raghavendra Koushik

2004-10-14 17:35 UTC #2 lore raw

Quoted 57 lines

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Thursday, October 14, 2004 7:44 AM
To: [email protected]
Cc: 'Francois Romieu'; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
Quoted 2 lines · Ravinandan Arakali
Hi,
Attached is the second patch in this submission. It contains the
following
Quoted 5 lines
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
Quoted 3 lines
the card rather than stopping Tx queue.
3. In s2io_close freeing IRQ before calling s2io_reset also added a new
call
Quoted 1 line
to flush queued tasks. This is not done if the s2io_close itself is
called
Quoted 3 lines
from a queued task like s2io_restart_nic.
4. read_eeprom function has been changed such that data to be returned
is
Quoted 1 line
sent as an input argument and the return value represents a pass/fail.
The
Quoted 7 lines
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
Quoted 3 lines
user.
6. Moved register_netdev call from just after setting entry points to
the
Quoted 7 lines
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 <[email protected]>
Comments:
1) the following code looks wrong:

Quoted 8 lines

Review
Quoted 4 lines
                DBG_PRINT(ERR_DBG, "%s: Device indicates ", dev->name);
                DBG_PRINT(ERR_DBG, "serious error!!\n");
(quoted diff) +1 -1
-               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.

The reset task (s2io_restart_nic) actually calls s2io_close first, which in turn stops the queue to begin with. Hence I got rid of netif_stop_queue.

Quoted 12 lines

2) can this 'if' test be removed? there is no harm in unconditionally calling flush_scheduled_work()
Quoted 7 lines
(quoted diff) +7 -0
+       /* 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();
+       }

As I said the s2io_close can be called from stack or by the reset task. If called from the task (or work) is it still Ok to flush all scheduled works?

Quoted 12 lines

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:
Quoted 7 lines
        nic_t *sp = dev->priv;
(quoted diff) +2 -0
+       sp->task_flag = 1;
        s2io_close(dev);
+       sp->task_flag = 0;
        sp->device_close_flag = TRUE;

I believe the best way to restart the NIC is to call s2io_close and s2io_open sequentially.

Quoted 12 lines

4) the following code is correct, but should additional propagate the register_netdev() error code back to its caller, upon failure:
Quoted 1 line
        sp->rx_csum = 1;        /* Rx chksum verify enabled by default
*/
Quoted 6 lines
(quoted diff) +5 -0
+       if (register_netdev(dev)) {
+               DBG_PRINT(ERR_DBG, "Device registration failed\n");
+               goto register_failed;
+       }
+

Jeff Garzik

2004-10-15 18:52 UTC #3 lore raw

Quoted 2 lines · Raghavendra Koushik

As I said the s2io_close can be called from stack or by the reset task. If called from the task (or work) is it still Ok to flush all scheduled works?

[...]

Quoted 2 lines

I believe the best way to restart the NIC is to call s2io_close and s2io_open sequentially.

Look at other drivers, e.g. drivers/net/tulip/tulip_core.c. You will see two levels of functions:

	tulip_open() -- net stack dev->open() hook
	tulip_close() -- net stack dev->stop() hook

	tulip_up() -- reset-and-start-hardware helper function
	tulip_down() -- stop-hardware helper function

If S2IO follows this model, all good.

If you have both the net stack and your own code directly calling the net stack dev->stop() hook, that will lead to problems. If you are doing this, you must _precisely duplicate_ the code in dev_close() in order to avoid races; but even then, you are really interfering with the code that synchronizes and manages reference-counted objects (struct net_device), which is unwise.

Jeff