[PATCH 0/8] Implement NAPI in et131x

[PATCH 0/8] Implement NAPI in et131x

[Mark Einon]

Let's put a NAPI on this baby...

NAPI support was identified by Dave Miller <davem@davemloft.net> as a
must to get this driver out of staging. This patchset first tidies up
the code enough to make adding NAPI easier, and the final patch mostly
adds the 'boilerplate' NAPI code as described by:

http://www.linuxfoundation.org/collaborate/workgroups/networking/napi

The code has been tested on hardware, with no issues found to date.

**GregKH** - this patchset is based upon my previous bugfix 'Fix errors
caused by phydev->addr accesses' which is currently only on your
staging-linus branch, not staging-next - so won't apply cleanly in it's
current state. Please let me know if I need to do anything about this.

Cheers,

Mark

---
Mark Einon (8):
  staging: et131x: Use eth_mac_addr() instead of duplicating the
    functionality
  staging: et131x: Don't handle rx/tx packets when changing mtu
  staging: et131x: Use for loop to initialise contiguous registers to
    zero
  staging: et131x: Use for loop to initialise contiguous macstat
    registers to zero
  staging: et131x: Remove unnecessary i2c_wack variable
  staging: et131x: Rename NUM_PACKETS_HANDLED to MAX_PACKETS_HANDLED
  staging: et131x: Fix ET_INTR_TXDMA_ISR register name typo
  staging: et131x: Implement NAPI support

 drivers/staging/et131x/README   |   1 -
 drivers/staging/et131x/et131x.c | 271 ++++++++++------------------------------
 drivers/staging/et131x/et131x.h |  96 +-------------
 3 files changed, 69 insertions(+), 299 deletions(-)

-- 
2.1.0

[PATCH 0/8] Implement NAPI in et131x

[Mark Einon]

Let's put a NAPI on this baby...

NAPI support was identified by Dave Miller <davem@davemloft.net> as a
must to get this driver out of staging. This patchset first tidies up
the code enough to make adding NAPI easier, and the final patch mostly
adds the 'boilerplate' NAPI code as described by:

http://www.linuxfoundation.org/collaborate/workgroups/networking/napi

The code has been tested on hardware, with no issues found to date.

**GregKH** - this patchset is based upon my previous bugfix 'Fix errors
caused by phydev->addr accesses' which is currently only on your
staging-linus branch, not staging-next - so won't apply cleanly in it's
current state. Please let me know if I need to do anything about this.

Cheers,

Mark

---
Mark Einon (8):
  staging: et131x: Use eth_mac_addr() instead of duplicating the
    functionality
  staging: et131x: Don't handle rx/tx packets when changing mtu
  staging: et131x: Use for loop to initialise contiguous registers to
    zero
  staging: et131x: Use for loop to initialise contiguous macstat
    registers to zero
  staging: et131x: Remove unnecessary i2c_wack variable
  staging: et131x: Rename NUM_PACKETS_HANDLED to MAX_PACKETS_HANDLED
  staging: et131x: Fix ET_INTR_TXDMA_ISR register name typo
  staging: et131x: Implement NAPI support

 drivers/staging/et131x/README   |   1 -
 drivers/staging/et131x/et131x.c | 271 ++++++++++------------------------------
 drivers/staging/et131x/et131x.h |  96 +-------------
 3 files changed, 69 insertions(+), 299 deletions(-)

-- 
2.1.0

[PATCH 1/8] staging: et131x: Use eth_mac_addr() instead of duplicating the functionality

[Mark Einon]

There's already working code to set the mac address, so let's use it.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 53 +----------------------------------------
 1 file changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 831b7c6..ac6700b 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -4468,57 +4468,6 @@ static int et131x_change_mtu(struct net_device *netdev, int new_mtu)
 	return result;
 }
 
-/* et131x_set_mac_addr - handler to change the MAC address for the device */
-static int et131x_set_mac_addr(struct net_device *netdev, void *new_mac)
-{
-	int result = 0;
-	struct et131x_adapter *adapter = netdev_priv(netdev);
-	struct sockaddr *address = new_mac;
-
-	if (adapter == NULL)
-		return -ENODEV;
-
-	/* Make sure the requested MAC is valid */
-	if (!is_valid_ether_addr(address->sa_data))
-		return -EADDRNOTAVAIL;
-
-	et131x_disable_txrx(netdev);
-	et131x_handle_send_interrupt(adapter);
-	et131x_handle_recv_interrupt(adapter);
-
-	/* Set the new MAC */
-	/* netdev->set_mac_address  = &new_mac; */
-
-	memcpy(netdev->dev_addr, address->sa_data, netdev->addr_len);
-
-	netdev_info(netdev, "Setting MAC address to %pM\n",
-		    netdev->dev_addr);
-
-	/* Free Rx DMA memory */
-	et131x_adapter_memory_free(adapter);
-
-	et131x_soft_reset(adapter);
-
-	/* Alloc and init Rx DMA memory */
-	result = et131x_adapter_memory_alloc(adapter);
-	if (result != 0) {
-		dev_err(&adapter->pdev->dev,
-			"Change MAC failed; couldn't re-alloc DMA memory\n");
-		return result;
-	}
-
-	et131x_init_send(adapter);
-
-	et131x_hwaddr_init(adapter);
-
-	/* Init the device with the new settings */
-	et131x_adapter_setup(adapter);
-
-	et131x_enable_txrx(netdev);
-
-	return result;
-}
-
 static const struct net_device_ops et131x_netdev_ops = {
 	.ndo_open		= et131x_open,
 	.ndo_stop		= et131x_close,
@@ -4526,7 +4475,7 @@ static const struct net_device_ops et131x_netdev_ops = {
 	.ndo_set_rx_mode	= et131x_multicast,
 	.ndo_tx_timeout		= et131x_tx_timeout,
 	.ndo_change_mtu		= et131x_change_mtu,
-	.ndo_set_mac_address	= et131x_set_mac_addr,
+	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_get_stats		= et131x_stats,
 	.ndo_do_ioctl		= et131x_ioctl,
-- 
2.1.0

[PATCH 1/8] staging: et131x: Use eth_mac_addr() instead of duplicating the functionality

[Mark Einon]

There's already working code to set the mac address, so let's use it.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 53 +----------------------------------------
 1 file changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 831b7c6..ac6700b 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -4468,57 +4468,6 @@ static int et131x_change_mtu(struct net_device *netdev, int new_mtu)
 	return result;
 }
 
-/* et131x_set_mac_addr - handler to change the MAC address for the device */
-static int et131x_set_mac_addr(struct net_device *netdev, void *new_mac)
-{
-	int result = 0;
-	struct et131x_adapter *adapter = netdev_priv(netdev);
-	struct sockaddr *address = new_mac;
-
-	if (adapter == NULL)
-		return -ENODEV;
-
-	/* Make sure the requested MAC is valid */
-	if (!is_valid_ether_addr(address->sa_data))
-		return -EADDRNOTAVAIL;
-
-	et131x_disable_txrx(netdev);
-	et131x_handle_send_interrupt(adapter);
-	et131x_handle_recv_interrupt(adapter);
-
-	/* Set the new MAC */
-	/* netdev->set_mac_address  = &new_mac; */
-
-	memcpy(netdev->dev_addr, address->sa_data, netdev->addr_len);
-
-	netdev_info(netdev, "Setting MAC address to %pM\n",
-		    netdev->dev_addr);
-
-	/* Free Rx DMA memory */
-	et131x_adapter_memory_free(adapter);
-
-	et131x_soft_reset(adapter);
-
-	/* Alloc and init Rx DMA memory */
-	result = et131x_adapter_memory_alloc(adapter);
-	if (result != 0) {
-		dev_err(&adapter->pdev->dev,
-			"Change MAC failed; couldn't re-alloc DMA memory\n");
-		return result;
-	}
-
-	et131x_init_send(adapter);
-
-	et131x_hwaddr_init(adapter);
-
-	/* Init the device with the new settings */
-	et131x_adapter_setup(adapter);
-
-	et131x_enable_txrx(netdev);
-
-	return result;
-}
-
 static const struct net_device_ops et131x_netdev_ops = {
 	.ndo_open		= et131x_open,
 	.ndo_stop		= et131x_close,
@@ -4526,7 +4475,7 @@ static const struct net_device_ops et131x_netdev_ops = {
 	.ndo_set_rx_mode	= et131x_multicast,
 	.ndo_tx_timeout		= et131x_tx_timeout,
 	.ndo_change_mtu		= et131x_change_mtu,
-	.ndo_set_mac_address	= et131x_set_mac_addr,
+	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_get_stats		= et131x_stats,
 	.ndo_do_ioctl		= et131x_ioctl,
-- 
2.1.0

[PATCH 2/8] staging: et131x: Don't handle rx/tx packets when changing mtu

[Mark Einon]

There's no need to handle any rx/tx interrupts in the middle of an mtu
change, so don't.

After this change, receive and transmit interrupts are only handled in
one place, which paves the way to using NAPI.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index ac6700b..fffe763 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -4434,8 +4434,6 @@ static int et131x_change_mtu(struct net_device *netdev, int new_mtu)
 		return -EINVAL;
 
 	et131x_disable_txrx(netdev);
-	et131x_handle_send_interrupt(adapter);
-	et131x_handle_recv_interrupt(adapter);
 
 	/* Set the new MTU */
 	netdev->mtu = new_mtu;
-- 
2.1.0

[PATCH 2/8] staging: et131x: Don't handle rx/tx packets when changing mtu

[Mark Einon]

There's no need to handle any rx/tx interrupts in the middle of an mtu
change, so don't.

After this change, receive and transmit interrupts are only handled in
one place, which paves the way to using NAPI.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index ac6700b..fffe763 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -4434,8 +4434,6 @@ static int et131x_change_mtu(struct net_device *netdev, int new_mtu)
 		return -EINVAL;
 
 	et131x_disable_txrx(netdev);
-	et131x_handle_send_interrupt(adapter);
-	et131x_handle_recv_interrupt(adapter);
 
 	/* Set the new MTU */
 	netdev->mtu = new_mtu;
-- 
2.1.0

[PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero

[Mark Einon]

Replace a long list of contiguous writel() calls with a for loop iterating
over the same values.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index fffe763..44cc684 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
 	u32 sa_lo;
 	u32 sa_hi = 0;
 	u32 pf_ctrl = 0;
+	u32 *wolw;
 
 	/* Disable the MAC while it is being configured (also disable WOL) */
 	writel(0x8, &rxmac->ctrl);
@@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
 	 * its default Values of 0x00000000 because there are not WOL masks
 	 * as of this time.
 	 */
-	writel(0, &rxmac->mask0_word0);
-	writel(0, &rxmac->mask0_word1);
-	writel(0, &rxmac->mask0_word2);
-	writel(0, &rxmac->mask0_word3);
-
-	writel(0, &rxmac->mask1_word0);
-	writel(0, &rxmac->mask1_word1);
-	writel(0, &rxmac->mask1_word2);
-	writel(0, &rxmac->mask1_word3);
-
-	writel(0, &rxmac->mask2_word0);
-	writel(0, &rxmac->mask2_word1);
-	writel(0, &rxmac->mask2_word2);
-	writel(0, &rxmac->mask2_word3);
-
-	writel(0, &rxmac->mask3_word0);
-	writel(0, &rxmac->mask3_word1);
-	writel(0, &rxmac->mask3_word2);
-	writel(0, &rxmac->mask3_word3);
-
-	writel(0, &rxmac->mask4_word0);
-	writel(0, &rxmac->mask4_word1);
-	writel(0, &rxmac->mask4_word2);
-	writel(0, &rxmac->mask4_word3);
+	for (wolw = &rxmac->mask0_word0; wolw <= &rxmac->mask4_word3; wolw++)
+		writel(0, wolw);
 
 	/* Lets setup the WOL Source Address */
 	sa_lo = (adapter->addr[2] << ET_RX_WOL_LO_SA3_SHIFT) |
-- 
2.1.0

[PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero

[Mark Einon]

Replace a long list of contiguous writel() calls with a for loop iterating
over the same values.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index fffe763..44cc684 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
 	u32 sa_lo;
 	u32 sa_hi = 0;
 	u32 pf_ctrl = 0;
+	u32 *wolw;
 
 	/* Disable the MAC while it is being configured (also disable WOL) */
 	writel(0x8, &rxmac->ctrl);
@@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
 	 * its default Values of 0x00000000 because there are not WOL masks
 	 * as of this time.
 	 */
-	writel(0, &rxmac->mask0_word0);
-	writel(0, &rxmac->mask0_word1);
-	writel(0, &rxmac->mask0_word2);
-	writel(0, &rxmac->mask0_word3);
-
-	writel(0, &rxmac->mask1_word0);
-	writel(0, &rxmac->mask1_word1);
-	writel(0, &rxmac->mask1_word2);
-	writel(0, &rxmac->mask1_word3);
-
-	writel(0, &rxmac->mask2_word0);
-	writel(0, &rxmac->mask2_word1);
-	writel(0, &rxmac->mask2_word2);
-	writel(0, &rxmac->mask2_word3);
-
-	writel(0, &rxmac->mask3_word0);
-	writel(0, &rxmac->mask3_word1);
-	writel(0, &rxmac->mask3_word2);
-	writel(0, &rxmac->mask3_word3);
-
-	writel(0, &rxmac->mask4_word0);
-	writel(0, &rxmac->mask4_word1);
-	writel(0, &rxmac->mask4_word2);
-	writel(0, &rxmac->mask4_word3);
+	for (wolw = &rxmac->mask0_word0; wolw <= &rxmac->mask4_word3; wolw++)
+		writel(0, wolw);
 
 	/* Lets setup the WOL Source Address */
 	sa_lo = (adapter->addr[2] << ET_RX_WOL_LO_SA3_SHIFT) |
-- 
2.1.0

Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero

[Greg KH]

On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote:
> Replace a long list of contiguous writel() calls with a for loop iterating
> over the same values.
> 
> Signed-off-by: Mark Einon <mark.einon@gmail.com>
> ---
>  drivers/staging/et131x/et131x.c | 27 +++------------------------
>  1 file changed, 3 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> index fffe763..44cc684 100644
> --- a/drivers/staging/et131x/et131x.c
> +++ b/drivers/staging/et131x/et131x.c
> @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
>  	u32 sa_lo;
>  	u32 sa_hi = 0;
>  	u32 pf_ctrl = 0;
> +	u32 *wolw;
>  
>  	/* Disable the MAC while it is being configured (also disable WOL) */
>  	writel(0x8, &rxmac->ctrl);
> @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
>  	 * its default Values of 0x00000000 because there are not WOL masks
>  	 * as of this time.
>  	 */
> -	writel(0, &rxmac->mask0_word0);
> -	writel(0, &rxmac->mask0_word1);
> -	writel(0, &rxmac->mask0_word2);
> -	writel(0, &rxmac->mask0_word3);
> -
> -	writel(0, &rxmac->mask1_word0);
> -	writel(0, &rxmac->mask1_word1);
> -	writel(0, &rxmac->mask1_word2);
> -	writel(0, &rxmac->mask1_word3);
> -
> -	writel(0, &rxmac->mask2_word0);
> -	writel(0, &rxmac->mask2_word1);
> -	writel(0, &rxmac->mask2_word2);
> -	writel(0, &rxmac->mask2_word3);
> -
> -	writel(0, &rxmac->mask3_word0);
> -	writel(0, &rxmac->mask3_word1);
> -	writel(0, &rxmac->mask3_word2);
> -	writel(0, &rxmac->mask3_word3);
> -
> -	writel(0, &rxmac->mask4_word0);
> -	writel(0, &rxmac->mask4_word1);
> -	writel(0, &rxmac->mask4_word2);
> -	writel(0, &rxmac->mask4_word3);
> +	for (wolw = &rxmac->mask0_word0; wolw <= &rxmac->mask4_word3; wolw++)
> +		writel(0, wolw);

You are now only writing to all locations 1 time, instead of 4 times,
like before, are you sure that is ok?  Hardware is flaky, sometimes it
wants to be written to multiple times...

greg k-h

Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero

[Mark Einon]

On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote:
> On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote:
> > Replace a long list of contiguous writel() calls with a for loop iterating
> > over the same values.
> > 
> > Signed-off-by: Mark Einon <mark.einon@gmail.com>
> > ---
> >  drivers/staging/et131x/et131x.c | 27 +++------------------------
> >  1 file changed, 3 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> > index fffe763..44cc684 100644
> > --- a/drivers/staging/et131x/et131x.c
> > +++ b/drivers/staging/et131x/et131x.c
> > @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
> >  	u32 sa_lo;
> >  	u32 sa_hi = 0;
> >  	u32 pf_ctrl = 0;
> > +	u32 *wolw;
> >  
> >  	/* Disable the MAC while it is being configured (also disable WOL) */
> >  	writel(0x8, &rxmac->ctrl);
> > @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
> >  	 * its default Values of 0x00000000 because there are not WOL masks
> >  	 * as of this time.
> >  	 */
> > -	writel(0, &rxmac->mask0_word0);
> > -	writel(0, &rxmac->mask0_word1);
> > -	writel(0, &rxmac->mask0_word2);
> > -	writel(0, &rxmac->mask0_word3);
> > -
> > -	writel(0, &rxmac->mask1_word0);
> > -	writel(0, &rxmac->mask1_word1);
> > -	writel(0, &rxmac->mask1_word2);
> > -	writel(0, &rxmac->mask1_word3);
> > -
> > -	writel(0, &rxmac->mask2_word0);
> > -	writel(0, &rxmac->mask2_word1);
> > -	writel(0, &rxmac->mask2_word2);
> > -	writel(0, &rxmac->mask2_word3);
> > -
> > -	writel(0, &rxmac->mask3_word0);
> > -	writel(0, &rxmac->mask3_word1);
> > -	writel(0, &rxmac->mask3_word2);
> > -	writel(0, &rxmac->mask3_word3);
> > -
> > -	writel(0, &rxmac->mask4_word0);
> > -	writel(0, &rxmac->mask4_word1);
> > -	writel(0, &rxmac->mask4_word2);
> > -	writel(0, &rxmac->mask4_word3);
> > +	for (wolw = &rxmac->mask0_word0; wolw <= &rxmac->mask4_word3; wolw++)
> > +		writel(0, wolw);
> 
> You are now only writing to all locations 1 time, instead of 4 times,
> like before, are you sure that is ok?  Hardware is flaky, sometimes it
> wants to be written to multiple times...

Hi Greg,

Thanks for the review.

As far as my understanding goes, the new code is equivalent to the old
code - it's a little confusing that the name refers to a word, but the
masks are all 32 bit values, and the loop iterates over the contiguous
list of masks found in txmac_regs (et131x.h:891 - the masks are also
unused in the driver after being set).

Or am I missing something here?

Cheers,

Mark

Re: [PATCH 3/8] staging: et131x: Use for loop to initialise contiguous registers to zero

[Greg KH]

On Sun, Aug 31, 2014 at 03:25:03PM +0100, Mark Einon wrote:
> On Sat, Aug 30, 2014 at 01:32:16PM -0700, Greg KH wrote:
> > On Wed, Aug 20, 2014 at 11:17:53PM +0100, Mark Einon wrote:
> > > Replace a long list of contiguous writel() calls with a for loop iterating
> > > over the same values.
> > > 
> > > Signed-off-by: Mark Einon <mark.einon@gmail.com>
> > > ---
> > >  drivers/staging/et131x/et131x.c | 27 +++------------------------
> > >  1 file changed, 3 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> > > index fffe763..44cc684 100644
> > > --- a/drivers/staging/et131x/et131x.c
> > > +++ b/drivers/staging/et131x/et131x.c
> > > @@ -1138,6 +1138,7 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
> > >  	u32 sa_lo;
> > >  	u32 sa_hi = 0;
> > >  	u32 pf_ctrl = 0;
> > > +	u32 *wolw;
> > >  
> > >  	/* Disable the MAC while it is being configured (also disable WOL) */
> > >  	writel(0x8, &rxmac->ctrl);
> > > @@ -1151,30 +1152,8 @@ static void et1310_config_rxmac_regs(struct et131x_adapter *adapter)
> > >  	 * its default Values of 0x00000000 because there are not WOL masks
> > >  	 * as of this time.
> > >  	 */
> > > -	writel(0, &rxmac->mask0_word0);
> > > -	writel(0, &rxmac->mask0_word1);
> > > -	writel(0, &rxmac->mask0_word2);
> > > -	writel(0, &rxmac->mask0_word3);
> > > -
> > > -	writel(0, &rxmac->mask1_word0);
> > > -	writel(0, &rxmac->mask1_word1);
> > > -	writel(0, &rxmac->mask1_word2);
> > > -	writel(0, &rxmac->mask1_word3);
> > > -
> > > -	writel(0, &rxmac->mask2_word0);
> > > -	writel(0, &rxmac->mask2_word1);
> > > -	writel(0, &rxmac->mask2_word2);
> > > -	writel(0, &rxmac->mask2_word3);
> > > -
> > > -	writel(0, &rxmac->mask3_word0);
> > > -	writel(0, &rxmac->mask3_word1);
> > > -	writel(0, &rxmac->mask3_word2);
> > > -	writel(0, &rxmac->mask3_word3);
> > > -
> > > -	writel(0, &rxmac->mask4_word0);
> > > -	writel(0, &rxmac->mask4_word1);
> > > -	writel(0, &rxmac->mask4_word2);
> > > -	writel(0, &rxmac->mask4_word3);
> > > +	for (wolw = &rxmac->mask0_word0; wolw <= &rxmac->mask4_word3; wolw++)
> > > +		writel(0, wolw);
> > 
> > You are now only writing to all locations 1 time, instead of 4 times,
> > like before, are you sure that is ok?  Hardware is flaky, sometimes it
> > wants to be written to multiple times...
> 
> Hi Greg,
> 
> Thanks for the review.
> 
> As far as my understanding goes, the new code is equivalent to the old
> code - it's a little confusing that the name refers to a word, but the
> masks are all 32 bit values, and the loop iterates over the contiguous
> list of masks found in txmac_regs (et131x.h:891 - the masks are also
> unused in the driver after being set).
> 
> Or am I missing something here?

No, I was wrong, sorry, your explanation makes sense, sorry for the
noise.

greg k-h

[PATCH 4/8] staging: et131x: Use for loop to initialise contiguous macstat registers to zero

[Mark Einon]

Replace a long list of contiguous writel() calls with a for loop iterating
over the same address values.

Also remove redundant comments on the macstat registers, the variable names
are good enough.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 59 +++----------------------
 drivers/staging/et131x/et131x.h | 96 +----------------------------------------
 2 files changed, 7 insertions(+), 148 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 44cc684..fc18e8d 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1257,60 +1257,13 @@ static void et1310_config_txmac_regs(struct et131x_adapter *adapter)
 
 static void et1310_config_macstat_regs(struct et131x_adapter *adapter)
 {
-	struct macstat_regs __iomem *macstat =
-		&adapter->regs->macstat;
+	struct macstat_regs __iomem *macstat = &adapter->regs->macstat;
+	u32 *reg;
 
-	/* Next we need to initialize all the macstat registers to zero on
-	 * the device.
-	 */
-	writel(0, &macstat->txrx_0_64_byte_frames);
-	writel(0, &macstat->txrx_65_127_byte_frames);
-	writel(0, &macstat->txrx_128_255_byte_frames);
-	writel(0, &macstat->txrx_256_511_byte_frames);
-	writel(0, &macstat->txrx_512_1023_byte_frames);
-	writel(0, &macstat->txrx_1024_1518_byte_frames);
-	writel(0, &macstat->txrx_1519_1522_gvln_frames);
-
-	writel(0, &macstat->rx_bytes);
-	writel(0, &macstat->rx_packets);
-	writel(0, &macstat->rx_fcs_errs);
-	writel(0, &macstat->rx_multicast_packets);
-	writel(0, &macstat->rx_broadcast_packets);
-	writel(0, &macstat->rx_control_frames);
-	writel(0, &macstat->rx_pause_frames);
-	writel(0, &macstat->rx_unknown_opcodes);
-	writel(0, &macstat->rx_align_errs);
-	writel(0, &macstat->rx_frame_len_errs);
-	writel(0, &macstat->rx_code_errs);
-	writel(0, &macstat->rx_carrier_sense_errs);
-	writel(0, &macstat->rx_undersize_packets);
-	writel(0, &macstat->rx_oversize_packets);
-	writel(0, &macstat->rx_fragment_packets);
-	writel(0, &macstat->rx_jabbers);
-	writel(0, &macstat->rx_drops);
-
-	writel(0, &macstat->tx_bytes);
-	writel(0, &macstat->tx_packets);
-	writel(0, &macstat->tx_multicast_packets);
-	writel(0, &macstat->tx_broadcast_packets);
-	writel(0, &macstat->tx_pause_frames);
-	writel(0, &macstat->tx_deferred);
-	writel(0, &macstat->tx_excessive_deferred);
-	writel(0, &macstat->tx_single_collisions);
-	writel(0, &macstat->tx_multiple_collisions);
-	writel(0, &macstat->tx_late_collisions);
-	writel(0, &macstat->tx_excessive_collisions);
-	writel(0, &macstat->tx_total_collisions);
-	writel(0, &macstat->tx_pause_honored_frames);
-	writel(0, &macstat->tx_drops);
-	writel(0, &macstat->tx_jabbers);
-	writel(0, &macstat->tx_fcs_errs);
-	writel(0, &macstat->tx_control_frames);
-	writel(0, &macstat->tx_oversize_frames);
-	writel(0, &macstat->tx_undersize_frames);
-	writel(0, &macstat->tx_fragments);
-	writel(0, &macstat->carry_reg1);
-	writel(0, &macstat->carry_reg2);
+	/* initialize all the macstat registers to zero on the device  */
+	for (reg = &macstat->txrx_0_64_byte_frames;
+	     reg <= &macstat->carry_reg2; reg++)
+		writel(0, reg);
 
 	/* Unmask any counters that we want to track the overflow of.
 	 * Initially this will be all counters.  It may become clear later
diff --git a/drivers/staging/et131x/et131x.h b/drivers/staging/et131x/et131x.h
index 1318439..95d6d45 100644
--- a/drivers/staging/et131x/et131x.h
+++ b/drivers/staging/et131x/et131x.h
@@ -1259,148 +1259,54 @@ struct mac_regs {					/* Location: */
 struct macstat_regs {			/* Location: */
 	u32 pad[32];			/*  0x6000 - 607C */
 
-	/* Tx/Rx 0-64 Byte Frame Counter */
+	/* counters */
 	u32 txrx_0_64_byte_frames;	/*  0x6080 */
-
-	/* Tx/Rx 65-127 Byte Frame Counter */
 	u32 txrx_65_127_byte_frames;	/*  0x6084 */
-
-	/* Tx/Rx 128-255 Byte Frame Counter */
 	u32 txrx_128_255_byte_frames;	/*  0x6088 */
-
-	/* Tx/Rx 256-511 Byte Frame Counter */
 	u32 txrx_256_511_byte_frames;	/*  0x608C */
-
-	/* Tx/Rx 512-1023 Byte Frame Counter */
 	u32 txrx_512_1023_byte_frames;	/*  0x6090 */
-
-	/* Tx/Rx 1024-1518 Byte Frame Counter */
 	u32 txrx_1024_1518_byte_frames;	/*  0x6094 */
-
-	/* Tx/Rx 1519-1522 Byte Good VLAN Frame Count */
 	u32 txrx_1519_1522_gvln_frames;	/*  0x6098 */
-
-	/* Rx Byte Counter */
 	u32 rx_bytes;			/*  0x609C */
-
-	/* Rx Packet Counter */
 	u32 rx_packets;			/*  0x60A0 */
-
-	/* Rx FCS Error Counter */
 	u32 rx_fcs_errs;		/*  0x60A4 */
-
-	/* Rx Multicast Packet Counter */
 	u32 rx_multicast_packets;	/*  0x60A8 */
-
-	/* Rx Broadcast Packet Counter */
 	u32 rx_broadcast_packets;	/*  0x60AC */
-
-	/* Rx Control Frame Packet Counter */
 	u32 rx_control_frames;		/*  0x60B0 */
-
-	/* Rx Pause Frame Packet Counter */
 	u32 rx_pause_frames;		/*  0x60B4 */
-
-	/* Rx Unknown OP Code Counter */
 	u32 rx_unknown_opcodes;		/*  0x60B8 */
-
-	/* Rx Alignment Error Counter */
 	u32 rx_align_errs;		/*  0x60BC */
-
-	/* Rx Frame Length Error Counter */
 	u32 rx_frame_len_errs;		/*  0x60C0 */
-
-	/* Rx Code Error Counter */
 	u32 rx_code_errs;		/*  0x60C4 */
-
-	/* Rx Carrier Sense Error Counter */
 	u32 rx_carrier_sense_errs;	/*  0x60C8 */
-
-	/* Rx Undersize Packet Counter */
 	u32 rx_undersize_packets;	/*  0x60CC */
-
-	/* Rx Oversize Packet Counter */
 	u32 rx_oversize_packets;	/*  0x60D0 */
-
-	/* Rx Fragment Counter */
 	u32 rx_fragment_packets;	/*  0x60D4 */
-
-	/* Rx Jabber Counter */
 	u32 rx_jabbers;			/*  0x60D8 */
-
-	/* Rx Drop */
 	u32 rx_drops;			/*  0x60DC */
-
-	/* Tx Byte Counter */
 	u32 tx_bytes;			/*  0x60E0 */
-
-	/* Tx Packet Counter */
 	u32 tx_packets;			/*  0x60E4 */
-
-	/* Tx Multicast Packet Counter */
 	u32 tx_multicast_packets;	/*  0x60E8 */
-
-	/* Tx Broadcast Packet Counter */
 	u32 tx_broadcast_packets;	/*  0x60EC */
-
-	/* Tx Pause Control Frame Counter */
 	u32 tx_pause_frames;		/*  0x60F0 */
-
-	/* Tx Deferral Packet Counter */
 	u32 tx_deferred;		/*  0x60F4 */
-
-	/* Tx Excessive Deferral Packet Counter */
 	u32 tx_excessive_deferred;	/*  0x60F8 */
-
-	/* Tx Single Collision Packet Counter */
 	u32 tx_single_collisions;	/*  0x60FC */
-
-	/* Tx Multiple Collision Packet Counter */
 	u32 tx_multiple_collisions;	/*  0x6100 */
-
-	/* Tx Late Collision Packet Counter */
 	u32 tx_late_collisions;		/*  0x6104 */
-
-	/* Tx Excessive Collision Packet Counter */
 	u32 tx_excessive_collisions;	/*  0x6108 */
-
-	/* Tx Total Collision Packet Counter */
 	u32 tx_total_collisions;	/*  0x610C */
-
-	/* Tx Pause Frame Honored Counter */
 	u32 tx_pause_honored_frames;	/*  0x6110 */
-
-	/* Tx Drop Frame Counter */
 	u32 tx_drops;			/*  0x6114 */
-
-	/* Tx Jabber Frame Counter */
 	u32 tx_jabbers;			/*  0x6118 */
-
-	/* Tx FCS Error Counter */
 	u32 tx_fcs_errs;		/*  0x611C */
-
-	/* Tx Control Frame Counter */
 	u32 tx_control_frames;		/*  0x6120 */
-
-	/* Tx Oversize Frame Counter */
 	u32 tx_oversize_frames;		/*  0x6124 */
-
-	/* Tx Undersize Frame Counter */
 	u32 tx_undersize_frames;	/*  0x6128 */
-
-	/* Tx Fragments Frame Counter */
 	u32 tx_fragments;		/*  0x612C */
-
-	/* Carry Register One Register */
 	u32 carry_reg1;			/*  0x6130 */
-
-	/* Carry Register Two Register */
 	u32 carry_reg2;			/*  0x6134 */
-
-	/* Carry Register One Mask Register */
 	u32 carry_reg1_mask;		/*  0x6138 */
-
-	/* Carry Register Two Mask Register */
 	u32 carry_reg2_mask;		/*  0x613C */
 };
 
-- 
2.1.0

[PATCH 4/8] staging: et131x: Use for loop to initialise contiguous macstat registers to zero

[Mark Einon]

Replace a long list of contiguous writel() calls with a for loop iterating
over the same address values.

Also remove redundant comments on the macstat registers, the variable names
are good enough.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 59 +++----------------------
 drivers/staging/et131x/et131x.h | 96 +----------------------------------------
 2 files changed, 7 insertions(+), 148 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 44cc684..fc18e8d 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -1257,60 +1257,13 @@ static void et1310_config_txmac_regs(struct et131x_adapter *adapter)
 
 static void et1310_config_macstat_regs(struct et131x_adapter *adapter)
 {
-	struct macstat_regs __iomem *macstat =
-		&adapter->regs->macstat;
+	struct macstat_regs __iomem *macstat = &adapter->regs->macstat;
+	u32 *reg;
 
-	/* Next we need to initialize all the macstat registers to zero on
-	 * the device.
-	 */
-	writel(0, &macstat->txrx_0_64_byte_frames);
-	writel(0, &macstat->txrx_65_127_byte_frames);
-	writel(0, &macstat->txrx_128_255_byte_frames);
-	writel(0, &macstat->txrx_256_511_byte_frames);
-	writel(0, &macstat->txrx_512_1023_byte_frames);
-	writel(0, &macstat->txrx_1024_1518_byte_frames);
-	writel(0, &macstat->txrx_1519_1522_gvln_frames);
-
-	writel(0, &macstat->rx_bytes);
-	writel(0, &macstat->rx_packets);
-	writel(0, &macstat->rx_fcs_errs);
-	writel(0, &macstat->rx_multicast_packets);
-	writel(0, &macstat->rx_broadcast_packets);
-	writel(0, &macstat->rx_control_frames);
-	writel(0, &macstat->rx_pause_frames);
-	writel(0, &macstat->rx_unknown_opcodes);
-	writel(0, &macstat->rx_align_errs);
-	writel(0, &macstat->rx_frame_len_errs);
-	writel(0, &macstat->rx_code_errs);
-	writel(0, &macstat->rx_carrier_sense_errs);
-	writel(0, &macstat->rx_undersize_packets);
-	writel(0, &macstat->rx_oversize_packets);
-	writel(0, &macstat->rx_fragment_packets);
-	writel(0, &macstat->rx_jabbers);
-	writel(0, &macstat->rx_drops);
-
-	writel(0, &macstat->tx_bytes);
-	writel(0, &macstat->tx_packets);
-	writel(0, &macstat->tx_multicast_packets);
-	writel(0, &macstat->tx_broadcast_packets);
-	writel(0, &macstat->tx_pause_frames);
-	writel(0, &macstat->tx_deferred);
-	writel(0, &macstat->tx_excessive_deferred);
-	writel(0, &macstat->tx_single_collisions);
-	writel(0, &macstat->tx_multiple_collisions);
-	writel(0, &macstat->tx_late_collisions);
-	writel(0, &macstat->tx_excessive_collisions);
-	writel(0, &macstat->tx_total_collisions);
-	writel(0, &macstat->tx_pause_honored_frames);
-	writel(0, &macstat->tx_drops);
-	writel(0, &macstat->tx_jabbers);
-	writel(0, &macstat->tx_fcs_errs);
-	writel(0, &macstat->tx_control_frames);
-	writel(0, &macstat->tx_oversize_frames);
-	writel(0, &macstat->tx_undersize_frames);
-	writel(0, &macstat->tx_fragments);
-	writel(0, &macstat->carry_reg1);
-	writel(0, &macstat->carry_reg2);
+	/* initialize all the macstat registers to zero on the device  */
+	for (reg = &macstat->txrx_0_64_byte_frames;
+	     reg <= &macstat->carry_reg2; reg++)
+		writel(0, reg);
 
 	/* Unmask any counters that we want to track the overflow of.
 	 * Initially this will be all counters.  It may become clear later
diff --git a/drivers/staging/et131x/et131x.h b/drivers/staging/et131x/et131x.h
index 1318439..95d6d45 100644
--- a/drivers/staging/et131x/et131x.h
+++ b/drivers/staging/et131x/et131x.h
@@ -1259,148 +1259,54 @@ struct mac_regs {					/* Location: */
 struct macstat_regs {			/* Location: */
 	u32 pad[32];			/*  0x6000 - 607C */
 
-	/* Tx/Rx 0-64 Byte Frame Counter */
+	/* counters */
 	u32 txrx_0_64_byte_frames;	/*  0x6080 */
-
-	/* Tx/Rx 65-127 Byte Frame Counter */
 	u32 txrx_65_127_byte_frames;	/*  0x6084 */
-
-	/* Tx/Rx 128-255 Byte Frame Counter */
 	u32 txrx_128_255_byte_frames;	/*  0x6088 */
-
-	/* Tx/Rx 256-511 Byte Frame Counter */
 	u32 txrx_256_511_byte_frames;	/*  0x608C */
-
-	/* Tx/Rx 512-1023 Byte Frame Counter */
 	u32 txrx_512_1023_byte_frames;	/*  0x6090 */
-
-	/* Tx/Rx 1024-1518 Byte Frame Counter */
 	u32 txrx_1024_1518_byte_frames;	/*  0x6094 */
-
-	/* Tx/Rx 1519-1522 Byte Good VLAN Frame Count */
 	u32 txrx_1519_1522_gvln_frames;	/*  0x6098 */
-
-	/* Rx Byte Counter */
 	u32 rx_bytes;			/*  0x609C */
-
-	/* Rx Packet Counter */
 	u32 rx_packets;			/*  0x60A0 */
-
-	/* Rx FCS Error Counter */
 	u32 rx_fcs_errs;		/*  0x60A4 */
-
-	/* Rx Multicast Packet Counter */
 	u32 rx_multicast_packets;	/*  0x60A8 */
-
-	/* Rx Broadcast Packet Counter */
 	u32 rx_broadcast_packets;	/*  0x60AC */
-
-	/* Rx Control Frame Packet Counter */
 	u32 rx_control_frames;		/*  0x60B0 */
-
-	/* Rx Pause Frame Packet Counter */
 	u32 rx_pause_frames;		/*  0x60B4 */
-
-	/* Rx Unknown OP Code Counter */
 	u32 rx_unknown_opcodes;		/*  0x60B8 */
-
-	/* Rx Alignment Error Counter */
 	u32 rx_align_errs;		/*  0x60BC */
-
-	/* Rx Frame Length Error Counter */
 	u32 rx_frame_len_errs;		/*  0x60C0 */
-
-	/* Rx Code Error Counter */
 	u32 rx_code_errs;		/*  0x60C4 */
-
-	/* Rx Carrier Sense Error Counter */
 	u32 rx_carrier_sense_errs;	/*  0x60C8 */
-
-	/* Rx Undersize Packet Counter */
 	u32 rx_undersize_packets;	/*  0x60CC */
-
-	/* Rx Oversize Packet Counter */
 	u32 rx_oversize_packets;	/*  0x60D0 */
-
-	/* Rx Fragment Counter */
 	u32 rx_fragment_packets;	/*  0x60D4 */
-
-	/* Rx Jabber Counter */
 	u32 rx_jabbers;			/*  0x60D8 */
-
-	/* Rx Drop */
 	u32 rx_drops;			/*  0x60DC */
-
-	/* Tx Byte Counter */
 	u32 tx_bytes;			/*  0x60E0 */
-
-	/* Tx Packet Counter */
 	u32 tx_packets;			/*  0x60E4 */
-
-	/* Tx Multicast Packet Counter */
 	u32 tx_multicast_packets;	/*  0x60E8 */
-
-	/* Tx Broadcast Packet Counter */
 	u32 tx_broadcast_packets;	/*  0x60EC */
-
-	/* Tx Pause Control Frame Counter */
 	u32 tx_pause_frames;		/*  0x60F0 */
-
-	/* Tx Deferral Packet Counter */
 	u32 tx_deferred;		/*  0x60F4 */
-
-	/* Tx Excessive Deferral Packet Counter */
 	u32 tx_excessive_deferred;	/*  0x60F8 */
-
-	/* Tx Single Collision Packet Counter */
 	u32 tx_single_collisions;	/*  0x60FC */
-
-	/* Tx Multiple Collision Packet Counter */
 	u32 tx_multiple_collisions;	/*  0x6100 */
-
-	/* Tx Late Collision Packet Counter */
 	u32 tx_late_collisions;		/*  0x6104 */
-
-	/* Tx Excessive Collision Packet Counter */
 	u32 tx_excessive_collisions;	/*  0x6108 */
-
-	/* Tx Total Collision Packet Counter */
 	u32 tx_total_collisions;	/*  0x610C */
-
-	/* Tx Pause Frame Honored Counter */
 	u32 tx_pause_honored_frames;	/*  0x6110 */
-
-	/* Tx Drop Frame Counter */
 	u32 tx_drops;			/*  0x6114 */
-
-	/* Tx Jabber Frame Counter */
 	u32 tx_jabbers;			/*  0x6118 */
-
-	/* Tx FCS Error Counter */
 	u32 tx_fcs_errs;		/*  0x611C */
-
-	/* Tx Control Frame Counter */
 	u32 tx_control_frames;		/*  0x6120 */
-
-	/* Tx Oversize Frame Counter */
 	u32 tx_oversize_frames;		/*  0x6124 */
-
-	/* Tx Undersize Frame Counter */
 	u32 tx_undersize_frames;	/*  0x6128 */
-
-	/* Tx Fragments Frame Counter */
 	u32 tx_fragments;		/*  0x612C */
-
-	/* Carry Register One Register */
 	u32 carry_reg1;			/*  0x6130 */
-
-	/* Carry Register Two Register */
 	u32 carry_reg2;			/*  0x6134 */
-
-	/* Carry Register One Mask Register */
 	u32 carry_reg1_mask;		/*  0x6138 */
-
-	/* Carry Register Two Mask Register */
 	u32 carry_reg2_mask;		/*  0x613C */
 };
 
-- 
2.1.0

RE: [PATCH 4/8] staging: et131x: Use for loop to initialise contiguous macstat registers to zero

[David Laight]

From: Mark Einon
> Replace a long list of contiguous writel() calls with a for loop iterating
> over the same address values.
> 
> Also remove redundant comments on the macstat registers, the variable names
> are good enough.
...
> -	writel(0, &macstat->txrx_0_64_byte_frames);
...
> -	writel(0, &macstat->carry_reg2);
> +	/* initialize all the macstat registers to zero on the device  */
> +	for (reg = &macstat->txrx_0_64_byte_frames;
> +	     reg <= &macstat->carry_reg2; reg++)
> +		writel(0, reg);
...
>  struct macstat_regs {			/* Location: */
>  	u32 pad[32];			/*  0x6000 - 607C */
> 
> -	/* Tx/Rx 0-64 Byte Frame Counter */
> +	/* counters */
>  	u32 txrx_0_64_byte_frames;	/*  0x6080 */
> -
> -	/* Tx/Rx 65-127 Byte Frame Counter */
>  	u32 txrx_65_127_byte_frames;	/*  0x6084 */

I think it would be best to also convert the stats counters to an array.

	David

Re: [PATCH 4/8] staging: et131x: Use for loop to initialise contiguous macstat registers to zero

[Mark Einon]

On Thu, Aug 21, 2014 at 08:40:20AM +0000, David Laight wrote:
> From: Mark Einon
> > Replace a long list of contiguous writel() calls with a for loop iterating
> > over the same address values.
> > 
> > Also remove redundant comments on the macstat registers, the variable names
> > are good enough.
> ...
> > -	writel(0, &macstat->txrx_0_64_byte_frames);
> ...
> > -	writel(0, &macstat->carry_reg2);
> > +	/* initialize all the macstat registers to zero on the device  */
> > +	for (reg = &macstat->txrx_0_64_byte_frames;
> > +	     reg <= &macstat->carry_reg2; reg++)
> > +		writel(0, reg);
> ...
> >  struct macstat_regs {			/* Location: */
> >  	u32 pad[32];			/*  0x6000 - 607C */
> > 
> > -	/* Tx/Rx 0-64 Byte Frame Counter */
> > +	/* counters */
> >  	u32 txrx_0_64_byte_frames;	/*  0x6080 */
> > -
> > -	/* Tx/Rx 65-127 Byte Frame Counter */
> >  	u32 txrx_65_127_byte_frames;	/*  0x6084 */
> 
> I think it would be best to also convert the stats counters to an array.

Hi David, thanks for the review.

There's other code that accesses these registers individually, taking into
account carries - so I don't think using an array would change much, as
we'd still need a way of identifying individual indices.

Cheers,

Mark

Re: [PATCH 4/8] staging: et131x: Use for loop to initialise contiguous macstat registers to zero

[Mark Einon]

On Thu, Aug 21, 2014 at 08:40:20AM +0000, David Laight wrote:
> From: Mark Einon
> > Replace a long list of contiguous writel() calls with a for loop iterating
> > over the same address values.
> > 
> > Also remove redundant comments on the macstat registers, the variable names
> > are good enough.
> ...
> > -	writel(0, &macstat->txrx_0_64_byte_frames);
> ...
> > -	writel(0, &macstat->carry_reg2);
> > +	/* initialize all the macstat registers to zero on the device  */
> > +	for (reg = &macstat->txrx_0_64_byte_frames;
> > +	     reg <= &macstat->carry_reg2; reg++)
> > +		writel(0, reg);
> ...
> >  struct macstat_regs {			/* Location: */
> >  	u32 pad[32];			/*  0x6000 - 607C */
> > 
> > -	/* Tx/Rx 0-64 Byte Frame Counter */
> > +	/* counters */
> >  	u32 txrx_0_64_byte_frames;	/*  0x6080 */
> > -
> > -	/* Tx/Rx 65-127 Byte Frame Counter */
> >  	u32 txrx_65_127_byte_frames;	/*  0x6084 */
> 
> I think it would be best to also convert the stats counters to an array.

Hi David, thanks for the review.

There's other code that accesses these registers individually, taking into
account carries - so I don't think using an array would change much, as
we'd still need a way of identifying individual indices.

Cheers,

Mark

[PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Mark Einon]

i2c_wack is only used to implement a while(1) loop, so let's remove it.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index fc18e8d..551b250 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -573,7 +573,6 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 	int index = 0;
 	int retries;
 	int err = 0;
-	int i2c_wack = 0;
 	int writeok = 0;
 	u32 status;
 	u32 val = 0;
@@ -599,8 +598,6 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 			LBCIF_CONTROL_LBCIF_ENABLE | LBCIF_CONTROL_I2C_WRITE))
 		return -EIO;
 
-	i2c_wack = 1;
-
 	/* Prepare EEPROM address for Step 3 */
 
 	for (retries = 0; retries < MAX_NUM_WRITE_RETRIES; retries++) {
@@ -656,9 +653,9 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 	 */
 	udelay(10);
 
-	while (i2c_wack) {
+	while (1) {
 		if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
-			LBCIF_CONTROL_LBCIF_ENABLE))
+					  LBCIF_CONTROL_LBCIF_ENABLE))
 			writeok = 0;
 
 		/* Do read until internal ACK_ERROR goes away meaning write
@@ -670,7 +667,8 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 					       addr);
 			do {
 				pci_read_config_dword(pdev,
-					LBCIF_DATA_REGISTER, &val);
+						      LBCIF_DATA_REGISTER,
+						      &val);
 			} while ((val & 0x00010000) == 0);
 		} while (val & 0x00040000);
 
-- 
2.1.0

[PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Mark Einon]

i2c_wack is only used to implement a while(1) loop, so let's remove it.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index fc18e8d..551b250 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -573,7 +573,6 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 	int index = 0;
 	int retries;
 	int err = 0;
-	int i2c_wack = 0;
 	int writeok = 0;
 	u32 status;
 	u32 val = 0;
@@ -599,8 +598,6 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 			LBCIF_CONTROL_LBCIF_ENABLE | LBCIF_CONTROL_I2C_WRITE))
 		return -EIO;
 
-	i2c_wack = 1;
-
 	/* Prepare EEPROM address for Step 3 */
 
 	for (retries = 0; retries < MAX_NUM_WRITE_RETRIES; retries++) {
@@ -656,9 +653,9 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 	 */
 	udelay(10);
 
-	while (i2c_wack) {
+	while (1) {
 		if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
-			LBCIF_CONTROL_LBCIF_ENABLE))
+					  LBCIF_CONTROL_LBCIF_ENABLE))
 			writeok = 0;
 
 		/* Do read until internal ACK_ERROR goes away meaning write
@@ -670,7 +667,8 @@ static int eeprom_write(struct et131x_adapter *adapter, u32 addr, u8 data)
 					       addr);
 			do {
 				pci_read_config_dword(pdev,
-					LBCIF_DATA_REGISTER, &val);
+						      LBCIF_DATA_REGISTER,
+						      &val);
 			} while ((val & 0x00010000) == 0);
 		} while (val & 0x00040000);
 
-- 
2.1.0

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Fabio Estevam]

On Wed, Aug 20, 2014 at 7:17 PM, Mark Einon <mark.einon@gmail.com> wrote:

>                         do {
>                                 pci_read_config_dword(pdev,
> -                                       LBCIF_DATA_REGISTER, &val);
> +                                                     LBCIF_DATA_REGISTER,
> +                                                     &val);

This seems to be an unrelated change.

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Fabio Estevam]

On Wed, Aug 20, 2014 at 7:17 PM, Mark Einon <mark.einon@gmail.com> wrote:

>                         do {
>                                 pci_read_config_dword(pdev,
> -                                       LBCIF_DATA_REGISTER, &val);
> +                                                     LBCIF_DATA_REGISTER,
> +                                                     &val);

This seems to be an unrelated change.

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Mark Einon]

On Wed, Aug 20, 2014 at 07:22:54PM -0300, Fabio Estevam wrote:
> On Wed, Aug 20, 2014 at 7:17 PM, Mark Einon <mark.einon@gmail.com> wrote:
> 
> >                         do {
> >                                 pci_read_config_dword(pdev,
> > -                                       LBCIF_DATA_REGISTER, &val);
> > +                                                     LBCIF_DATA_REGISTER,
> > +                                                     &val);
> 
> This seems to be an unrelated change.

Hi Fabio, thanks for the review.

It's a space alignment of parameters to go with the previous change, to
keep wrapping consistent in the file:

-       while (i2c_wack) {
+       while (1) {
                if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
-                       LBCIF_CONTROL_LBCIF_ENABLE))
+                                         LBCIF_CONTROL_LBCIF_ENABLE))

So what are you saying - are you just commenting, document it, put it
in a seperate patch?

Cheers,

Mark

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Mark Einon]

On Wed, Aug 20, 2014 at 07:22:54PM -0300, Fabio Estevam wrote:
> On Wed, Aug 20, 2014 at 7:17 PM, Mark Einon <mark.einon@gmail.com> wrote:
> 
> >                         do {
> >                                 pci_read_config_dword(pdev,
> > -                                       LBCIF_DATA_REGISTER, &val);
> > +                                                     LBCIF_DATA_REGISTER,
> > +                                                     &val);
> 
> This seems to be an unrelated change.

Hi Fabio, thanks for the review.

It's a space alignment of parameters to go with the previous change, to
keep wrapping consistent in the file:

-       while (i2c_wack) {
+       while (1) {
                if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
-                       LBCIF_CONTROL_LBCIF_ENABLE))
+                                         LBCIF_CONTROL_LBCIF_ENABLE))

So what are you saying - are you just commenting, document it, put it
in a seperate patch?

Cheers,

Mark

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Fabio Estevam]

Hi Mark,

On Thu, Aug 21, 2014 at 6:18 AM, Mark Einon <mark.einon@gmail.com> wrote:

>
> Hi Fabio, thanks for the review.
>
> It's a space alignment of parameters to go with the previous change, to
> keep wrapping consistent in the file:
>
> -       while (i2c_wack) {
> +       while (1) {
>                 if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
> -                       LBCIF_CONTROL_LBCIF_ENABLE))
> +                                         LBCIF_CONTROL_LBCIF_ENABLE))
>
> So what are you saying - are you just commenting, document it, put it
> in a seperate patch?

Yes, it would be better if this part was part of a separate patch.

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Fabio Estevam]

Hi Mark,

On Thu, Aug 21, 2014 at 6:18 AM, Mark Einon <mark.einon@gmail.com> wrote:

>
> Hi Fabio, thanks for the review.
>
> It's a space alignment of parameters to go with the previous change, to
> keep wrapping consistent in the file:
>
> -       while (i2c_wack) {
> +       while (1) {
>                 if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
> -                       LBCIF_CONTROL_LBCIF_ENABLE))
> +                                         LBCIF_CONTROL_LBCIF_ENABLE))
>
> So what are you saying - are you just commenting, document it, put it
> in a seperate patch?

Yes, it would be better if this part was part of a separate patch.

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Mark Einon]

On Thu, Aug 21, 2014 at 09:06:40AM -0300, Fabio Estevam wrote:
> Hi Mark,
> 
> On Thu, Aug 21, 2014 at 6:18 AM, Mark Einon <mark.einon@gmail.com> wrote:
> 
> >
> > Hi Fabio, thanks for the review.
> >
> > It's a space alignment of parameters to go with the previous change, to
> > keep wrapping consistent in the file:
> >
> > -       while (i2c_wack) {
> > +       while (1) {
> >                 if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
> > -                       LBCIF_CONTROL_LBCIF_ENABLE))
> > +                                         LBCIF_CONTROL_LBCIF_ENABLE))
> >
> > So what are you saying - are you just commenting, document it, put it
> > in a seperate patch?
> 
> Yes, it would be better if this part was part of a separate patch.

Ok, I'll remove the indenting changes and sned them in a future patch.
I'll submit a revised v2 of this one.

Cheers,

Mark

Re: [PATCH 5/8] staging: et131x: Remove unnecessary i2c_wack variable

[Mark Einon]

On Thu, Aug 21, 2014 at 09:06:40AM -0300, Fabio Estevam wrote:
> Hi Mark,
> 
> On Thu, Aug 21, 2014 at 6:18 AM, Mark Einon <mark.einon@gmail.com> wrote:
> 
> >
> > Hi Fabio, thanks for the review.
> >
> > It's a space alignment of parameters to go with the previous change, to
> > keep wrapping consistent in the file:
> >
> > -       while (i2c_wack) {
> > +       while (1) {
> >                 if (pci_write_config_byte(pdev, LBCIF_CONTROL_REGISTER,
> > -                       LBCIF_CONTROL_LBCIF_ENABLE))
> > +                                         LBCIF_CONTROL_LBCIF_ENABLE))
> >
> > So what are you saying - are you just commenting, document it, put it
> > in a seperate patch?
> 
> Yes, it would be better if this part was part of a separate patch.

Ok, I'll remove the indenting changes and sned them in a future patch.
I'll submit a revised v2 of this one.

Cheers,

Mark

[PATCH 6/8] staging: et131x: Rename NUM_PACKETS_HANDLED to MAX_PACKETS_HANDLED

[Mark Einon]

To better describe it's use as a hard limit.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 551b250..df83ea3 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -179,7 +179,7 @@ MODULE_DESCRIPTION("10/100/1000 Base-T Ethernet Driver for the ET1310 by Agere S
 #define NIC_DEFAULT_NUM_RFD	1024
 #define NUM_FBRS		2
 
-#define NUM_PACKETS_HANDLED	256
+#define MAX_PACKETS_HANDLED	256
 
 #define ALCATEL_MULTICAST_PKT	0x01000000
 #define ALCATEL_BROADCAST_PKT	0x02000000
@@ -2557,7 +2557,7 @@ static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
 	struct rx_ring *rx_ring = &adapter->rx_ring;
 
 	/* Process up to available RFD's */
-	while (count < NUM_PACKETS_HANDLED) {
+	while (count < MAX_PACKETS_HANDLED) {
 		if (list_empty(&rx_ring->recv_list)) {
 			WARN_ON(rx_ring->num_ready_recv != 0);
 			done = false;
@@ -2589,7 +2589,7 @@ static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
 		count++;
 	}
 
-	if (count == NUM_PACKETS_HANDLED || !done) {
+	if (count == MAX_PACKETS_HANDLED || !done) {
 		rx_ring->unfinished_receives = true;
 		writel(PARM_TX_TIME_INT_DEF * NANO_IN_A_MICRO,
 		       &adapter->regs->global.watchdog_timer);
-- 
2.1.0

[PATCH 6/8] staging: et131x: Rename NUM_PACKETS_HANDLED to MAX_PACKETS_HANDLED

[Mark Einon]

To better describe it's use as a hard limit.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index 551b250..df83ea3 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -179,7 +179,7 @@ MODULE_DESCRIPTION("10/100/1000 Base-T Ethernet Driver for the ET1310 by Agere S
 #define NIC_DEFAULT_NUM_RFD	1024
 #define NUM_FBRS		2
 
-#define NUM_PACKETS_HANDLED	256
+#define MAX_PACKETS_HANDLED	256
 
 #define ALCATEL_MULTICAST_PKT	0x01000000
 #define ALCATEL_BROADCAST_PKT	0x02000000
@@ -2557,7 +2557,7 @@ static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
 	struct rx_ring *rx_ring = &adapter->rx_ring;
 
 	/* Process up to available RFD's */
-	while (count < NUM_PACKETS_HANDLED) {
+	while (count < MAX_PACKETS_HANDLED) {
 		if (list_empty(&rx_ring->recv_list)) {
 			WARN_ON(rx_ring->num_ready_recv != 0);
 			done = false;
@@ -2589,7 +2589,7 @@ static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
 		count++;
 	}
 
-	if (count == NUM_PACKETS_HANDLED || !done) {
+	if (count == MAX_PACKETS_HANDLED || !done) {
 		rx_ring->unfinished_receives = true;
 		writel(PARM_TX_TIME_INT_DEF * NANO_IN_A_MICRO,
 		       &adapter->regs->global.watchdog_timer);
-- 
2.1.0

[PATCH 7/8] staging: et131x: Fix ET_INTR_TXDMA_ISR register name typo

[Mark Einon]

We actually mean to clear the ET_INTR_TXDMA_ISR reg after handling
a completed transfer, not the ET_INTR_TXDMA_ERR reg, which should
get handled immediately after.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index df83ea3..bf9ac15 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -3891,7 +3891,7 @@ static void et131x_isr_handler(struct work_struct *work)
 	if (status & ET_INTR_RXDMA_XFR_DONE)
 		et131x_handle_recv_interrupt(adapter);
 
-	status &= ~(ET_INTR_TXDMA_ERR | ET_INTR_RXDMA_XFR_DONE);
+	status &= ~(ET_INTR_TXDMA_ISR | ET_INTR_RXDMA_XFR_DONE);
 
 	if (!status)
 		goto out;
-- 
2.1.0

[PATCH 7/8] staging: et131x: Fix ET_INTR_TXDMA_ISR register name typo

[Mark Einon]

We actually mean to clear the ET_INTR_TXDMA_ISR reg after handling
a completed transfer, not the ET_INTR_TXDMA_ERR reg, which should
get handled immediately after.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/et131x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index df83ea3..bf9ac15 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -3891,7 +3891,7 @@ static void et131x_isr_handler(struct work_struct *work)
 	if (status & ET_INTR_RXDMA_XFR_DONE)
 		et131x_handle_recv_interrupt(adapter);
 
-	status &= ~(ET_INTR_TXDMA_ERR | ET_INTR_RXDMA_XFR_DONE);
+	status &= ~(ET_INTR_TXDMA_ISR | ET_INTR_RXDMA_XFR_DONE);
 
 	if (!status)
 		goto out;
-- 
2.1.0

[PATCH 8/8] staging: et131x: Implement NAPI support

[Mark Einon]

This implements NAPI support for et131x by:

-adding a napi_struct to the private adapter struct
-changing netfif_rx_skb() call to netif_receive_skb()
-changing et131x_handle_recv_interrupt() to et131x_handle_recv_pkts()
 and taking a budget allocation.
-changing et131x_handle_send_interrupt() to et131x_handle_send_pkts()
-replacing bottom half workqueue with poll function which handles
 send & receive of skbs.
-adding various other necessary standard napi calls.

Also remove this item from the README TODO list.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/README   |   1 -
 drivers/staging/et131x/et131x.c | 116 ++++++++++++++++++----------------------
 2 files changed, 52 insertions(+), 65 deletions(-)

diff --git a/drivers/staging/et131x/README b/drivers/staging/et131x/README
index 3befc45..05555a3 100644
--- a/drivers/staging/et131x/README
+++ b/drivers/staging/et131x/README
@@ -10,7 +10,6 @@ driver as they did not build properly at the time.
 TODO:
 	- Look at reducing the number of spinlocks
 	- Simplify code in nic_rx_pkts(), when determining multicast_pkts_rcvd
-	- Implement NAPI support
 	- In et131x_tx(), don't return NETDEV_TX_BUSY, just drop the packet with kfree_skb().
 	- Reduce the number of split lines by careful consideration of variable names etc.
 
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index bf9ac15..485143a 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -470,7 +470,7 @@ struct et131x_adapter {
 	struct pci_dev *pdev;
 	struct mii_bus *mii_bus;
 	struct phy_device *phydev;
-	struct work_struct task;
+	struct napi_struct napi;
 
 	/* Flags that indicate current state of the adapter */
 	u32 flags;
@@ -2538,29 +2538,33 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
 
 	skb->protocol = eth_type_trans(skb, adapter->netdev);
 	skb->ip_summed = CHECKSUM_NONE;
-	netif_rx_ni(skb);
+	netif_receive_skb(skb);
 
 out:
 	nic_return_rfd(adapter, rfd);
 	return rfd;
 }
 
-/* et131x_handle_recv_interrupt - Interrupt handler for receive processing
+/* et131x_handle_recv_pkts - Interrupt handler for receive processing
  *
  * Assumption, Rcv spinlock has been acquired.
  */
-static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
+static int et131x_handle_recv_pkts(struct et131x_adapter *adapter, int budget)
 {
 	struct rfd *rfd = NULL;
-	u32 count = 0;
-	bool done = true;
+	int count = 0;
+	int limit = budget;
+	bool not_done = false;
 	struct rx_ring *rx_ring = &adapter->rx_ring;
 
+	if (budget > MAX_PACKETS_HANDLED)
+		limit = MAX_PACKETS_HANDLED;
+
 	/* Process up to available RFD's */
-	while (count < MAX_PACKETS_HANDLED) {
+	while (count < limit) {
 		if (list_empty(&rx_ring->recv_list)) {
 			WARN_ON(rx_ring->num_ready_recv != 0);
-			done = false;
+			not_done = true;
 			break;
 		}
 
@@ -2589,13 +2593,15 @@ static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
 		count++;
 	}
 
-	if (count == MAX_PACKETS_HANDLED || !done) {
+	if (count == limit || not_done) {
 		rx_ring->unfinished_receives = true;
 		writel(PARM_TX_TIME_INT_DEF * NANO_IN_A_MICRO,
 		       &adapter->regs->global.watchdog_timer);
 	} else
 		/* Watchdog timer will disable itself if appropriate. */
 		rx_ring->unfinished_receives = false;
+
+	return count;
 }
 
 /* et131x_tx_dma_memory_alloc
@@ -3081,14 +3087,14 @@ static void et131x_free_busy_send_packets(struct et131x_adapter *adapter)
 	tx_ring->used = 0;
 }
 
-/* et131x_handle_send_interrupt - Interrupt handler for sending processing
+/* et131x_handle_send_pkts - Interrupt handler for sending processing
  *
  * Re-claim the send resources, complete sends and get more to send from
  * the send wait queue.
  *
  * Assumption - Send spinlock has been acquired
  */
-static void et131x_handle_send_interrupt(struct et131x_adapter *adapter)
+static void et131x_handle_send_pkts(struct et131x_adapter *adapter)
 {
 	unsigned long flags;
 	u32 serviced;
@@ -3708,9 +3714,9 @@ static void et131x_pci_remove(struct pci_dev *pdev)
 	struct et131x_adapter *adapter = netdev_priv(netdev);
 
 	unregister_netdev(netdev);
+	netif_napi_del(&adapter->napi);
 	phy_disconnect(adapter->phydev);
 	mdiobus_unregister(adapter->mii_bus);
-	cancel_work_sync(&adapter->task);
 	kfree(adapter->mii_bus->irq);
 	mdiobus_free(adapter->mii_bus);
 
@@ -3790,6 +3796,7 @@ static irqreturn_t et131x_isr(int irq, void *dev_id)
 	bool handled = true;
 	struct net_device *netdev = (struct net_device *)dev_id;
 	struct et131x_adapter *adapter = netdev_priv(netdev);
+	struct address_map __iomem *iomem = adapter->regs;
 	struct rx_ring *rx_ring = &adapter->rx_ring;
 	struct tx_ring *tx_ring = &adapter->tx_ring;
 	u32 status;
@@ -3826,7 +3833,6 @@ static irqreturn_t et131x_isr(int irq, void *dev_id)
 	}
 
 	/* This is our interrupt, so process accordingly */
-
 	if (status & ET_INTR_WATCHDOG) {
 		struct tcb *tcb = tx_ring->send_head;
 
@@ -3842,54 +3848,8 @@ static irqreturn_t et131x_isr(int irq, void *dev_id)
 		status &= ~ET_INTR_WATCHDOG;
 	}
 
-	if (!status) {
-		/* This interrupt has in some way been "handled" by
-		 * the ISR. Either it was a spurious Rx interrupt, or
-		 * it was a Tx interrupt that has been filtered by
-		 * the ISR.
-		 */
-		et131x_enable_interrupts(adapter);
-		goto out;
-	}
-
-	/* We need to save the interrupt status value for use in our
-	 * DPC. We will clear the software copy of that in that
-	 * routine.
-	 */
-	adapter->stats.interrupt_status = status;
-
-	/* Schedule the ISR handler as a bottom-half task in the
-	 * kernel's tq_immediate queue, and mark the queue for
-	 * execution
-	 */
-	schedule_work(&adapter->task);
-out:
-	return IRQ_RETVAL(handled);
-}
-
-/* et131x_isr_handler - The ISR handler
- *
- * scheduled to run in a deferred context by the ISR. This is where the ISR's
- * work actually gets done.
- */
-static void et131x_isr_handler(struct work_struct *work)
-{
-	struct et131x_adapter *adapter =
-		container_of(work, struct et131x_adapter, task);
-	u32 status = adapter->stats.interrupt_status;
-	struct address_map __iomem *iomem = adapter->regs;
-
-	/* These first two are by far the most common.  Once handled, we clear
-	 * their two bits in the status word.  If the word is now zero, we
-	 * exit.
-	 */
-	/* Handle all the completed Transmit interrupts */
-	if (status & ET_INTR_TXDMA_ISR)
-		et131x_handle_send_interrupt(adapter);
-
-	/* Handle all the completed Receives interrupts */
-	if (status & ET_INTR_RXDMA_XFR_DONE)
-		et131x_handle_recv_interrupt(adapter);
+	if (status & (ET_INTR_RXDMA_XFR_DONE | ET_INTR_TXDMA_ISR))
+		napi_schedule(&adapter->napi);
 
 	status &= ~(ET_INTR_TXDMA_ISR | ET_INTR_RXDMA_XFR_DONE);
 
@@ -4041,8 +4001,34 @@ static void et131x_isr_handler(struct work_struct *work)
 		 * addressed module is in a power-down state and can't respond.
 		 */
 	}
+
+	if (!status) {
+		/* This interrupt has in some way been "handled" by
+		 * the ISR. Either it was a spurious Rx interrupt, or
+		 * it was a Tx interrupt that has been filtered by
+		 * the ISR.
+		 */
+		et131x_enable_interrupts(adapter);
+	}
+
 out:
-	et131x_enable_interrupts(adapter);
+	return IRQ_RETVAL(handled);
+}
+
+static int et131x_poll(struct napi_struct *napi, int budget)
+{
+	struct et131x_adapter *adapter =
+		container_of(napi, struct et131x_adapter, napi);
+	int work_done = et131x_handle_recv_pkts(adapter, budget);
+
+	et131x_handle_send_pkts(adapter);
+
+	if (work_done < budget) {
+		napi_complete(&adapter->napi);
+		et131x_enable_interrupts(adapter);
+	}
+
+	return work_done;
 }
 
 /* et131x_stats - Return the current device statistics  */
@@ -4111,6 +4097,8 @@ static int et131x_open(struct net_device *netdev)
 
 	adapter->flags |= FMP_ADAPTER_INTERRUPT_IN_USE;
 
+	napi_enable(&adapter->napi);
+
 	et131x_up(netdev);
 
 	return result;
@@ -4122,6 +4110,7 @@ static int et131x_close(struct net_device *netdev)
 	struct et131x_adapter *adapter = netdev_priv(netdev);
 
 	et131x_down(netdev);
+	napi_disable(&adapter->napi);
 
 	adapter->flags &= ~FMP_ADAPTER_INTERRUPT_IN_USE;
 	free_irq(adapter->pdev->irq, netdev);
@@ -4502,8 +4491,7 @@ static int et131x_pci_setup(struct pci_dev *pdev,
 	/* Init send data structures */
 	et131x_init_send(adapter);
 
-	/* Set up the task structure for the ISR's deferred handler */
-	INIT_WORK(&adapter->task, et131x_isr_handler);
+	netif_napi_add(netdev, &adapter->napi, et131x_poll, 64);
 
 	/* Copy address into the net_device struct */
 	memcpy(netdev->dev_addr, adapter->addr, ETH_ALEN);
-- 
2.1.0

[PATCH 8/8] staging: et131x: Implement NAPI support

[Mark Einon]

This implements NAPI support for et131x by:

-adding a napi_struct to the private adapter struct
-changing netfif_rx_skb() call to netif_receive_skb()
-changing et131x_handle_recv_interrupt() to et131x_handle_recv_pkts()
 and taking a budget allocation.
-changing et131x_handle_send_interrupt() to et131x_handle_send_pkts()
-replacing bottom half workqueue with poll function which handles
 send & receive of skbs.
-adding various other necessary standard napi calls.

Also remove this item from the README TODO list.

Signed-off-by: Mark Einon <mark.einon@gmail.com>
---
 drivers/staging/et131x/README   |   1 -
 drivers/staging/et131x/et131x.c | 116 ++++++++++++++++++----------------------
 2 files changed, 52 insertions(+), 65 deletions(-)

diff --git a/drivers/staging/et131x/README b/drivers/staging/et131x/README
index 3befc45..05555a3 100644
--- a/drivers/staging/et131x/README
+++ b/drivers/staging/et131x/README
@@ -10,7 +10,6 @@ driver as they did not build properly at the time.
 TODO:
 	- Look at reducing the number of spinlocks
 	- Simplify code in nic_rx_pkts(), when determining multicast_pkts_rcvd
-	- Implement NAPI support
 	- In et131x_tx(), don't return NETDEV_TX_BUSY, just drop the packet with kfree_skb().
 	- Reduce the number of split lines by careful consideration of variable names etc.
 
diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
index bf9ac15..485143a 100644
--- a/drivers/staging/et131x/et131x.c
+++ b/drivers/staging/et131x/et131x.c
@@ -470,7 +470,7 @@ struct et131x_adapter {
 	struct pci_dev *pdev;
 	struct mii_bus *mii_bus;
 	struct phy_device *phydev;
-	struct work_struct task;
+	struct napi_struct napi;
 
 	/* Flags that indicate current state of the adapter */
 	u32 flags;
@@ -2538,29 +2538,33 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
 
 	skb->protocol = eth_type_trans(skb, adapter->netdev);
 	skb->ip_summed = CHECKSUM_NONE;
-	netif_rx_ni(skb);
+	netif_receive_skb(skb);
 
 out:
 	nic_return_rfd(adapter, rfd);
 	return rfd;
 }
 
-/* et131x_handle_recv_interrupt - Interrupt handler for receive processing
+/* et131x_handle_recv_pkts - Interrupt handler for receive processing
  *
  * Assumption, Rcv spinlock has been acquired.
  */
-static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
+static int et131x_handle_recv_pkts(struct et131x_adapter *adapter, int budget)
 {
 	struct rfd *rfd = NULL;
-	u32 count = 0;
-	bool done = true;
+	int count = 0;
+	int limit = budget;
+	bool not_done = false;
 	struct rx_ring *rx_ring = &adapter->rx_ring;
 
+	if (budget > MAX_PACKETS_HANDLED)
+		limit = MAX_PACKETS_HANDLED;
+
 	/* Process up to available RFD's */
-	while (count < MAX_PACKETS_HANDLED) {
+	while (count < limit) {
 		if (list_empty(&rx_ring->recv_list)) {
 			WARN_ON(rx_ring->num_ready_recv != 0);
-			done = false;
+			not_done = true;
 			break;
 		}
 
@@ -2589,13 +2593,15 @@ static void et131x_handle_recv_interrupt(struct et131x_adapter *adapter)
 		count++;
 	}
 
-	if (count == MAX_PACKETS_HANDLED || !done) {
+	if (count == limit || not_done) {
 		rx_ring->unfinished_receives = true;
 		writel(PARM_TX_TIME_INT_DEF * NANO_IN_A_MICRO,
 		       &adapter->regs->global.watchdog_timer);
 	} else
 		/* Watchdog timer will disable itself if appropriate. */
 		rx_ring->unfinished_receives = false;
+
+	return count;
 }
 
 /* et131x_tx_dma_memory_alloc
@@ -3081,14 +3087,14 @@ static void et131x_free_busy_send_packets(struct et131x_adapter *adapter)
 	tx_ring->used = 0;
 }
 
-/* et131x_handle_send_interrupt - Interrupt handler for sending processing
+/* et131x_handle_send_pkts - Interrupt handler for sending processing
  *
  * Re-claim the send resources, complete sends and get more to send from
  * the send wait queue.
  *
  * Assumption - Send spinlock has been acquired
  */
-static void et131x_handle_send_interrupt(struct et131x_adapter *adapter)
+static void et131x_handle_send_pkts(struct et131x_adapter *adapter)
 {
 	unsigned long flags;
 	u32 serviced;
@@ -3708,9 +3714,9 @@ static void et131x_pci_remove(struct pci_dev *pdev)
 	struct et131x_adapter *adapter = netdev_priv(netdev);
 
 	unregister_netdev(netdev);
+	netif_napi_del(&adapter->napi);
 	phy_disconnect(adapter->phydev);
 	mdiobus_unregister(adapter->mii_bus);
-	cancel_work_sync(&adapter->task);
 	kfree(adapter->mii_bus->irq);
 	mdiobus_free(adapter->mii_bus);
 
@@ -3790,6 +3796,7 @@ static irqreturn_t et131x_isr(int irq, void *dev_id)
 	bool handled = true;
 	struct net_device *netdev = (struct net_device *)dev_id;
 	struct et131x_adapter *adapter = netdev_priv(netdev);
+	struct address_map __iomem *iomem = adapter->regs;
 	struct rx_ring *rx_ring = &adapter->rx_ring;
 	struct tx_ring *tx_ring = &adapter->tx_ring;
 	u32 status;
@@ -3826,7 +3833,6 @@ static irqreturn_t et131x_isr(int irq, void *dev_id)
 	}
 
 	/* This is our interrupt, so process accordingly */
-
 	if (status & ET_INTR_WATCHDOG) {
 		struct tcb *tcb = tx_ring->send_head;
 
@@ -3842,54 +3848,8 @@ static irqreturn_t et131x_isr(int irq, void *dev_id)
 		status &= ~ET_INTR_WATCHDOG;
 	}
 
-	if (!status) {
-		/* This interrupt has in some way been "handled" by
-		 * the ISR. Either it was a spurious Rx interrupt, or
-		 * it was a Tx interrupt that has been filtered by
-		 * the ISR.
-		 */
-		et131x_enable_interrupts(adapter);
-		goto out;
-	}
-
-	/* We need to save the interrupt status value for use in our
-	 * DPC. We will clear the software copy of that in that
-	 * routine.
-	 */
-	adapter->stats.interrupt_status = status;
-
-	/* Schedule the ISR handler as a bottom-half task in the
-	 * kernel's tq_immediate queue, and mark the queue for
-	 * execution
-	 */
-	schedule_work(&adapter->task);
-out:
-	return IRQ_RETVAL(handled);
-}
-
-/* et131x_isr_handler - The ISR handler
- *
- * scheduled to run in a deferred context by the ISR. This is where the ISR's
- * work actually gets done.
- */
-static void et131x_isr_handler(struct work_struct *work)
-{
-	struct et131x_adapter *adapter =
-		container_of(work, struct et131x_adapter, task);
-	u32 status = adapter->stats.interrupt_status;
-	struct address_map __iomem *iomem = adapter->regs;
-
-	/* These first two are by far the most common.  Once handled, we clear
-	 * their two bits in the status word.  If the word is now zero, we
-	 * exit.
-	 */
-	/* Handle all the completed Transmit interrupts */
-	if (status & ET_INTR_TXDMA_ISR)
-		et131x_handle_send_interrupt(adapter);
-
-	/* Handle all the completed Receives interrupts */
-	if (status & ET_INTR_RXDMA_XFR_DONE)
-		et131x_handle_recv_interrupt(adapter);
+	if (status & (ET_INTR_RXDMA_XFR_DONE | ET_INTR_TXDMA_ISR))
+		napi_schedule(&adapter->napi);
 
 	status &= ~(ET_INTR_TXDMA_ISR | ET_INTR_RXDMA_XFR_DONE);
 
@@ -4041,8 +4001,34 @@ static void et131x_isr_handler(struct work_struct *work)
 		 * addressed module is in a power-down state and can't respond.
 		 */
 	}
+
+	if (!status) {
+		/* This interrupt has in some way been "handled" by
+		 * the ISR. Either it was a spurious Rx interrupt, or
+		 * it was a Tx interrupt that has been filtered by
+		 * the ISR.
+		 */
+		et131x_enable_interrupts(adapter);
+	}
+
 out:
-	et131x_enable_interrupts(adapter);
+	return IRQ_RETVAL(handled);
+}
+
+static int et131x_poll(struct napi_struct *napi, int budget)
+{
+	struct et131x_adapter *adapter =
+		container_of(napi, struct et131x_adapter, napi);
+	int work_done = et131x_handle_recv_pkts(adapter, budget);
+
+	et131x_handle_send_pkts(adapter);
+
+	if (work_done < budget) {
+		napi_complete(&adapter->napi);
+		et131x_enable_interrupts(adapter);
+	}
+
+	return work_done;
 }
 
 /* et131x_stats - Return the current device statistics  */
@@ -4111,6 +4097,8 @@ static int et131x_open(struct net_device *netdev)
 
 	adapter->flags |= FMP_ADAPTER_INTERRUPT_IN_USE;
 
+	napi_enable(&adapter->napi);
+
 	et131x_up(netdev);
 
 	return result;
@@ -4122,6 +4110,7 @@ static int et131x_close(struct net_device *netdev)
 	struct et131x_adapter *adapter = netdev_priv(netdev);
 
 	et131x_down(netdev);
+	napi_disable(&adapter->napi);
 
 	adapter->flags &= ~FMP_ADAPTER_INTERRUPT_IN_USE;
 	free_irq(adapter->pdev->irq, netdev);
@@ -4502,8 +4491,7 @@ static int et131x_pci_setup(struct pci_dev *pdev,
 	/* Init send data structures */
 	et131x_init_send(adapter);
 
-	/* Set up the task structure for the ISR's deferred handler */
-	INIT_WORK(&adapter->task, et131x_isr_handler);
+	netif_napi_add(netdev, &adapter->napi, et131x_poll, 64);
 
 	/* Copy address into the net_device struct */
 	memcpy(netdev->dev_addr, adapter->addr, ETH_ALEN);
-- 
2.1.0

Re: [PATCH 8/8] staging: et131x: Implement NAPI support

[Stephen Hemminger]

On Wed, 20 Aug 2014 23:17:58 +0100
Mark Einon <mark.einon@gmail.com> wrote:

>  
> +	if (budget > MAX_PACKETS_HANDLED)
> +		limit = MAX_PACKETS_HANDLED;

Why this artificial restriction?

Re: [PATCH 8/8] staging: et131x: Implement NAPI support

[Mark Einon]

On Wed, Aug 20, 2014 at 08:25:01PM -0700, Stephen Hemminger wrote:
> On Wed, 20 Aug 2014 23:17:58 +0100
> Mark Einon <mark.einon@gmail.com> wrote:
> 
> >  
> > +	if (budget > MAX_PACKETS_HANDLED)
> > +		limit = MAX_PACKETS_HANDLED;
> 
> Why this artificial restriction?

Hi Stephen, thanks for the review.

It's a restriction that was in the original driver code, and I'm being
cautious. I don't have much documentation for the device, and I haven't
yet figured a way to test the limit so I can play with removing it. If
you have any suggestions on how to do that, I'd be happy to hear them.

Cheers,

Mark

Re: [PATCH 8/8] staging: et131x: Implement NAPI support

[Mark Einon]

On Wed, Aug 20, 2014 at 08:25:01PM -0700, Stephen Hemminger wrote:
> On Wed, 20 Aug 2014 23:17:58 +0100
> Mark Einon <mark.einon@gmail.com> wrote:
> 
> >  
> > +	if (budget > MAX_PACKETS_HANDLED)
> > +		limit = MAX_PACKETS_HANDLED;
> 
> Why this artificial restriction?

Hi Stephen, thanks for the review.

It's a restriction that was in the original driver code, and I'm being
cautious. I don't have much documentation for the device, and I haven't
yet figured a way to test the limit so I can play with removing it. If
you have any suggestions on how to do that, I'd be happy to hear them.

Cheers,

Mark

Re: [PATCH 8/8] staging: et131x: Implement NAPI support

[Stephen Hemminger]

On Wed, 20 Aug 2014 23:17:58 +0100
Mark Einon <mark.einon@gmail.com> wrote:

> -	bool done = true;
> +	int count = 0;
> +	int limit = budget;
> +	bool not_done = false;

Don't use negative variables. Better to keep the original done variable.

Re: [PATCH 8/8] staging: et131x: Implement NAPI support

[Mark Einon]

On Wed, Aug 20, 2014 at 08:25:45PM -0700, Stephen Hemminger wrote:
> On Wed, 20 Aug 2014 23:17:58 +0100
> Mark Einon <mark.einon@gmail.com> wrote:
> 
> > -	bool done = true;
> > +	int count = 0;
> > +	int limit = budget;
> > +	bool not_done = false;
> 
> Don't use negative variables. Better to keep the original done variable.

Fair comment, I'll send a v2.

Cheers,

Mark

Re: [PATCH 8/8] staging: et131x: Implement NAPI support

[Mark Einon]

On Wed, Aug 20, 2014 at 08:25:45PM -0700, Stephen Hemminger wrote:
> On Wed, 20 Aug 2014 23:17:58 +0100
> Mark Einon <mark.einon@gmail.com> wrote:
> 
> > -	bool done = true;
> > +	int count = 0;
> > +	int limit = budget;
> > +	bool not_done = false;
> 
> Don't use negative variables. Better to keep the original done variable.

Fair comment, I'll send a v2.

Cheers,

Mark