[PATCH RFC 00/15] via-rhine: fix resume, cleanup, eth ops (regs)

[PATCH RFC 00/15] via-rhine: fix resume, cleanup, eth ops (regs)

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

This patchset fixes suspend/resume of via-rhine in the NetworkManager case
(patch 0001), with subsequent patches being predominantly
about general cleanup/renovation work
(one patch adds get_regs() ethtool support, though).

Currently marked as RFC since it's somewhat larger,
without prior review activity.

checkpatch.pl'd patchset against a slightly oldish master
(current state of via-rhine.c in linux-next is identical).

Note that some parts in this patchset have a dependency on predecessors
(e.g. __read_mostly comes to mind).

I managed to stay at 15 patches, thus right at (below) the
official mailing list patch limit.

Somehow Christmas must have been mighty boring :)

And one of the patches narrows down on non-default
compiler warning levels, too (c.f. my recent LKML tirade).

The first patch (resume fix) possibly is -stable material,
but since layer separation currently is not crystal clear
there might be some risk, thus I'm hesitating.

Thanks!


Andreas Mohr (15):
  via-rhine: YARB: fix broken resume of ifdown case (NetworkManager).
  via-rhine: some suspend/resume cleanup.
  via-rhine: small rhine_wait_bit() improvement.
  via-rhine: handle compile warnings (use PCI_VDEVICE macro).
  via-rhine: Spelling/phrases cleanup.
  via-rhine: The Great Renaming.
  via-rhine: MMIO: move register verify into helper function.
  via-rhine: MMIO: move support decision (compile-time-only to
    runtime).
  via-rhine: mark some variables as __read_mostly.
  via-rhine: WOL: remove duplication into a helper.
  via-rhine: WOL: separate WOL configuration (and its logging).
  via-rhine: implement get_regs() ethtool ops.
  via-rhine: misc. cleanup.
  via-rhine: The Great Deduplication.
  via-rhine: add helper (reduce type-unsafe void * assignments).

 drivers/net/ethernet/via/via-rhine.c |  572 +++++++++++++++++++++++-----------
 1 files changed, 396 insertions(+), 176 deletions(-)

-- 
1.7.2.5

[PATCH RFC 01/15] via-rhine: YARB: fix broken resume of ifdown case (NetworkManager).

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Well, it's probably not a *breakage* this time after all
(handling probably was never fully correct before).

resume() in the case of an ifdown iface
(as e.g. usually done by NetworkManager)
did not restore the card to proper full post-probe() card state
(see resume()s !netif_running() check),
thereby causing the subsequent iface open() as done by NM to fail,
since that one requires full post-probe() card state
(e.g. PHY available) yet the PHY was not available
(at least in the case of full-shutdown S2R).

Some related NetworkManager links are
  https://bugs.mageia.org/show_bug.cgi?id=7498
  http://crunchbang.org/forums/viewtopic.php?id=15169
  https://bugzilla.redhat.com/show_bug.cgi?id=477964
  https://bugzilla.redhat.com/show_bug.cgi?id=549089

Suspend/resume behaviour verified with both NM running and not running,
and with both mem (S2R) and standby.
Tested on Neoware CA10 (VIA C3), will also test on EPoX 8K5A2+.

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 7992b3e..5facb0b 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -2324,13 +2324,20 @@ static int rhine_resume(struct device *device)
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 
-	if (!netif_running(dev))
-		return 0;
-
 #ifdef USE_MMIO
 	enable_mmio(rp->pioaddr, rp->quirks);
 #endif
+	/*
+	 * FIXME: some power calls here are being done
+	 * internally in netdev setup parts below, too -
+	 * should be corrected eventually.
+	 */
 	rhine_power_init(dev);
+	rhine_hw_init(dev, rp->pioaddr);
+
+	if (!netif_running(dev))
+		return 0;
+
 	free_tbufs(dev);
 	free_rbufs(dev);
 	alloc_tbufs(dev);
-- 
1.7.2.5

[PATCH RFC 02/15] via-rhine: some suspend/resume cleanup.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Work towards unifying duplicated power setup sections,
add large comment.

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 5facb0b..928d96f 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -955,8 +955,10 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_out_free_res;
 	}
 
+	rp->base = ioaddr;
+
 #ifdef USE_MMIO
-	enable_mmio(pioaddr, quirks);
+	enable_mmio(rp->pioaddr, rp->quirks);
 
 	/* Check that selected MMIO registers match the PIO ones */
 	i = 0;
@@ -974,11 +976,9 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 #endif /* USE_MMIO */
 
-	rp->base = ioaddr;
-
 	/* Get chip registers into a sane state */
 	rhine_power_init(dev);
-	rhine_hw_init(dev, pioaddr);
+	rhine_hw_init(dev, rp->pioaddr);
 
 	for (i = 0; i < 6; i++)
 		dev->dev_addr[i] = ioread8(ioaddr + StationAddr + i);
@@ -2324,6 +2324,25 @@ static int rhine_resume(struct device *device)
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 
+	/*
+	 * Resume theory of operation ("state machine" description):
+	 * Card state post resume() needs to be sufficiently restored
+	 * to be at least as "good" as post-probe()
+	 * (plus certain user-side card attribute modifications
+	 * ideally being preserved, too!).
+	 * In the case of an ifdown iface prior to suspend,
+	 * this means equal state.
+	 * open()/close() then do a state elevation (working iface)
+	 * and reversal (back down to post-probe() card state).
+	 * NetworkManager likes to close() ifaces prior to suspend(),
+	 * thus they end up "closed" on resume() yet the card
+	 * needs to be sufficiently restored to have its basics working...
+	 * (open() does NOT and should not carry out such things!!).
+	 * FIXME: the card setup parts here are duplicated
+	 * with corresponding parts in probe() - they should be moved
+	 * into a helper to be commonly called by both places.
+	 */
+
 #ifdef USE_MMIO
 	enable_mmio(rp->pioaddr, rp->quirks);
 #endif
-- 
1.7.2.5

[PATCH RFC 03/15] via-rhine: small rhine_wait_bit() improvement.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Precalculate actual I/O address,
make sure to remember value in helper variable,
add a logging helper line.

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 928d96f..6be6566 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -505,11 +505,19 @@ static void rhine_restart_tx(struct net_device *dev);
 
 static void rhine_wait_bit(struct rhine_private *rp, u8 reg, u8 mask, bool low)
 {
-	void __iomem *ioaddr = rp->base;
+	void __iomem *ioaddr_reg = rp->base;
 	int i;
 
+	ioaddr_reg += reg;
 	for (i = 0; i < 1024; i++) {
-		bool has_mask_bits = !!(ioread8(ioaddr + reg) & mask);
+		u8 val = ioread8(ioaddr_reg);
+		bool has_mask_bits = !!(val & mask);
+
+		if (i < 20 /* avoid log spew */) {
+			/* netif_err(rp, hw, rp->dev,
+			"wait_bit %02x val %02x mask %02x %s hmb %d\n",
+			reg, val, mask, low ? "low" : "high", has_mask_bits); */
+		}
 
 		if (low ^ has_mask_bits)
 			break;
-- 
1.7.2.5

[PATCH RFC 04/15] via-rhine: handle compile warnings (use PCI_VDEVICE macro).

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Gets rid of W=2 warnings
drivers/net/ethernet/via/via-rhine.c:274: warning: missing initializer
drivers/net/ethernet/via/via-rhine.c:274: warning: (near initialization for 'rhine_pci_tbl[0].class')

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 6be6566..2d410ec 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -271,10 +271,10 @@ enum rhine_quirks {
 #define IOSYNC	do { ioread8(ioaddr + StationAddr); } while (0)
 
 static DEFINE_PCI_DEVICE_TABLE(rhine_pci_tbl) = {
-	{ 0x1106, 0x3043, PCI_ANY_ID, PCI_ANY_ID, },	/* VT86C100A */
-	{ 0x1106, 0x3065, PCI_ANY_ID, PCI_ANY_ID, },	/* VT6102 */
-	{ 0x1106, 0x3106, PCI_ANY_ID, PCI_ANY_ID, },	/* 6105{,L,LOM} */
-	{ 0x1106, 0x3053, PCI_ANY_ID, PCI_ANY_ID, },	/* VT6105M */
+	{ PCI_VDEVICE(VIA, 0x3043), 0 },	/* VT86C100A */
+	{ PCI_VDEVICE(VIA, 0x3065), 0 },	/* VT6102 */
+	{ PCI_VDEVICE(VIA, 0x3106), 0 },	/* 6105{,L,LOM} */
+	{ PCI_VDEVICE(VIA, 0x3053), 0 },	/* VT6105M */
 	{ }	/* terminate list */
 };
 MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
-- 
1.7.2.5

[PATCH RFC 05/15] via-rhine: Spelling/phrases cleanup.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 2d410ec..4162649 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -83,7 +83,7 @@ static const int multicast_filter_limit = 32;
 /* Time in jiffies before concluding the transmitter is hung. */
 #define TX_TIMEOUT	(2*HZ)
 
-#define PKT_BUF_SZ	1536	/* Size of each temporary Rx buffer.*/
+#define PKT_BUF_SZ	1536	/* Size of each temporary Rx buffer. */
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -264,7 +264,7 @@ enum rhine_quirks {
 /*
  * rqRhineI: VT86C100A (aka Rhine-I) uses different bits to enable
  * MMIO as well as for the collision counter and the Tx FIFO underflow
- * indicator. In addition, Tx and Rx buffers need to 4 byte aligned.
+ * indicator. In addition, Tx and Rx buffers need to be 4 byte aligned.
  */
 
 /* Beware of PCI posted writes */
@@ -1266,7 +1266,7 @@ static void rhine_set_carrier(struct mii_if_info *mii)
 		/* autoneg is off: Link is always assumed to be up */
 		if (!netif_carrier_ok(dev))
 			netif_carrier_on(dev);
-	} else	/* Let MMI library update carrier status */
+	} else	/* Let MII library update carrier status */
 		rhine_check_media(dev, 0);
 
 	netif_info(rp, link, dev, "force_media %d, carrier %d\n",
@@ -1649,7 +1649,7 @@ static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 	void __iomem *ioaddr = rp->base;
 	unsigned entry;
 
-	/* Caution: the write order is important here, set the field
+	/* Caution: the write order is important here - set the field
 	   with the "ownership" bits last. */
 
 	/* Calculate the next Tx descriptor entry. */
@@ -1868,7 +1868,7 @@ static int rhine_rx(struct net_device *dev, int limit)
 					    &rp->rx_ring[entry]);
 				dev->stats.rx_length_errors++;
 			} else if (desc_status & RxErr) {
-				/* There was a error. */
+				/* There was an error. */
 				netif_dbg(rp, rx_err, dev,
 					  "%s() Rx error %08x\n", __func__,
 					  desc_status);
-- 
1.7.2.5

[PATCH RFC 06/15] via-rhine: The Great Renaming.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

- rhine_dmi_table --> rhine_dmi_table_quirk_avoid_d3
- rhine_ethop_*()
  (clarify layering by renaming parts)
- rqWOL --> rqHaveWOL
  (improves clarity - WOL is not "quirky", we simply have it)
- rhine_cleanup() --> rhine_exit()
  These are specific handlers meant to serve the module_init()/module_exit()
  interface, thus there's no reason to have them named differently
  from the interface that they cover
  - until a rename of module_*() happens to come along, that is...
- rhine_shutdown() --> rhine_shutdown_and_keep_wol()
- correct to rhine_update_rx_crc_and_missed_errors()

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   47 ++++++++++++++++++---------------
 1 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 4162649..053375b 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -255,7 +255,7 @@ enum rhine_revs {
 };
 
 enum rhine_quirks {
-	rqWOL		= 0x0001,	/* Wake-On-LAN support */
+	rqHaveWOL	= 0x0001,	/* Wake-On-LAN support */
 	rqForceReset	= 0x0002,
 	rq6patterns	= 0x0040,	/* 6 instead of 4 patterns for WOL */
 	rqStatusWBRace	= 0x0080,	/* Tx Status Writeback Error possible */
@@ -571,7 +571,7 @@ static void rhine_power_init(struct net_device *dev)
 	void __iomem *ioaddr = rp->base;
 	u16 wolstat;
 
-	if (rp->quirks & rqWOL) {
+	if (rp->quirks & rqHaveWOL) {
 		/* Make sure chip is in power state D0 */
 		iowrite8(ioread8(ioaddr + StickyHW) & 0xFC, ioaddr + StickyHW);
 
@@ -689,7 +689,7 @@ static void rhine_reload_eeprom(long pioaddr, struct net_device *dev)
 #endif
 
 	/* Turn off EEPROM-controlled wake-up (magic packet) */
-	if (rp->quirks & rqWOL)
+	if (rp->quirks & rqHaveWOL)
 		iowrite8(ioread8(ioaddr + ConfigA) & 0xFC, ioaddr + ConfigA);
 
 }
@@ -744,7 +744,8 @@ static void rhine_tx_err(struct rhine_private *rp, u32 status)
 	rhine_restart_tx(dev);
 }
 
-static void rhine_update_rx_crc_and_missed_errord(struct rhine_private *rp)
+static void
+rhine_update_rx_crc_and_missed_errors(struct rhine_private *rp)
 {
 	void __iomem *ioaddr = rp->base;
 	struct net_device_stats *stats = &rp->dev->stats;
@@ -814,7 +815,7 @@ static int rhine_napipoll(struct napi_struct *napi, int budget)
 
 	if (status & IntrStatsMax) {
 		spin_lock(&rp->lock);
-		rhine_update_rx_crc_and_missed_errord(rp);
+		rhine_update_rx_crc_and_missed_errors(rp);
 		spin_unlock(&rp->lock);
 	}
 
@@ -895,7 +896,7 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		io_size = 128;
 	}
 	else if (pdev->revision >= VT6102) {
-		quirks = rqWOL | rqForceReset;
+		quirks = rqHaveWOL | rqForceReset;
 		if (pdev->revision < VT6105) {
 			name = "Rhine II";
 			quirks |= rqStatusWBRace;	/* Rhine-II exclusive */
@@ -2033,7 +2034,7 @@ static struct net_device_stats *rhine_get_stats(struct net_device *dev)
 	struct rhine_private *rp = netdev_priv(dev);
 
 	spin_lock_bh(&rp->lock);
-	rhine_update_rx_crc_and_missed_errord(rp);
+	rhine_update_rx_crc_and_missed_errors(rp);
 	spin_unlock_bh(&rp->lock);
 
 	return &dev->stats;
@@ -2149,11 +2150,12 @@ static void netdev_set_msglevel(struct net_device *dev, u32 value)
 	rp->msg_enable = value;
 }
 
-static void rhine_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+static void
+rhine_ethop_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 
-	if (!(rp->quirks & rqWOL))
+	if (!(rp->quirks & rqHaveWOL))
 		return;
 
 	spin_lock_irq(&rp->lock);
@@ -2163,13 +2165,14 @@ static void rhine_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	spin_unlock_irq(&rp->lock);
 }
 
-static int rhine_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+static int
+rhine_ethop_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	u32 support = WAKE_PHY | WAKE_MAGIC |
 		      WAKE_UCAST | WAKE_MCAST | WAKE_BCAST;	/* Untested */
 
-	if (!(rp->quirks & rqWOL))
+	if (!(rp->quirks & rqHaveWOL))
 		return -EINVAL;
 
 	if (wol->wolopts & ~support)
@@ -2190,8 +2193,8 @@ static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_link		= netdev_get_link,
 	.get_msglevel		= netdev_get_msglevel,
 	.set_msglevel		= netdev_set_msglevel,
-	.get_wol		= rhine_get_wol,
-	.set_wol		= rhine_set_wol,
+	.get_wol		= rhine_ethop_get_wol,
+	.set_wol		= rhine_ethop_set_wol,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
@@ -2254,13 +2257,14 @@ static void rhine_remove_one(struct pci_dev *pdev)
 	pci_set_drvdata(pdev, NULL);
 }
 
-static void rhine_shutdown (struct pci_dev *pdev)
+static void
+rhine_shutdown_and_keep_wol(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 	void __iomem *ioaddr = rp->base;
 
-	if (!(rp->quirks & rqWOL))
+	if (!(rp->quirks & rqHaveWOL))
 		return; /* Nothing to do for non-WOL adapters */
 
 	rhine_power_init(dev);
@@ -2321,7 +2325,7 @@ static int rhine_suspend(struct device *device)
 
 	netif_device_detach(dev);
 
-	rhine_shutdown(pdev);
+	rhine_shutdown_and_keep_wol(pdev);
 
 	return 0;
 }
@@ -2393,11 +2397,11 @@ static struct pci_driver rhine_driver = {
 	.id_table	= rhine_pci_tbl,
 	.probe		= rhine_init_one,
 	.remove		= rhine_remove_one,
-	.shutdown	= rhine_shutdown,
+	.shutdown	= rhine_shutdown_and_keep_wol,
 	.driver.pm	= RHINE_PM_OPS,
 };
 
-static struct dmi_system_id __initdata rhine_dmi_table[] = {
+static struct dmi_system_id __initdata rhine_dmi_table_quirk_avoid_d3[] = {
 	{
 		.ident = "EPIA-M",
 		.matches = {
@@ -2421,7 +2425,7 @@ static int __init rhine_init(void)
 #ifdef MODULE
 	pr_info("%s\n", version);
 #endif
-	if (dmi_check_system(rhine_dmi_table)) {
+	if (dmi_check_system(rhine_dmi_table_quirk_avoid_d3)) {
 		/* these BIOSes fail at PXE boot if chip is in D3 */
 		avoid_D3 = true;
 		pr_warn("Broken BIOS detected, avoid_D3 enabled\n");
@@ -2433,11 +2437,12 @@ static int __init rhine_init(void)
 }
 
 
-static void __exit rhine_cleanup(void)
+static void __exit
+rhine_exit(void)
 {
 	pci_unregister_driver(&rhine_driver);
 }
 
 
 module_init(rhine_init);
-module_exit(rhine_cleanup);
+module_exit(rhine_exit);
-- 
1.7.2.5

[PATCH RFC 07/15] via-rhine: MMIO: move register verify into helper function.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Add mmio_verify_registers() function.
Gets rid of a useless USE_MMIO define section, too.
Reverse logging of MMIO vs. PIO values since the description suggests
different order.

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   49 +++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 053375b..a16e227 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -338,14 +338,6 @@ enum bcr1_bits {
 	BCR1_MED1=0x80,		/* for VT6102 */
 };
 
-#ifdef USE_MMIO
-/* Registers we check that mmio and reg are the same. */
-static const int mmio_verify_registers[] = {
-	RxConfig, TxConfig, IntrEnable, ConfigA, ConfigB, ConfigC, ConfigD,
-	0
-};
-#endif
-
 /* Bits in the interrupt status/mask registers. */
 enum intr_status_bits {
 	IntrRxDone	= 0x0001,
@@ -659,6 +651,31 @@ static void enable_mmio(long pioaddr, u32 quirks)
 		outb(n, pioaddr + ConfigD);
 	}
 }
+/* Check that selected MMIO registers match the PIO ones */
+static inline bool
+mmio_verify_registers(struct pci_dev *pdev, long pioaddr, void __iomem *ioaddr)
+{
+	/* Registers we check to verify that mmio and reg are the same. */
+	static const int registers_to_verify[] = {
+		RxConfig, TxConfig, IntrEnable,
+		ConfigA, ConfigB, ConfigC, ConfigD,
+		0
+	};
+
+	int i = 0;
+	while (registers_to_verify[i]) {
+		int reg = registers_to_verify[i++];
+		unsigned char pio = inb(pioaddr+reg);
+		unsigned char mmio = readb(ioaddr+reg);
+		if (pio != mmio) {
+			dev_err(&pdev->dev,
+			"MMIO do not match PIO [%02x] (%02x != %02x)\n",
+			reg, mmio, pio);
+			return false;
+		}
+	}
+	return true;
+}
 #endif
 
 /*
@@ -969,19 +986,9 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 #ifdef USE_MMIO
 	enable_mmio(rp->pioaddr, rp->quirks);
 
-	/* Check that selected MMIO registers match the PIO ones */
-	i = 0;
-	while (mmio_verify_registers[i]) {
-		int reg = mmio_verify_registers[i++];
-		unsigned char a = inb(pioaddr+reg);
-		unsigned char b = readb(ioaddr+reg);
-		if (a != b) {
-			rc = -EIO;
-			dev_err(&pdev->dev,
-				"MMIO do not match PIO [%02x] (%02x != %02x)\n",
-				reg, a, b);
-			goto err_out_unmap;
-		}
+	if (!mmio_verify_registers(pdev, pioaddr, ioaddr)) {
+		rc = -EIO;
+		goto err_out_unmap;
 	}
 #endif /* USE_MMIO */
 
-- 
1.7.2.5

[PATCH RFC 08/15] via-rhine: MMIO: move support decision (compile-time-only to runtime).

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Reduces ifdefs a bit in the process, too.

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   56 ++++++++++++++++++++-------------
 1 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index a16e227..a4b8d84 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -36,6 +36,7 @@
 #define DRV_RELDATE	"2010-10-09"
 
 #include <linux/types.h>
+#include <linux/cache.h> /* __read_mostly */
 
 /* A few user-configurable values.
    These may be modified when a driver module is loaded. */
@@ -119,7 +120,12 @@ static const char version[] =
 /* This driver was written to use PCI memory space. Some early versions
    of the Rhine may only work correctly with I/O space accesses. */
 #ifdef CONFIG_VIA_RHINE_MMIO
-#define USE_MMIO
+#define IMPL_MMIO
+/* Hmm, I don't know whether we actually want to offer this
+ * as a publicly visible module parameter, with the likely ensuing confusion
+ * (does/doesn't this module build variant have it, and what is the default?).
+ */
+static bool use_mmio __read_mostly = 1;
 #else
 #endif
 
@@ -137,6 +143,9 @@ MODULE_PARM_DESC(avoid_D3, "Avoid power state D3 (work-around for broken BIOSes)
 #define MCAM_SIZE	32
 #define VCAM_SIZE	32
 
+#define VIA_PCIBAR_IO 0
+#define VIA_PCIBAR_MMIO 1
+
 /*
 		Theory of Operation
 
@@ -638,7 +647,7 @@ static void rhine_chip_reset(struct net_device *dev)
 		   "failed" : "succeeded");
 }
 
-#ifdef USE_MMIO
+#ifdef IMPL_MMIO
 static void enable_mmio(long pioaddr, u32 quirks)
 {
 	int n;
@@ -696,13 +705,14 @@ static void rhine_reload_eeprom(long pioaddr, struct net_device *dev)
 	if (i > 512)
 		pr_info("%4d cycles used @ %s:%d\n", i, __func__, __LINE__);
 
-#ifdef USE_MMIO
+#ifdef IMPL_MMIO
 	/*
 	 * Reloading from EEPROM overwrites ConfigA-D, so we must re-enable
 	 * MMIO. If reloading EEPROM was done first this could be avoided, but
 	 * it is not known if that still works with the "win98-reboot" problem.
 	 */
-	enable_mmio(pioaddr, rp->quirks);
+	if (use_mmio)
+		enable_mmio(pioaddr, rp->quirks);
 #endif
 
 	/* Turn off EEPROM-controlled wake-up (magic packet) */
@@ -890,14 +900,11 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	u32 quirks;
 	long pioaddr;
 	long memaddr;
+	long log_ioaddr; /* (non-)MMIO switch helper */
 	void __iomem *ioaddr;
 	int io_size, phy_id;
 	const char *name;
-#ifdef USE_MMIO
-	int bar = 1;
-#else
-	int bar = 0;
-#endif
+	int bar = VIA_PCIBAR_IO;
 
 /* when built into the kernel, we only print version if device is found */
 #ifndef MODULE
@@ -972,6 +979,10 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		goto err_out_free_netdev;
 
+#ifdef IMPL_MMIO
+	if (use_mmio)
+		bar = VIA_PCIBAR_MMIO;
+#endif
 	ioaddr = pci_iomap(pdev, bar, io_size);
 	if (!ioaddr) {
 		rc = -EIO;
@@ -983,14 +994,18 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	rp->base = ioaddr;
 
-#ifdef USE_MMIO
-	enable_mmio(rp->pioaddr, rp->quirks);
+	log_ioaddr = (long)ioaddr;
+#ifdef IMPL_MMIO
+	if (use_mmio) {
+		enable_mmio(rp->pioaddr, rp->quirks);
 
-	if (!mmio_verify_registers(pdev, pioaddr, ioaddr)) {
-		rc = -EIO;
-		goto err_out_unmap;
+		if (!mmio_verify_registers(pdev, pioaddr, ioaddr)) {
+			rc = -EIO;
+			goto err_out_unmap;
+		}
+		log_ioaddr = memaddr;
 	}
-#endif /* USE_MMIO */
+#endif /* IMPL_MMIO */
 
 	/* Get chip registers into a sane state */
 	rhine_power_init(dev);
@@ -1044,11 +1059,7 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	netdev_info(dev, "VIA %s at 0x%lx, %pM, IRQ %d\n",
 		    name,
-#ifdef USE_MMIO
-		    memaddr,
-#else
-		    (long)ioaddr,
-#endif
+		    log_ioaddr,
 		    dev->dev_addr, pdev->irq);
 
 	pci_set_drvdata(pdev, dev);
@@ -2362,8 +2373,9 @@ static int rhine_resume(struct device *device)
 	 * into a helper to be commonly called by both places.
 	 */
 
-#ifdef USE_MMIO
-	enable_mmio(rp->pioaddr, rp->quirks);
+#ifdef IMPL_MMIO
+	if (use_mmio)
+		enable_mmio(rp->pioaddr, rp->quirks);
 #endif
 	/*
 	 * FIXME: some power calls here are being done
-- 
1.7.2.5

[PATCH RFC 09/15] via-rhine: mark some variables as __read_mostly.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index a4b8d84..ca788c0 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -49,14 +49,14 @@ static int debug = 0;
 #if defined(__alpha__) || defined(__arm__) || defined(__hppa__) || \
 	defined(CONFIG_SPARC) || defined(__ia64__) ||		   \
 	defined(__sh__) || defined(__mips__)
-static int rx_copybreak = 1518;
+static int rx_copybreak __read_mostly = 1518;
 #else
-static int rx_copybreak;
+static int rx_copybreak __read_mostly;
 #endif
 
 /* Work-around for broken BIOSes: they are unable to get the chip back out of
    power state D3 so PXE booting fails. bootparam(7): via-rhine.avoid_D3=1 */
-static bool avoid_D3;
+static bool avoid_D3 __read_mostly;
 
 /*
  * In case you are looking for 'options[]' or 'full_duplex[]', they
-- 
1.7.2.5

[PATCH RFC 10/15] via-rhine: WOL: remove duplication into a helper.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index ca788c0..7e13d50 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -2168,6 +2168,14 @@ static void netdev_set_msglevel(struct net_device *dev, u32 value)
 	rp->msg_enable = value;
 }
 
+/* Indicates the set of WOL features supported by this card rev. */
+static inline u32
+rhine_wol_support_bits_get(struct rhine_private *rp)
+{
+	return	WAKE_PHY | WAKE_MAGIC |
+		WAKE_UCAST | WAKE_MCAST | WAKE_BCAST;	/* Untested */
+}
+
 static void
 rhine_ethop_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
@@ -2177,8 +2185,7 @@ rhine_ethop_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 		return;
 
 	spin_lock_irq(&rp->lock);
-	wol->supported = WAKE_PHY | WAKE_MAGIC |
-			 WAKE_UCAST | WAKE_MCAST | WAKE_BCAST;	/* Untested */
+	wol->supported = rhine_wol_support_bits_get(rp);
 	wol->wolopts = rp->wolopts;
 	spin_unlock_irq(&rp->lock);
 }
@@ -2187,13 +2194,11 @@ static int
 rhine_ethop_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct rhine_private *rp = netdev_priv(dev);
-	u32 support = WAKE_PHY | WAKE_MAGIC |
-		      WAKE_UCAST | WAKE_MCAST | WAKE_BCAST;	/* Untested */
 
 	if (!(rp->quirks & rqHaveWOL))
 		return -EINVAL;
 
-	if (wol->wolopts & ~support)
+	if (wol->wolopts & ~(rhine_wol_support_bits_get(rp)))
 		return -EINVAL;
 
 	spin_lock_irq(&rp->lock);
-- 
1.7.2.5

[PATCH RFC 11/15] via-rhine: WOL: separate WOL configuration (and its logging).

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Add helper rhine_power_state_configure().

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |  129 +++++++++++++++++++++-------------
 1 files changed, 80 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 7e13d50..8950eb0 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -562,6 +562,81 @@ static void rhine_ack_events(struct rhine_private *rp, u32 mask)
 	mmiowb();
 }
 
+/* Configures ACPI D0..D3 on card side. */
+static void
+rhine_power_state_configure(struct rhine_private *rp, u8 pci_state)
+{
+	void __iomem *ioaddr = rp->base;
+	u8 other_bits;
+	WARN_ON(!((pci_state >= 0) && (pci_state <= 3)));
+
+	other_bits = ioread8(ioaddr + StickyHW) & 0xFC;
+	iowrite8(other_bits | pci_state, ioaddr + StickyHW);
+}
+
+static inline void
+rhine_wol_configure(struct net_device *dev, u16 *wolstat_out)
+{
+	struct rhine_private *rp = netdev_priv(dev);
+	void __iomem *ioaddr = rp->base;
+	u16 wolstat = 0;
+
+	/* Make sure chip is in power state D0 */
+	rhine_power_state_configure(rp, 0);
+
+	/* Disable "force PME-enable" */
+	iowrite8(0x80, ioaddr + WOLcgClr);
+
+	/* Clear power-event config bits (WOL) */
+	iowrite8(0xFF, ioaddr + WOLcrClr);
+	/* More recent cards can manage two additional patterns */
+	if (rp->quirks & rq6patterns)
+		iowrite8(0x03, ioaddr + WOLcrClr1);
+
+	/* Save power-event status bits */
+	wolstat = ioread8(ioaddr + PwrcsrSet);
+	if (rp->quirks & rq6patterns)
+		wolstat |= (ioread8(ioaddr + PwrcsrSet1) & 0x03) << 8;
+
+	/* Clear power-event status bits */
+	iowrite8(0xFF, ioaddr + PwrcsrClr);
+	if (rp->quirks & rq6patterns)
+		iowrite8(0x03, ioaddr + PwrcsrClr1);
+
+	if (wolstat_out)
+		*wolstat_out = wolstat;
+}
+
+static inline void
+rhine_wol_log_wakeup_reason(struct net_device *dev, u16 wolstat)
+{
+	if (wolstat) {
+		const char *reason;
+		switch (wolstat) {
+		case WOLmagic:
+			reason = "Magic packet";
+			break;
+		case WOLlnkon:
+			reason = "Link went up";
+			break;
+		case WOLlnkoff:
+			reason = "Link went down";
+			break;
+		case WOLucast:
+			reason = "Unicast packet";
+			break;
+		case WOLbmcast:
+			reason = "Multicast/broadcast packet";
+			break;
+		default:
+			reason = "Unknown";
+			break;
+		}
+		netdev_info(dev, "Woke system up. Reason: %s\n",
+			    reason);
+	}
+}
+
 /*
  * Get power related registers into sane state.
  * Notify user about past WOL event.
@@ -569,56 +644,12 @@ static void rhine_ack_events(struct rhine_private *rp, u32 mask)
 static void rhine_power_init(struct net_device *dev)
 {
 	struct rhine_private *rp = netdev_priv(dev);
-	void __iomem *ioaddr = rp->base;
-	u16 wolstat;
 
 	if (rp->quirks & rqHaveWOL) {
-		/* Make sure chip is in power state D0 */
-		iowrite8(ioread8(ioaddr + StickyHW) & 0xFC, ioaddr + StickyHW);
-
-		/* Disable "force PME-enable" */
-		iowrite8(0x80, ioaddr + WOLcgClr);
-
-		/* Clear power-event config bits (WOL) */
-		iowrite8(0xFF, ioaddr + WOLcrClr);
-		/* More recent cards can manage two additional patterns */
-		if (rp->quirks & rq6patterns)
-			iowrite8(0x03, ioaddr + WOLcrClr1);
-
-		/* Save power-event status bits */
-		wolstat = ioread8(ioaddr + PwrcsrSet);
-		if (rp->quirks & rq6patterns)
-			wolstat |= (ioread8(ioaddr + PwrcsrSet1) & 0x03) << 8;
-
-		/* Clear power-event status bits */
-		iowrite8(0xFF, ioaddr + PwrcsrClr);
-		if (rp->quirks & rq6patterns)
-			iowrite8(0x03, ioaddr + PwrcsrClr1);
-
-		if (wolstat) {
-			char *reason;
-			switch (wolstat) {
-			case WOLmagic:
-				reason = "Magic packet";
-				break;
-			case WOLlnkon:
-				reason = "Link went up";
-				break;
-			case WOLlnkoff:
-				reason = "Link went down";
-				break;
-			case WOLucast:
-				reason = "Unicast packet";
-				break;
-			case WOLbmcast:
-				reason = "Multicast/broadcast packet";
-				break;
-			default:
-				reason = "Unknown";
-			}
-			netdev_info(dev, "Woke system up. Reason: %s\n",
-				    reason);
-		}
+		u16 wolstat;
+		rhine_wol_configure(dev, &wolstat);
+
+		rhine_wol_log_wakeup_reason(dev, wolstat);
 	}
 }
 
@@ -2325,7 +2356,7 @@ rhine_shutdown_and_keep_wol(struct pci_dev *pdev)
 	spin_unlock(&rp->lock);
 
 	if (system_state == SYSTEM_POWER_OFF && !avoid_D3) {
-		iowrite8(ioread8(ioaddr + StickyHW) | 0x03, ioaddr + StickyHW);
+		rhine_power_state_configure(rp, 3);
 
 		pci_wake_from_d3(pdev, true);
 		pci_set_power_state(pdev, PCI_D3hot);
-- 
1.7.2.5

[PATCH RFC 12/15] via-rhine: implement get_regs() ethtool ops.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Quite useful to pinpoint pre-suspend vs. post-resume register mismatches.
Adds io_size member for use in rhine_ethop_get_regs_len().

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   40 ++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 8950eb0..90d109a 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -452,6 +452,7 @@ struct rhine_private {
 	struct work_struct reset_task;
 
 	u32 msg_enable;
+	int io_size;
 
 	/* Frequently used values: keep some adjacent for cache effect. */
 	u32 quirks;
@@ -1001,6 +1002,7 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	rp = netdev_priv(dev);
 	rp->dev = dev;
+	rp->io_size = io_size;
 	rp->quirks = quirks;
 	rp->pioaddr = pioaddr;
 	rp->pdev = pdev;
@@ -2199,6 +2201,42 @@ static void netdev_set_msglevel(struct net_device *dev, u32 value)
 	rp->msg_enable = value;
 }
 
+/*
+ * ethtool get_regs() handlers.
+ * Use e.g. to pinpoint pre-suspend vs. post-resume register mismatches
+ * due to yet another all-too-frequent suspend/resume issue.
+ */
+static int
+rhine_ethop_get_regs_len(struct net_device *dev)
+{
+	struct rhine_private *rp = netdev_priv(dev);
+	return rp->io_size;
+}
+
+static void
+rhine_ethop_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *p)
+{
+	struct rhine_private *rp = netdev_priv(dev);
+	unsigned long flags;
+	void __iomem *ioaddr = rp->base;
+	u8 *p8 = (u8 *)p;
+	int len = regs->len;
+	int len_max = rhine_ethop_get_regs_len(dev);
+	int i = 0;
+
+	if (len > len_max)
+		len = len_max;
+
+	regs->version = 1;
+
+	memset(p, 0xFF, len);
+
+	spin_lock_irqsave(&rp->lock, flags);
+	for (i = 0; i < len; ++i)
+		p8[i] = ioread8(ioaddr + i);
+	spin_unlock_irqrestore(&rp->lock, flags);
+}
+
 /* Indicates the set of WOL features supported by this card rev. */
 static inline u32
 rhine_wol_support_bits_get(struct rhine_private *rp)
@@ -2241,6 +2279,8 @@ rhine_ethop_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 
 static const struct ethtool_ops netdev_ethtool_ops = {
 	.get_drvinfo		= netdev_get_drvinfo,
+	.get_regs_len		= rhine_ethop_get_regs_len,
+	.get_regs		= rhine_ethop_get_regs,
 	.get_settings		= netdev_get_settings,
 	.set_settings		= netdev_set_settings,
 	.nway_reset		= netdev_nway_reset,
-- 
1.7.2.5

[PATCH RFC 13/15] via-rhine: misc. cleanup.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Several register values remained open-coded - use their defines instead.
Improve register define grouping.
Improve log messages.
Add comments.

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   92 ++++++++++++++++++++++++++++------
 1 files changed, 76 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 90d109a..984f056 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -27,6 +27,34 @@
 	http://www.scyld.com/network/via-rhine.html
 	[link no longer provides useful info -jgarzik]
 
+
+	This driver should get some whack: several handlers
+	seem to have their concerns intermingled. Rather,
+	functions should be grouped into generically usable micro handlers
+	(I/O access concerns, WOL handling, MII, etc. - with card specifics
+	properly dealt with via suitable *internal* abstraction as needed)
+	which are then being invoked by *specifically-purposed*
+	high-level, system-side operational handlers
+	on an as-needed-in-this-scope basis.
+	But that's a very painful issue with too many drivers, unfortunately.
+	Some rather unrelated handling within a function may easily end up
+	becoming an unwanted "side effect" once other use cases appear,
+	with rather unclear grouping of concerns being the root cause :(
+	Or, to put it bluntly: you're in the wild here - an uncontrollable mass
+	of unwashed developers hacking away at things
+	(and breaking things willy-nilly whenever getting confused
+	about intentions of original implementation!!),
+	with the only marginal chance of getting this avoided being
+	to better get your core implementation, layering
+	and especially component naming right (well, DAMN RIGHT).
+
+	TODO: one example would be clean encapsulation of I/O register access -
+	such helper functions would optionally allow keeping a watch on access
+	to extended registers unsupported by earlier revs.
+
+	TODO list:
+		(see TODO annotations at future work sites below
+		 [more hunk-friendly])
 */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -299,18 +327,23 @@ enum register_offsets {
 	MIIPhyAddr=0x6C, MIIStatus=0x6D, PCIBusConfig=0x6E, PCIBusConfig1=0x6F,
 	MIICmd=0x70, MIIRegAddr=0x71, MIIData=0x72, MACRegEEcsr=0x74,
 	ConfigA=0x78, ConfigB=0x79, ConfigC=0x7A, ConfigD=0x7B,
-	RxMissed=0x7C, RxCRCErrs=0x7E, MiscCmd=0x81,
+	RxMissed = 0x7C, RxCRCErrs = 0x7E,
+	/*** Extended register range (newer revs only) starts here ***/
+	MiscCmd = 0x81,
 	StickyHW=0x83, IntrStatus2=0x84,
-	CamMask=0x88, CamCon=0x92, CamAddr=0x93,
-	WOLcrSet=0xA0, PwcfgSet=0xA1, WOLcgSet=0xA3, WOLcrClr=0xA4,
-	WOLcrClr1=0xA6, WOLcgClr=0xA7,
-	PwrcsrSet=0xA8, PwrcsrSet1=0xA9, PwrcsrClr=0xAC, PwrcsrClr1=0xAD,
+	CamMask = 0x88, CamCon = 0x92, CamAddr = 0x93,
+	/* various flag set/clear registers */
+	WOLcrSet = 0xA0, PwcfgSet = 0xA1, WOLcgSet = 0xA3,
+	WOLcrClr = 0xA4, WOLcrClr1 = 0xA6, WOLcgClr = 0xA7,
+	PwrcsrSet = 0xA8, PwrcsrSet1 = 0xA9,
+	PwrcsrClr = 0xAC, PwrcsrClr1 = 0xAD,
 };
 
 /* Bits in ConfigD */
 enum backoff_bits {
-	BackOptional=0x01, BackModify=0x02,
-	BackCaptureEffect=0x04, BackRandom=0x08
+	BackOptional = 0x01, BackModify = 0x02,
+	BackCaptureEffect = 0x04, BackRandom = 0x08,
+	MMIOEnable = 0x80
 };
 
 /* Bits in the TxConfig (TCR) register */
@@ -688,7 +721,7 @@ static void enable_mmio(long pioaddr, u32 quirks)
 		n = inb(pioaddr + ConfigA) | 0x20;
 		outb(n, pioaddr + ConfigA);
 	} else {
-		n = inb(pioaddr + ConfigD) | 0x80;
+		n = inb(pioaddr + ConfigD) | MMIOEnable;
 		outb(n, pioaddr + ConfigD);
 	}
 }
@@ -759,6 +792,18 @@ static void rhine_poll(struct net_device *dev)
 	struct rhine_private *rp = netdev_priv(dev);
 	const int irq = rp->pdev->irq;
 
+	/*
+	 * TODO: this "weird" section (passes in a "fake" irq number
+	 * param, etc.) here looks like some functionality inversion
+	 * is in order (i.e., probably the non-irq related IRQ handler core
+	 * should be moved into helper function, which then is the one
+	 * to *cleanly* be called from here, too!)
+	 * OTOH maybe it's a specific requirement to disable IRQ
+	 * and then do call the real handler impl instead -
+	 * but then a comment should have been provided,
+	 * to explain specifics of why we're manually calling
+	 * into the actual interrupt handler...
+	 */
 	disable_irq(irq);
 	rhine_interrupt(irq, dev);
 	enable_irq(irq);
@@ -771,7 +816,7 @@ static void rhine_kick_tx_threshold(struct rhine_private *rp)
 		void __iomem *ioaddr = rp->base;
 
 		rp->tx_thresh += 0x20;
-		BYTE_REG_BITS_SET(rp->tx_thresh, 0x80, ioaddr + TxConfig);
+		BYTE_REG_BITS_SET(rp->tx_thresh, TCR_RTSF, ioaddr + TxConfig);
 	}
 }
 
@@ -1044,6 +1089,7 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	rhine_power_init(dev);
 	rhine_hw_init(dev, rp->pioaddr);
 
+	/* Do bootstrap-only init steps (initial dev_addr, phy_id) */
 	for (i = 0; i < 6; i++)
 		dev->dev_addr[i] = ioread8(ioaddr + StationAddr + i);
 
@@ -1058,7 +1104,7 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* For Rhine-I/II, phy_id is loaded from EEPROM */
 	if (!phy_id)
-		phy_id = ioread8(ioaddr + 0x6C);
+		phy_id = ioread8(ioaddr + MIIPhyAddr);
 
 	spin_lock_init(&rp->lock);
 	mutex_init(&rp->task_lock);
@@ -1097,6 +1143,10 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_drvdata(pdev, dev);
 
+	/* FIXME!! I really *don't* think that this stuff has any business
+	 * being open-coded in probe() rather than being a helper possibly
+	 * called from multiple sites (resume, etc.) - netif_carrier_on()!!
+	 */
 	{
 		u16 mii_cmd;
 		int mii_status = mdio_read(dev, phy_id, 1);
@@ -1119,8 +1169,10 @@ static int rhine_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		}
 	}
 	rp->mii_if.phy_id = phy_id;
+
+	/* Use this occasion to announce avoid_D3 state, too. */
 	if (avoid_D3)
-		netif_info(rp, probe, dev, "No D3 power state at shutdown\n");
+		netif_info(rp, probe, dev, "Will avoid entering D3 power state at shutdown\n");
 
 	return 0;
 
@@ -1499,8 +1551,10 @@ static void init_registers(struct net_device *dev)
 
 	/* Initialize other registers. */
 	iowrite16(0x0006, ioaddr + PCIBusConfig);	/* Tune configuration??? */
+	/* Seems that it's DMA Length reg (select "store & forward" option). */
+
 	/* Configure initial FIFO thresholds. */
-	iowrite8(0x20, ioaddr + TxConfig);
+	iowrite8(TCR_RTFT0, ioaddr + TxConfig);
 	rp->tx_thresh = 0x20;
 	rp->rx_thresh = 0x60;		/* Written in rhine_set_rx_mode(). */
 
@@ -1543,7 +1597,7 @@ static void rhine_disable_linkmon(struct rhine_private *rp)
 	iowrite8(0, ioaddr + MIICmd);
 
 	if (rp->quirks & rqRhineI) {
-		iowrite8(0x01, ioaddr + MIIRegAddr);	// MII_BMSR
+		iowrite8(MII_BMSR, ioaddr + MIIRegAddr);
 
 		/* Can be called from ISR. Evil. */
 		mdelay(1);
@@ -2316,11 +2370,11 @@ static int rhine_close(struct net_device *dev)
 	napi_disable(&rp->napi);
 	netif_stop_queue(dev);
 
-	netif_dbg(rp, ifdown, dev, "Shutting down ethercard, status was %04x\n",
-		  ioread16(ioaddr + ChipCmd));
+	netif_dbg(rp, ifdown, dev, "%s() Shutting down ethercard, status was %04x\n",
+		  __func__, ioread16(ioaddr + ChipCmd));
 
 	/* Switch to loopback mode to avoid hardware races. */
-	iowrite8(rp->tx_thresh | 0x02, ioaddr + TxConfig);
+	iowrite8(rp->tx_thresh | TCR_LB0, ioaddr + TxConfig);
 
 	rhine_irq_disable(rp);
 
@@ -2369,6 +2423,11 @@ rhine_shutdown_and_keep_wol(struct pci_dev *pdev)
 
 	spin_lock(&rp->lock);
 
+	/*
+	 * We are able to poke single bits in sequence
+	 * (due to registers being organised as a set/clear combo).
+	 */
+
 	if (rp->wolopts & WAKE_MAGIC) {
 		iowrite8(WOLmagic, ioaddr + WOLcrSet);
 		/*
@@ -2404,6 +2463,7 @@ rhine_shutdown_and_keep_wol(struct pci_dev *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+/* TODO: implement full runtime pm, e.g. as done by r8169.c */
 static int rhine_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
-- 
1.7.2.5

[PATCH RFC 14/15] via-rhine: The Great Deduplication.

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

- remove descriptor call duplication into helper
- remove duplication into a helper variable
- remove force_media log duplication into helper
- remove IRQ unmask duplication into helper

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   67 ++++++++++++++++++++++------------
 1 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 984f056..051bb95 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -426,6 +426,9 @@ struct tx_desc {
 	__le32 next_desc;
 };
 
+/* An invalid address. */
+static const __le32 invalid_ring_address = cpu_to_le32(0xBADF00D0);
+
 /* Initial value for tx_desc.desc_length, Buffer size goes to bits 0-10 */
 #define TXDESC		0x00e08000
 
@@ -786,6 +789,19 @@ static void rhine_reload_eeprom(long pioaddr, struct net_device *dev)
 
 }
 
+static inline void
+rhine_irq_unmask(struct rhine_private *rp, u16 mask)
+{
+	iowrite16(mask, rp->base + IntrEnable);
+	mmiowb();
+}
+
+static void
+rhine_irq_disable(struct rhine_private *rp)
+{
+	rhine_irq_unmask(rp, 0x0000);
+}
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void rhine_poll(struct net_device *dev)
 {
@@ -930,8 +946,7 @@ static int rhine_napipoll(struct napi_struct *napi, int budget)
 
 	if (work_done < budget) {
 		napi_complete(napi);
-		iowrite16(enable_mask, ioaddr + IntrEnable);
-		mmiowb();
+		rhine_irq_unmask(rp, enable_mask);
 	}
 	return work_done;
 }
@@ -1287,7 +1302,7 @@ static void free_rbufs(struct net_device* dev)
 	/* Free all the skbuffs in the Rx queue. */
 	for (i = 0; i < RX_RING_SIZE; i++) {
 		rp->rx_ring[i].rx_status = 0;
-		rp->rx_ring[i].addr = cpu_to_le32(0xBADF00D0); /* An invalid address. */
+		rp->rx_ring[i].addr = invalid_ring_address;
 		if (rp->rx_skbuff[i]) {
 			pci_unmap_single(rp->pdev,
 					 rp->rx_skbuff_dma[i],
@@ -1327,7 +1342,7 @@ static void free_tbufs(struct net_device* dev)
 	for (i = 0; i < TX_RING_SIZE; i++) {
 		rp->tx_ring[i].tx_status = 0;
 		rp->tx_ring[i].desc_length = cpu_to_le32(TXDESC);
-		rp->tx_ring[i].addr = cpu_to_le32(0xBADF00D0); /* An invalid address. */
+		rp->tx_ring[i].addr = invalid_ring_address;
 		if (rp->tx_skbuff[i]) {
 			if (rp->tx_skbuff_dma[i]) {
 				pci_unmap_single(rp->pdev,
@@ -1342,6 +1357,25 @@ static void free_tbufs(struct net_device* dev)
 	}
 }
 
+static inline void
+rhine_realloc_all_descriptors(struct net_device *dev)
+{
+	free_tbufs(dev);
+	free_rbufs(dev);
+	alloc_tbufs(dev);
+	alloc_rbufs(dev);
+}
+
+static void
+rhine_force_media_log(struct mii_if_info *mii)
+{
+	struct net_device *dev = mii->dev;
+	struct rhine_private *rp = netdev_priv(dev);
+
+	netif_info(rp, link, dev, "force_media %d, carrier %d\n",
+		   mii->force_media, netif_carrier_ok(dev));
+}
+
 static void rhine_check_media(struct net_device *dev, unsigned int init_media)
 {
 	struct rhine_private *rp = netdev_priv(dev);
@@ -1356,15 +1390,13 @@ static void rhine_check_media(struct net_device *dev, unsigned int init_media)
 	    iowrite8(ioread8(ioaddr + ChipCmd1) & ~Cmd1FDuplex,
 		   ioaddr + ChipCmd1);
 
-	netif_info(rp, link, dev, "force_media %d, carrier %d\n",
-		   rp->mii_if.force_media, netif_carrier_ok(dev));
+	rhine_force_media_log(&rp->mii_if);
 }
 
 /* Called after status of force_media possibly changed */
 static void rhine_set_carrier(struct mii_if_info *mii)
 {
 	struct net_device *dev = mii->dev;
-	struct rhine_private *rp = netdev_priv(dev);
 
 	if (mii->force_media) {
 		/* autoneg is off: Link is always assumed to be up */
@@ -1373,8 +1405,7 @@ static void rhine_set_carrier(struct mii_if_info *mii)
 	} else	/* Let MII library update carrier status */
 		rhine_check_media(dev, 0);
 
-	netif_info(rp, link, dev, "force_media %d, carrier %d\n",
-		   mii->force_media, netif_carrier_ok(dev));
+	rhine_force_media_log(mii);
 }
 
 /**
@@ -1568,7 +1599,7 @@ static void init_registers(struct net_device *dev)
 
 	napi_enable(&rp->napi);
 
-	iowrite16(RHINE_EVENT & 0xffff, ioaddr + IntrEnable);
+	rhine_irq_unmask(rp, RHINE_EVENT & 0xffff);
 
 	iowrite16(CmdStart | CmdTxOn | CmdRxOn | (Cmd1NoTxPoll << 8),
 	       ioaddr + ChipCmd);
@@ -1717,10 +1748,7 @@ static void rhine_reset_task(struct work_struct *work)
 	spin_lock_bh(&rp->lock);
 
 	/* clear all descriptors */
-	free_tbufs(dev);
-	free_rbufs(dev);
-	alloc_tbufs(dev);
-	alloc_rbufs(dev);
+	rhine_realloc_all_descriptors(dev);
 
 	/* Reinitialize the hardware. */
 	rhine_chip_reset(dev);
@@ -1831,12 +1859,6 @@ static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
-static void rhine_irq_disable(struct rhine_private *rp)
-{
-	iowrite16(0x0000, rp->base + IntrEnable);
-	mmiowb();
-}
-
 /* The interrupt handler does all of the Rx thread work and cleans up
    after the Tx thread. */
 static irqreturn_t rhine_interrupt(int irq, void *dev_instance)
@@ -2524,10 +2546,7 @@ static int rhine_resume(struct device *device)
 	if (!netif_running(dev))
 		return 0;
 
-	free_tbufs(dev);
-	free_rbufs(dev);
-	alloc_tbufs(dev);
-	alloc_rbufs(dev);
+	rhine_realloc_all_descriptors(dev);
 	rhine_task_enable(rp);
 	spin_lock_bh(&rp->lock);
 	init_registers(dev);
-- 
1.7.2.5

[PATCH RFC 15/15] via-rhine: add helper (reduce type-unsafe void * assignments).

[Andreas Mohr]

From: Andreas Mohr <andim2@users.sf.net>

Signed-off-by: Andreas Mohr <andim2@users.sf.net>
---
 drivers/net/ethernet/via/via-rhine.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 051bb95..dd4fd04 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -2411,10 +2411,17 @@ static int rhine_close(struct net_device *dev)
 	return 0;
 }
 
+/* Cast-only helper (try to minimize type-unsafe void * assignments). */
+/* TODO: could be doing the same thing for netdev_priv() use. */
+static inline struct net_device *
+rhine_to_net_device(struct pci_dev *pdev)
+{
+	return pci_get_drvdata(pdev);
+}
 
 static void rhine_remove_one(struct pci_dev *pdev)
 {
-	struct net_device *dev = pci_get_drvdata(pdev);
+	struct net_device *dev = rhine_to_net_device(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 
 	unregister_netdev(dev);
@@ -2430,7 +2437,7 @@ static void rhine_remove_one(struct pci_dev *pdev)
 static void
 rhine_shutdown_and_keep_wol(struct pci_dev *pdev)
 {
-	struct net_device *dev = pci_get_drvdata(pdev);
+	struct net_device *dev = rhine_to_net_device(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 	void __iomem *ioaddr = rp->base;
 
@@ -2489,7 +2496,7 @@ rhine_shutdown_and_keep_wol(struct pci_dev *pdev)
 static int rhine_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
-	struct net_device *dev = pci_get_drvdata(pdev);
+	struct net_device *dev = rhine_to_net_device(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 
 	if (!netif_running(dev))
@@ -2509,7 +2516,7 @@ static int rhine_suspend(struct device *device)
 static int rhine_resume(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
-	struct net_device *dev = pci_get_drvdata(pdev);
+	struct net_device *dev = rhine_to_net_device(pdev);
 	struct rhine_private *rp = netdev_priv(dev);
 
 	/*
-- 
1.7.2.5

Re: [PATCH RFC 15/15] via-rhine: add helper (reduce type-unsafe void * assignments).

[David Miller]

From: Andreas Mohr <andi@lisas.de>
Date: Mon, 31 Dec 2012 16:25:49 +0100

> From: Andreas Mohr <andim2@users.sf.net>
> 
> Signed-off-by: Andreas Mohr <andim2@users.sf.net>

This is of zero value.

It is just as likely one would forget to use the helper as it would be
to assign pci_get_drvdata() to a pointer of the wrong type.