* [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
@ 2004-10-07 1:23 Ravinandan Arakali
0 siblings, 0 replies; 6+ messages in thread
From: Ravinandan Arakali @ 2004-10-07 1:23 UTC (permalink / raw)
To: 'Jeff Garzik', 'Francois Romieu'
Cc: netdev, leonid.grossman, raghavendra.koushik, rapuru.sriram
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".
Thanks,
Ravi
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
@ 2004-10-07 1:24 Ravinandan Arakali
0 siblings, 0 replies; 6+ messages in thread
From: Ravinandan Arakali @ 2004-10-07 1:24 UTC (permalink / raw)
To: 'Jeff Garzik', 'Francois Romieu'
Cc: netdev, leonid.grossman, raghavendra.koushik, rapuru.sriram
[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]
Sorry, forgot to attach the patch.
Here you go.
-----Original Message-----
From: Ravinandan Arakali [mailto:ravinandan.arakali@s2io.com]
Sent: Wednesday, October 06, 2004 6:23 PM
To: 'Jeff Garzik'; 'Francois Romieu'
Cc: 'netdev@oss.sgi.com'; 'leonid.grossman@s2io.com';
'raghavendra.koushik@s2io.com'; 'rapuru.sriram@s2io.com'
Subject: [PATCH 2.6.9-rc2 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".
Thanks,
Ravi
[-- Attachment #2: s2io_bugfixes.patch2 --]
[-- Type: application/octet-stream, Size: 4767 bytes --]
diff -urN vanilla-linux/drivers/net/s2io.c linux-2.6.8.1/drivers/net/s2io.c
--- vanilla-linux/drivers/net/s2io.c 2004-10-06 15:18:10.000000000 -0700
+++ linux-2.6.8.1/drivers/net/s2io.c 2004-10-06 16:19:52.821420504 -0700
@@ -1525,6 +1525,11 @@
blk++;
}
+ if (!(rxdp->Control_1 & RXD_OWN_XENA)) {
+ memset(rxdp, 0, sizeof(RxD_t));
+ continue;
+ }
+
skb =
(struct sk_buff *) ((unsigned long) rxdp->
Host_Control);
@@ -1887,7 +1892,7 @@
if (val64 & SERR_SOURCE_ANY) {
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);
}
/* Other type of interrupts are not being handled now, TODO */
@@ -2205,6 +2210,17 @@
}
tasklet_kill(&sp->task);
+ /* Free the Registered IRQ */
+ free_irq(dev->irq, dev);
+
+ /* 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();
+ }
+
/* Check if the device is Quiescent and then Reset the NIC */
do {
val64 = readq(&bar0->adapter_status);
@@ -2225,9 +2241,6 @@
} while (1);
s2io_reset(sp);
- /* Free the Registered IRQ */
- free_irq(dev->irq, dev);
-
/* Free all Tx Buffers waiting for transmission */
free_tx_buffers(sp);
@@ -2982,9 +2995,10 @@
*/
#define S2IO_DEV_ID 5
-static u32 read_eeprom(nic_t * sp, int off)
+static int read_eeprom(nic_t * sp, int off, u32 * data)
{
- u32 data = -1, exit_cnt = 0;
+ int ret = -1;
+ u32 exit_cnt = 0;
u64 val64;
XENA_dev_config_t *bar0 = (XENA_dev_config_t *) sp->bar0;
@@ -2996,7 +3010,8 @@
while (exit_cnt < 5) {
val64 = readq(&bar0->i2c_control);
if (I2C_CONTROL_CNTL_END(val64)) {
- data = I2C_CONTROL_GET_DATA(val64);
+ *data = I2C_CONTROL_GET_DATA(val64);
+ ret = 0;
break;
}
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -3004,7 +3019,7 @@
exit_cnt++;
}
- return data;
+ return ret;
}
/**
@@ -3073,8 +3088,7 @@
eeprom->len = XENA_EEPROM_SPACE - eeprom->offset;
for (i = 0; i < eeprom->len; i += 4) {
- data = read_eeprom(sp, eeprom->offset + i);
- if (data < 0) {
+ if (read_eeprom(sp, (eeprom->offset + i), &data)) {
DBG_PRINT(ERR_DBG, "Read of EEPROM failed\n");
return -EFAULT;
}
@@ -3213,7 +3227,8 @@
static int s2io_eeprom_test(nic_t * sp, uint64_t * data)
{
- int fail = 0, ret_data;
+ int fail = 0;
+ u32 ret_data;
/* Test Write Error at offset 0 */
if (!write_eeprom(sp, 0, 0, 3))
@@ -3222,7 +3237,7 @@
/* Test Write at offset 4f0 */
if (write_eeprom(sp, 0x4F0, 0x01234567, 3))
fail = 1;
- if ((ret_data = read_eeprom(sp, 0x4F0)) < 0)
+ if (read_eeprom(sp, 0x4F0, &ret_data))
fail = 1;
if (ret_data != 0x01234567)
@@ -3238,7 +3253,7 @@
/* Test Write Request at offset 0x7fc */
if (write_eeprom(sp, 0x7FC, 0x01234567, 3))
fail = 1;
- if ((ret_data = read_eeprom(sp, 0x7FC)) < 0)
+ if (read_eeprom(sp, 0x7FC, &ret_data))
fail = 1;
if (ret_data != 0x01234567)
@@ -3811,7 +3826,9 @@
struct net_device *dev = (struct net_device *) data;
nic_t *sp = dev->priv;
+ sp->task_flag = 1;
s2io_close(dev);
+ sp->task_flag = 0;
sp->device_close_flag = TRUE;
s2io_open(dev);
DBG_PRINT(ERR_DBG,
@@ -4275,18 +4292,13 @@
INIT_WORK(&sp->set_link_task,
(void (*)(void *)) s2io_set_link, sp);
- if (register_netdev(dev)) {
- DBG_PRINT(ERR_DBG, "Device registration failed\n");
- goto register_failed;
- }
-
pci_save_state(sp->pdev, sp->config_space);
/* Setting swapper control on the NIC, for proper reset operation */
if (s2io_set_swapper(sp)) {
DBG_PRINT(ERR_DBG, "%s:swapper settings are wrong\n",
dev->name);
- goto register_failed;
+ goto set_swap_failed;
}
/* Fix for all "FFs" MAC address problems observed on Alpha platforms */
@@ -4363,6 +4375,11 @@
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;
+ }
+
/*
* Make Link state as off at this point, when the Link change
* interrupt comes the state will be automatically changed to
@@ -4373,9 +4390,8 @@
return 0;
- set_swap_failed:
- unregister_netdev(dev);
register_failed:
+ set_swap_failed:
iounmap(sp->bar1);
bar1_remap_failed:
iounmap(sp->bar0);
diff -urN vanilla-linux/drivers/net/s2io.h linux-2.6.8.1/drivers/net/s2io.h
--- vanilla-linux/drivers/net/s2io.h 2004-10-06 15:15:03.000000000 -0700
+++ linux-2.6.8.1/drivers/net/s2io.h 2004-10-06 16:19:52.829419288 -0700
@@ -668,6 +668,8 @@
u16 last_link_state;
#define LINK_DOWN 1
#define LINK_UP 2
+
+ int task_flag;
} nic_t;
#define RESET_ERROR 1;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
@ 2004-10-14 1:13 Ravinandan Arakali
2004-10-14 14:44 ` Jeff Garzik
0 siblings, 1 reply; 6+ messages in thread
From: Ravinandan Arakali @ 2004-10-14 1:13 UTC (permalink / raw)
To: 'Jeff Garzik', 'Francois Romieu'
Cc: netdev, leonid.grossman, raghavendra.koushik, rapuru.sriram
[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]
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@s2io.com>
[-- Attachment #2: s2io_bugfixes.patch2 --]
[-- Type: application/octet-stream, Size: 4767 bytes --]
diff -urN vanilla-linux/drivers/net/s2io.c linux-2.6.8.1/drivers/net/s2io.c
--- vanilla-linux/drivers/net/s2io.c 2004-10-06 15:18:10.000000000 -0700
+++ linux-2.6.8.1/drivers/net/s2io.c 2004-10-06 16:19:52.821420504 -0700
@@ -1525,6 +1525,11 @@
blk++;
}
+ if (!(rxdp->Control_1 & RXD_OWN_XENA)) {
+ memset(rxdp, 0, sizeof(RxD_t));
+ continue;
+ }
+
skb =
(struct sk_buff *) ((unsigned long) rxdp->
Host_Control);
@@ -1887,7 +1892,7 @@
if (val64 & SERR_SOURCE_ANY) {
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);
}
/* Other type of interrupts are not being handled now, TODO */
@@ -2205,6 +2210,17 @@
}
tasklet_kill(&sp->task);
+ /* Free the Registered IRQ */
+ free_irq(dev->irq, dev);
+
+ /* 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();
+ }
+
/* Check if the device is Quiescent and then Reset the NIC */
do {
val64 = readq(&bar0->adapter_status);
@@ -2225,9 +2241,6 @@
} while (1);
s2io_reset(sp);
- /* Free the Registered IRQ */
- free_irq(dev->irq, dev);
-
/* Free all Tx Buffers waiting for transmission */
free_tx_buffers(sp);
@@ -2982,9 +2995,10 @@
*/
#define S2IO_DEV_ID 5
-static u32 read_eeprom(nic_t * sp, int off)
+static int read_eeprom(nic_t * sp, int off, u32 * data)
{
- u32 data = -1, exit_cnt = 0;
+ int ret = -1;
+ u32 exit_cnt = 0;
u64 val64;
XENA_dev_config_t *bar0 = (XENA_dev_config_t *) sp->bar0;
@@ -2996,7 +3010,8 @@
while (exit_cnt < 5) {
val64 = readq(&bar0->i2c_control);
if (I2C_CONTROL_CNTL_END(val64)) {
- data = I2C_CONTROL_GET_DATA(val64);
+ *data = I2C_CONTROL_GET_DATA(val64);
+ ret = 0;
break;
}
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -3004,7 +3019,7 @@
exit_cnt++;
}
- return data;
+ return ret;
}
/**
@@ -3073,8 +3088,7 @@
eeprom->len = XENA_EEPROM_SPACE - eeprom->offset;
for (i = 0; i < eeprom->len; i += 4) {
- data = read_eeprom(sp, eeprom->offset + i);
- if (data < 0) {
+ if (read_eeprom(sp, (eeprom->offset + i), &data)) {
DBG_PRINT(ERR_DBG, "Read of EEPROM failed\n");
return -EFAULT;
}
@@ -3213,7 +3227,8 @@
static int s2io_eeprom_test(nic_t * sp, uint64_t * data)
{
- int fail = 0, ret_data;
+ int fail = 0;
+ u32 ret_data;
/* Test Write Error at offset 0 */
if (!write_eeprom(sp, 0, 0, 3))
@@ -3222,7 +3237,7 @@
/* Test Write at offset 4f0 */
if (write_eeprom(sp, 0x4F0, 0x01234567, 3))
fail = 1;
- if ((ret_data = read_eeprom(sp, 0x4F0)) < 0)
+ if (read_eeprom(sp, 0x4F0, &ret_data))
fail = 1;
if (ret_data != 0x01234567)
@@ -3238,7 +3253,7 @@
/* Test Write Request at offset 0x7fc */
if (write_eeprom(sp, 0x7FC, 0x01234567, 3))
fail = 1;
- if ((ret_data = read_eeprom(sp, 0x7FC)) < 0)
+ if (read_eeprom(sp, 0x7FC, &ret_data))
fail = 1;
if (ret_data != 0x01234567)
@@ -3811,7 +3826,9 @@
struct net_device *dev = (struct net_device *) data;
nic_t *sp = dev->priv;
+ sp->task_flag = 1;
s2io_close(dev);
+ sp->task_flag = 0;
sp->device_close_flag = TRUE;
s2io_open(dev);
DBG_PRINT(ERR_DBG,
@@ -4275,18 +4292,13 @@
INIT_WORK(&sp->set_link_task,
(void (*)(void *)) s2io_set_link, sp);
- if (register_netdev(dev)) {
- DBG_PRINT(ERR_DBG, "Device registration failed\n");
- goto register_failed;
- }
-
pci_save_state(sp->pdev, sp->config_space);
/* Setting swapper control on the NIC, for proper reset operation */
if (s2io_set_swapper(sp)) {
DBG_PRINT(ERR_DBG, "%s:swapper settings are wrong\n",
dev->name);
- goto register_failed;
+ goto set_swap_failed;
}
/* Fix for all "FFs" MAC address problems observed on Alpha platforms */
@@ -4363,6 +4375,11 @@
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;
+ }
+
/*
* Make Link state as off at this point, when the Link change
* interrupt comes the state will be automatically changed to
@@ -4373,9 +4390,8 @@
return 0;
- set_swap_failed:
- unregister_netdev(dev);
register_failed:
+ set_swap_failed:
iounmap(sp->bar1);
bar1_remap_failed:
iounmap(sp->bar0);
diff -urN vanilla-linux/drivers/net/s2io.h linux-2.6.8.1/drivers/net/s2io.h
--- vanilla-linux/drivers/net/s2io.h 2004-10-06 15:15:03.000000000 -0700
+++ linux-2.6.8.1/drivers/net/s2io.h 2004-10-06 16:19:52.829419288 -0700
@@ -668,6 +668,8 @@
u16 last_link_state;
#define LINK_DOWN 1
#define LINK_UP 2
+
+ int task_flag;
} nic_t;
#define RESET_ERROR 1;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
2004-10-14 1:13 Ravinandan Arakali
@ 2004-10-14 14:44 ` Jeff Garzik
2004-10-14 17:35 ` Raghavendra Koushik
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Garzik @ 2004-10-14 14:44 UTC (permalink / raw)
To: ravinandan.arakali
Cc: 'Francois Romieu', netdev, leonid.grossman,
raghavendra.koushik, rapuru.sriram
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@s2io.com>
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;
> + }
> +
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
2004-10-14 14:44 ` Jeff Garzik
@ 2004-10-14 17:35 ` Raghavendra Koushik
2004-10-15 18:52 ` Jeff Garzik
0 siblings, 1 reply; 6+ messages in thread
From: Raghavendra Koushik @ 2004-10-14 17:35 UTC (permalink / raw)
To: 'Jeff Garzik', ravinandan.arakali
Cc: 'Francois Romieu', netdev, leonid.grossman, rapuru.sriram
> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com]
> Sent: Thursday, October 14, 2004 7:44 AM
> To: ravinandan.arakali@s2io.com
> Cc: 'Francois Romieu'; netdev@oss.sgi.com; leonid.grossman@s2io.com;
> raghavendra.koushik@s2io.com; rapuru.sriram@s2io.com
> Subject: Re: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
>
> 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@s2io.com>
>
>
> 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.
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.
>
> 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();
> > + }
>
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?
> 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;
>
I believe the best way to restart the NIC is to call s2io_close and
s2io_open sequentially.
>
> 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;
> > + }
> > +
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes
2004-10-14 17:35 ` Raghavendra Koushik
@ 2004-10-15 18:52 ` Jeff Garzik
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Garzik @ 2004-10-15 18:52 UTC (permalink / raw)
To: Raghavendra Koushik
Cc: ravinandan.arakali, 'Francois Romieu', netdev,
leonid.grossman, rapuru.sriram
Raghavendra Koushik wrote:
> 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?
[...]
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-10-15 18:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-07 1:23 [PATCH 2.6.9-rc2 2/8] S2io: sw bug fixes Ravinandan Arakali
-- strict thread matches above, loose matches on Subject: below --
2004-10-07 1:24 Ravinandan Arakali
2004-10-14 1:13 Ravinandan Arakali
2004-10-14 14:44 ` Jeff Garzik
2004-10-14 17:35 ` Raghavendra Koushik
2004-10-15 18:52 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).