Re: [PATCH 5/6] IP100A correct init and close step

Re: [PATCH 5/6] IP100A correct init and close step

[Jeff Garzik]

Jesse Huang wrote:
> From: Jesse Huang <jesse@icplus.com.tw>
> 
> 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
> diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
> index f63871a..c7c22f0 100755
> --- a/drivers/net/sundance.c
> +++ b/drivers/net/sundance.c
> @@ -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()


> @@ -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.

[PATCH 5/6] IP100A correct init and close step

[Jesse Huang]

From: Jesse Huang <jesse@icplus.com.tw>

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
diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index f63871a..c7c22f0 100755
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -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);
 
-- 
1.3.GIT

Re: [PATCH 5/6] IP100A correct init and close step

[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.

----- Original Message ----- 
From: "Jeff Garzik" <jgarzik@pobox.com>
To: "Jesse Huang" <jesse@icplus.com.tw>
Cc: <linux-kernel@vger.kernel.org>; <netdev@vger.kernel.org>;
<akpm@osdl.org>
Sent: Thursday, August 17, 2006 10:51 PM
Subject: Re: [PATCH 5/6] IP100A correct init and close step


Jesse Huang wrote:
> From: Jesse Huang <jesse@icplus.com.tw>
>
> 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
> diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
> index f63871a..c7c22f0 100755
> --- a/drivers/net/sundance.c
> +++ b/drivers/net/sundance.c
> @@ -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()


> @@ -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.

Re: [PATCH 5/6] IP100A correct init and close step

[Jeff Garzik]

Jesse Huang wrote:
> 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

Re: [PATCH 5/6] IP100A correct init and close step

[Jesse Huang]

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" <jgarzik@pobox.com>
To: "Jesse Huang" <jesse@icplus.com.tw>
Cc: <linux-kernel@vger.kernel.org>; <netdev@vger.kernel.org>;
<akpm@osdl.org>
Sent: Friday, August 18, 2006 7:12 PM
Subject: Re: [PATCH 5/6] IP100A correct init and close step


Jesse Huang wrote:
> 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