IP100A correct init and close step

PATCH NAK Jesse Huang 2 participants 5 messages Aug 17, 2006 – Aug 21, 2006

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Jesse Huang

author

2006-08-17 07:21 UTC #0 lore raw

PATCH 5/6 IP100A correct init and close step

From: Jesse Huang <[email protected]>

correct init and close step

Change Logs:
correct init and close step

---

 drivers/net/sundance.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

b5e343a17f5d70d1cc9a4ba20d366bab355f64a6

drivers/net/sundance.c +9 -1

index f63871a..c7c22f0 100755
@@ -830,6 +830,11 @@ #endif
 		iowrite8(0x01, ioaddr + DebugCtrl1);
 	netif_start_queue(dev);
 
+	// 04/19/2005 Jesse fix for complete initial step
+	spin_lock(&np->lock);
+	reset_tx(dev);
+	spin_unlock(&np->lock);
+
 	iowrite16 (StatsEnable | RxEnable | TxEnable, ioaddr + MACCtrl1);
 
 	if (netif_msg_ifup(np))

@@ -1654,7 +1659,10 @@ static int netdev_close(struct net_devic
 
 	/* Disable interrupts by clearing the interrupt mask. */
 	iowrite16(0x0000, ioaddr + IntrEnable);
-
+	
+	// 04/19/2005 Jesse fix for complete initial step
+	writew(0x500, ioaddr + DMACtrl);
+	
 	/* Stop the chip's Tx and Rx processes. */
 	iowrite16(TxDisable | RxDisable | StatsDisable, ioaddr + MACCtrl1);

Signature · 1 line

1.3.GIT

Jeff Garzik

NAK

2006-08-17 14:51 UTC #1 lore raw

Quoted 18 lines · Jesse Huang

From: Jesse Huang <[email protected]>
correct init and close step
Change Logs:
correct init and close step
---
 drivers/net/sundance.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
b5e343a17f5d70d1cc9a4ba20d366bab355f64a6
drivers/net/sundance.c +0 -0
index f63871a..c7c22f0 100755

drivers/net/sundance.c #endif · line 837

Quoted 8 lines

drivers/net/sundance.c +5 -0

@@ -830,6 +830,11 @@ #endif
 		iowrite8(0x01, ioaddr + DebugCtrl1);
 	netif_start_queue(dev);
 
+	// 04/19/2005 Jesse fix for complete initial step
+	spin_lock(&np->lock);
+	reset_tx(dev);
+	spin_unlock(&np->lock);
+

NAK -- ineffective locking.

If you need locking here, you must use spin_lock_irqsave()

drivers/net/sundance.c static int netdev_close(struct net_devic · line 1664

Quoted 8 lines

drivers/net/sundance.c +3 -1

@@ -1654,7 +1659,10 @@ static int netdev_close(struct net_devic
 
 	/* Disable interrupts by clearing the interrupt mask. */
 	iowrite16(0x0000, ioaddr + IntrEnable);
-
+	
+	// 04/19/2005 Jesse fix for complete initial step
+	writew(0x500, ioaddr + DMACtrl);

NAK:

1) date and author information is already present in the kernel changelog. It should not be present in the source code.

2) The comment (or patch description) fails to describe -why- this change is needed. It only described what is being changes, which is already obvious from reading the source code.

Jesse Huang

author NAK

2006-08-18 06:12 UTC #2 lore raw

Hi Jeff:
(1)Should I change to :
spin_lock_irqsave(&np->lock,flags);
reset_tx(dev);
spin_lock_irqrestore(&np->lock,flags);

(2)I will remove date and author information out of source code comment.

----- Original Message -----
From: "Jeff Garzik" <[email protected]>
To: "Jesse Huang" <[email protected]>
Cc: <[email protected]>; <[email protected]>; <[email protected]>
Sent: Thursday, August 17, 2006 10:51 PM
Subject: Re: [PATCH 5/6] IP100A correct init and close step

Quoted 18 lines · Jesse Huang

From: Jesse Huang <[email protected]>
correct init and close step
Change Logs:
correct init and close step
---
 drivers/net/sundance.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)
b5e343a17f5d70d1cc9a4ba20d366bab355f64a6
drivers/net/sundance.c +0 -0
index f63871a..c7c22f0 100755

drivers/net/sundance.c #endif · line 837

Quoted 8 lines

drivers/net/sundance.c +5 -0

@@ -830,6 +830,11 @@ #endif
 iowrite8(0x01, ioaddr + DebugCtrl1);
 netif_start_queue(dev);
 
+ // 04/19/2005 Jesse fix for complete initial step
+ spin_lock(&np->lock);
+ reset_tx(dev);
+ spin_unlock(&np->lock);
+

NAK -- ineffective locking.

If you need locking here, you must use spin_lock_irqsave()

drivers/net/sundance.c static int netdev_close(struct net_devic · line 1664

Quoted 8 lines

drivers/net/sundance.c +3 -1

@@ -1654,7 +1659,10 @@ static int netdev_close(struct net_devic
 
 /* Disable interrupts by clearing the interrupt mask. */
 iowrite16(0x0000, ioaddr + IntrEnable);
-
+
+ // 04/19/2005 Jesse fix for complete initial step
+ writew(0x500, ioaddr + DMACtrl);

NAK:

1) date and author information is already present in the kernel changelog. It should not be present in the source code.

2) The comment (or patch description) fails to describe -why- this change is needed. It only described what is being changes, which is already obvious from reading the source code.

Jeff Garzik

2006-08-18 11:12 UTC #3 lore raw

Quoted 7 lines · Jesse Huang

Hi Jeff:
(1)Should I change to :
spin_lock_irqsave(&np->lock,flags);
reset_tx(dev);
spin_lock_irqrestore(&np->lock,flags);
(2)I will remove date and author information out of source code comment.

Correct.

Also:

(3) Use iowrite16(), not writew(). I just noticed this bug. iowrite16() will work for both MMIO and IO cycles, writew() only works for MMIO.

(4) We need a description of why this change is needed. What does writing 0x500 to DMACtrl actually do? Why do we need to do this?

Jeff

Jesse Huang

author

2006-08-21 03:38 UTC #4 lore raw

Hi Jeff:

(3)Yes, This is a bug, I will correct it. Thanks.

(4)This will halt TxDMA and RxDMA, after that will let reseting safely. Should I add description in source code or in change log?

Thanks!

Jesse

----- Original Message -----
From: "Jeff Garzik" <[email protected]>
To: "Jesse Huang" <[email protected]>
Cc: <[email protected]>; <[email protected]>; <[email protected]>
Sent: Friday, August 18, 2006 7:12 PM
Subject: Re: [PATCH 5/6] IP100A correct init and close step

Quoted 7 lines · Jesse Huang

Hi Jeff:
(1)Should I change to :
spin_lock_irqsave(&np->lock,flags);
reset_tx(dev);
spin_lock_irqrestore(&np->lock,flags);
(2)I will remove date and author information out of source code comment.

Correct.

Also:

(3) Use iowrite16(), not writew(). I just noticed this bug. iowrite16() will work for both MMIO and IO cycles, writew() only works for MMIO.

(4) We need a description of why this change is needed. What does writing 0x500 to DMACtrl actually do? Why do we need to do this?

Jeff