[PATCH 0/5] sis900 printk and stack usage audit

[PATCH 0/5] sis900 printk and stack usage audit

[Daniele Venzano]

This patchset brings the debug output of the sis900 driver up to date,
making it actually useful.
There is also a rework of the code to avoid unnede calls to
sis630_set_eq and to remove repeated calls to pci_read_config_byte.

All the patches are made against latest netdev-2.6 tree (linus' sis900
+ altimata PHY patch)

I've tested all patches and the runtime debug parameter and everything
works as expected.

As usual all patches are available from
http://teg.homeunix.org/sis900.html

Thanks.

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org

[PATCH 1/5] sis900 printk and stack usage audit

[Daniele Venzano]

[-- Attachment #1.1: Type: text/plain, Size: 503 bytes --]

Audit of current printk() calls
Changed debug levels to 0,1,2,3 as follows:
 0 No debug
 1 load/probe/unload/suspend/resume stuff
 2 rx/tx errors
 3 rx/tx packets and every interrupt are logged (very verbose)
Debug levels are incremental
Removed double printing of version string in module_init and in sis900_probe
Made the sis900_debug parameter modifiable at runtime

Signed-off-by: Daniele Venzano <webvenza@libero.it>

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org


[-- Attachment #1.2: sis900-debug1.diff --]
[-- Type: text/plain, Size: 5233 bytes --]

Index: sis900.c
===================================================================
--- a/drivers/net/sis900.c	(revision 39)
+++ b/drivers/net/sis900.c	(revision 40)
@@ -183,10 +183,10 @@
 
 module_param(multicast_filter_limit, int, 0444);
 module_param(max_interrupt_work, int, 0444);
-module_param(debug, int, 0444);
+module_param(debug, int, 0644);
 MODULE_PARM_DESC(multicast_filter_limit, "SiS 900/7016 maximum number of filtered multicast addresses");
 MODULE_PARM_DESC(max_interrupt_work, "SiS 900/7016 maximum events handled per interrupt");
-MODULE_PARM_DESC(debug, "SiS 900/7016 debug level (2-4)");
+MODULE_PARM_DESC(debug, "SiS 900/7016 debug level (0-3)");
 
 static int sis900_open(struct net_device *net_dev);
 static int sis900_mii_probe (struct net_device * net_dev);
@@ -236,7 +236,7 @@
 	/* check to see if we have sane EEPROM */
 	signature = (u16) read_eeprom(ioaddr, EEPROMSignature);    
 	if (signature == 0xffff || signature == 0x0000) {
-		printk (KERN_INFO "%s: Error EERPOM read %x\n", 
+		printk (KERN_WARNING "%s: Error: EEPROM signature is %x\n", 
 			net_dev->name, signature);
 		return 0;
 	}
@@ -269,7 +269,7 @@
 	if (!isa_bridge) {
 		isa_bridge = pci_find_device(PCI_VENDOR_ID_SI, 0x0018, isa_bridge);
 		if (!isa_bridge) {
-			printk("%s: Can not find ISA bridge\n", net_dev->name);
+			printk(KERN_WARNING "%s: Can not find ISA bridge\n", net_dev->name);
 			return 0;
 		}
 	}
@@ -390,13 +390,6 @@
 	u8 revision;
 	char *card_name = card_names[pci_id->driver_data];
 
-/* when built into the kernel, we only print version if device is found */
-#ifndef MODULE
-	static int printed_version;
-	if (!printed_version++)
-		printk(version);
-#endif
-
 	/* setup various bits in PCI command register */
 	ret = pci_enable_device(pci_dev);
 	if(ret) return ret;
@@ -554,7 +547,7 @@
 			continue;
 		
 		if ((mii_phy = kmalloc(sizeof(struct mii_phy), GFP_KERNEL)) == NULL) {
-			printk(KERN_INFO "Cannot allocate mem for struct mii_phy\n");
+			printk(KERN_WARNING "Cannot allocate mem for struct mii_phy\n");
 			mii_phy = sis_priv->first_mii;
 			while (mii_phy) {
 				struct mii_phy *phy;
@@ -1015,8 +1008,8 @@
 		outl((i << RFADDR_shift), ioaddr + rfcr);
 		outl(w, ioaddr + rfdr);
 
-		if (sis900_debug > 2) {
-			printk(KERN_INFO "%s: Receive Filter Addrss[%d]=%x\n",
+		if (sis900_debug > 0) {
+			printk(KERN_DEBUG "%s: Receive Filter Addrss[%d]=%x\n",
 			       net_dev->name, i, inl(ioaddr + rfdr));
 		}
 	}
@@ -1053,8 +1046,8 @@
 
 	/* load Transmit Descriptor Register */
 	outl(sis_priv->tx_ring_dma, ioaddr + txdp);
-	if (sis900_debug > 2)
-		printk(KERN_INFO "%s: TX descriptor register loaded with: %8.8x\n",
+	if (sis900_debug > 0)
+		printk(KERN_DEBUG "%s: TX descriptor register loaded with: %8.8x\n",
 		       net_dev->name, inl(ioaddr + txdp));
 }
 
@@ -1107,8 +1100,8 @@
 
 	/* load Receive Descriptor Register */
 	outl(sis_priv->rx_ring_dma, ioaddr + rxdp);
-	if (sis900_debug > 2)
-		printk(KERN_INFO "%s: RX descriptor register loaded with: %8.8x\n",
+	if (sis900_debug > 0)
+		printk(KERN_DEBUG "%s: RX descriptor register loaded with: %8.8x\n",
 		       net_dev->name, inl(ioaddr + rxdp));
 }
 
@@ -1557,8 +1550,8 @@
 
 	net_dev->trans_start = jiffies;
 
-	if (sis900_debug > 3)
-		printk(KERN_INFO "%s: Queued Tx packet at %p size %d "
+	if (sis900_debug > 2)
+		printk(KERN_DEBUG "%s: Queued Tx packet at %p size %d "
 		       "to slot %d.\n",
 		       net_dev->name, skb->data, (int)skb->len, entry);
 
@@ -1617,8 +1610,8 @@
 		}
 	} while (1);
 
-	if (sis900_debug > 3)
-		printk(KERN_INFO "%s: exiting interrupt, "
+	if (sis900_debug > 2)
+		printk(KERN_DEBUG "%s: exiting interrupt, "
 		       "interrupt status = 0x%#8.8x.\n",
 		       net_dev->name, inl(ioaddr + isr));
 	
@@ -1643,8 +1636,8 @@
 	unsigned int entry = sis_priv->cur_rx % NUM_RX_DESC;
 	u32 rx_status = sis_priv->rx_ring[entry].cmdsts;
 
-	if (sis900_debug > 3)
-		printk(KERN_INFO "sis900_rx, cur_rx:%4.4d, dirty_rx:%4.4d "
+	if (sis900_debug > 2)
+		printk(KERN_DEBUG "sis900_rx, cur_rx:%4.4d, dirty_rx:%4.4d "
 		       "status:0x%8.8x\n",
 		       sis_priv->cur_rx, sis_priv->dirty_rx, rx_status);
 
@@ -1655,8 +1648,8 @@
 
 		if (rx_status & (ABORT|OVERRUN|TOOLONG|RUNT|RXISERR|CRCERR|FAERR)) {
 			/* corrupted packet received */
-			if (sis900_debug > 3)
-				printk(KERN_INFO "%s: Corrupted packet "
+			if (sis900_debug > 1)
+				printk(KERN_DEBUG "%s: Corrupted packet "
 				       "received, buffer status = 0x%8.8x.\n",
 				       net_dev->name, rx_status);
 			sis_priv->stats.rx_errors++;
@@ -1793,8 +1786,8 @@
 
 		if (tx_status & (ABORT | UNDERRUN | OWCOLL)) {
 			/* packet unsuccessfully transmitted */
-			if (sis900_debug > 3)
-				printk(KERN_INFO "%s: Transmit "
+			if (sis900_debug > 1)
+				printk(KERN_DEBUG "%s: Transmit "
 				       "error, Tx status %8.8x.\n",
 				       net_dev->name, tx_status);
 			sis_priv->stats.tx_errors++;
@@ -2047,12 +2040,10 @@
 		case IF_PORT_AUI: /* AUI */
 		case IF_PORT_100BASEFX: /* 100BaseFx */
                 	/* These Modes are not supported (are they?)*/
-			printk(KERN_INFO "Not supported");
 			return -EOPNOTSUPP;
 			break;
             
 		default:
-			printk(KERN_INFO "Invalid");
 			return -EINVAL;
 		}
 	}

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/5] sis900 printk and stack usage audit

[Jeff Garzik]

Daniele Venzano wrote:
> Audit of current printk() calls
> Changed debug levels to 0,1,2,3 as follows:
>  0 No debug
>  1 load/probe/unload/suspend/resume stuff
>  2 rx/tx errors
>  3 rx/tx packets and every interrupt are logged (very verbose)
> Debug levels are incremental
> Removed double printing of version string in module_init and in sis900_probe
> Made the sis900_debug parameter modifiable at runtime

Debug levels are standardized, please follow the standard.

All messages are bitmapped.  See NETIF_MSG_* and netif_msg_* in 
include/linux/netdevice.h.

Given the standard 'debug' module parameter, which takes a value from 
0-N (where N==10 or so), the driver uses netif_msg_init() to construct 
the bitmap of messages to be enabled.

	Jeff

Re: [PATCH 1/5] sis900 printk and stack usage audit

[Jeff Garzik]

Daniele Venzano wrote:
> Removed double printing of version string in module_init and in sis900_probe

> @@ -390,13 +390,6 @@
>  	u8 revision;
>  	char *card_name = card_names[pci_id->driver_data];
>  
> -/* when built into the kernel, we only print version if device is found */
> -#ifndef MODULE
> -	static int printed_version;
> -	if (!printed_version++)
> -		printk(version);
> -#endif
> -
>  	/* setup various bits in PCI command register */
>  	ret = pci_enable_device(pci_dev);
>  	if(ret) return ret;

There is no double-printing.  One is #ifndef MODULE, one is #ifdef MODULE.

Did you read the comment included in the code you deleted???

	Jeff

Re: [PATCH 1/5] sis900 printk and stack usage audit

[Daniele Venzano]

On Sat, Dec 11, 2004 at 02:06:21PM -0500, Jeff Garzik wrote:
> >@@ -390,13 +390,6 @@
> > 	u8 revision;
> > 	char *card_name = card_names[pci_id->driver_data];
> > 
> >-/* when built into the kernel, we only print version if device is found */
> >-#ifndef MODULE
> >-	static int printed_version;
> >-	if (!printed_version++)
> >-		printk(version);
> >-#endif
> >-
> > 	/* setup various bits in PCI command register */
> > 	ret = pci_enable_device(pci_dev);
> > 	if(ret) return ret;
> 
> There is no double-printing.  One is #ifndef MODULE, one is #ifdef MODULE.
> 
> Did you read the comment included in the code you deleted???

My bad, dropped.

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org

[PATCH 2/5] sis900 printk and stack usage audit

[Daniele Venzano]

[-- Attachment #1.1: Type: text/plain, Size: 186 bytes --]

Added some initialization debug output
Version bump

Signed-off-by: Daniele Venzano <webvenza@libero.it>

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org


[-- Attachment #1.2: sis900-debug2.diff --]
[-- Type: text/plain, Size: 4442 bytes --]

Index: sis900.c
===================================================================
--- a/drivers/net/sis900.c	(revision 40)
+++ b/drivers/net/sis900.c	(revision 41)
@@ -1,6 +1,6 @@
 /* sis900.c: A SiS 900/7016 PCI Fast Ethernet driver for Linux.
    Copyright 1999 Silicon Integrated System Corporation 
-   Revision:	1.08.06 Sep. 24 2002
+   Revision:	1.08.08 Nov. 28 2004
    
    Modified from the driver which is originally written by Donald Becker.
    
@@ -18,6 +18,7 @@
    preliminary Rev. 1.0 Jan. 18, 1998
    http://www.sis.com.tw/support/databook.htm
 
+   Rev 1.08.08 Nov. 28 2004 Daniele Venzano audit debug code and printk() calls
    Rev 1.08.07 Nov.  2 2003 Daniele Venzano <webvenza@libero.it> add suspend/resume support
    Rev 1.08.06 Sep. 24 2002 Mufasa Yang bug fix for Tx timeout & add SiS963 support
    Rev 1.08.05 Jun.  6 2002 Mufasa Yang bug fix for read_eeprom & Tx descriptor over-boundary
@@ -74,7 +75,7 @@
 #include "sis900.h"
 
 #define SIS900_MODULE_NAME "sis900"
-#define SIS900_DRV_VERSION "v1.08.07 11/02/2003"
+#define SIS900_DRV_VERSION "v1.08.08 Nov. 28 2004"
 
 static char version[] __devinitdata =
 KERN_INFO "sis900.c: " SIS900_DRV_VERSION "\n";
@@ -107,8 +108,6 @@
 };
 MODULE_DEVICE_TABLE (pci, sis900_pci_tbl);
 
-static void sis900_read_mode(struct net_device *net_dev, int *speed, int *duplex);
-
 static struct mii_chip_info {
 	const char * name;
 	u16 phy_id0;
@@ -216,6 +215,7 @@
 static u16 sis900_reset_phy(struct net_device *net_dev, int phy_addr);
 static void sis900_auto_negotiate(struct net_device *net_dev, int phy_addr);
 static void sis900_set_mode (long ioaddr, int speed, int duplex);
+static void sis900_read_mode(struct net_device *net_dev, int *speed, int *duplex);
 static struct ethtool_ops sis900_ethtool_ops;
 
 /**
@@ -269,7 +269,7 @@
 	if (!isa_bridge) {
 		isa_bridge = pci_find_device(PCI_VENDOR_ID_SI, 0x0018, isa_bridge);
 		if (!isa_bridge) {
-			printk(KERN_WARNING "%s: Can not find ISA bridge\n", net_dev->name);
+			printk(KERN_WARNING "%s: Cannot find ISA bridge\n", net_dev->name);
 			return 0;
 		}
 	}
@@ -455,10 +455,15 @@
 	if (ret)
 		goto err_unmap_rx;
 		
+	pci_read_config_byte(pci_dev, PCI_CLASS_REVISION, &revision);
+
+	if(sis900_debug > 0)
+		printk(KERN_DEBUG "%s: detected revision %2.2x,"
+				"trying to get MAC address...\n",
+				net_dev->name, revision);
+
 	/* Get Mac address according to the chip revision */
-	pci_read_config_byte(pci_dev, PCI_CLASS_REVISION, &revision);
 	ret = 0;
-
 	if (revision == SIS630E_900_REV)
 		ret = sis630e_get_mac_addr(pci_dev, net_dev);
 	else if ((revision > 0x81) && (revision <= 0x90) )
@@ -469,6 +474,7 @@
 		ret = sis900_get_mac_addr(pci_dev, net_dev);
 
 	if (ret == 0) {
+		printk(KERN_WARNING "%s: Cannot read MAC address.\n", net_dev->name);
 		ret = -ENODEV;
 		goto err_out_unregister;
 	}
@@ -479,6 +485,7 @@
 
 	/* probe for mii transceiver */
 	if (sis900_mii_probe(net_dev) == 0) {
+		printk(KERN_WARNING "%s: Error probing MII device.\n", net_dev->name);
 		ret = -ENODEV;
 		goto err_out_unregister;
 	}
@@ -542,9 +549,14 @@
 		for(i = 0; i < 2; i++)
 			mii_status = mdio_read(net_dev, phy_addr, MII_STATUS);
 
-		if (mii_status == 0xffff || mii_status == 0x0000)
+		if (mii_status == 0xffff || mii_status == 0x0000) {
 			/* the mii is not accessible, try next one */
+			if (sis900_debug > 0)
+				printk(KERN_DEBUG "%s: MII at address %d"
+						" not accessible\n",
+						net_dev->name, phy_addr);
 			continue;
+		}
 		
 		if ((mii_phy = kmalloc(sizeof(struct mii_phy), GFP_KERNEL)) == NULL) {
 			printk(KERN_WARNING "Cannot allocate mem for struct mii_phy\n");
@@ -568,14 +580,15 @@
 
 		for (i = 0; mii_chip_table[i].phy_id1; i++)
 			if ((mii_phy->phy_id0 == mii_chip_table[i].phy_id0 ) &&
-			    ((mii_phy->phy_id1 & 0xFFF0) == mii_chip_table[i].phy_id1)){
+			    ((mii_phy->phy_id1 & 0xFFF0) == mii_chip_table[i].phy_id1))
+			{
 				mii_phy->phy_types = mii_chip_table[i].phy_types;
 				if (mii_chip_table[i].phy_types == MIX)
-					mii_phy->phy_types =
-					    (mii_status & (MII_STAT_CAN_TX_FDX | MII_STAT_CAN_TX)) ? LAN : HOME;
+					mii_phy->phy_types = (mii_status & (MII_STAT_CAN_TX_FDX | MII_STAT_CAN_TX)) ? LAN : HOME;
 				printk(KERN_INFO "%s: %s transceiver found at address %d.\n",
-				       net_dev->name, mii_chip_table[i].name,
-				       phy_addr);
+							net_dev->name,
+							mii_chip_table[i].name,
+							phy_addr);
 				break;
 			}
 			

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/5] sis900 printk and stack usage audit

[Jeff Garzik]

Daniele Venzano wrote:
> @@ -455,10 +455,15 @@
>  	if (ret)
>  		goto err_unmap_rx;
>  		
> +	pci_read_config_byte(pci_dev, PCI_CLASS_REVISION, &revision);
> +
> +	if(sis900_debug > 0)
> +		printk(KERN_DEBUG "%s: detected revision %2.2x,"
> +				"trying to get MAC address...\n",
> +				net_dev->name, revision);
> +
>  	/* Get Mac address according to the chip revision */
> -	pci_read_config_byte(pci_dev, PCI_CLASS_REVISION, &revision);
>  	ret = 0;
> -

use netif_msg_* bitmap


> @@ -542,9 +549,14 @@
>  		for(i = 0; i < 2; i++)
>  			mii_status = mdio_read(net_dev, phy_addr, MII_STATUS);
>  
> -		if (mii_status == 0xffff || mii_status == 0x0000)
> +		if (mii_status == 0xffff || mii_status == 0x0000) {
>  			/* the mii is not accessible, try next one */
> +			if (sis900_debug > 0)
> +				printk(KERN_DEBUG "%s: MII at address %d"
> +						" not accessible\n",
> +						net_dev->name, phy_addr);
>  			continue;
> +		}
>  		

ditto


> @@ -568,14 +580,15 @@
>  
>  		for (i = 0; mii_chip_table[i].phy_id1; i++)
>  			if ((mii_phy->phy_id0 == mii_chip_table[i].phy_id0 ) &&
> -			    ((mii_phy->phy_id1 & 0xFFF0) == mii_chip_table[i].phy_id1)){
> +			    ((mii_phy->phy_id1 & 0xFFF0) == mii_chip_table[i].phy_id1))
> +			{

CodingStyle:  the brace following an 'if' does not exist on a line by 
itself.

	Jeff

Re: [PATCH 2/5] sis900 printk and stack usage audit

[Daniele Venzano]

On Sat, Dec 11, 2004 at 02:09:10PM -0500, Jeff Garzik wrote:
> >@@ -568,14 +580,15 @@
> > 
> > 		for (i = 0; mii_chip_table[i].phy_id1; i++)
> > 			if ((mii_phy->phy_id0 == mii_chip_table[i].phy_id0 ) 
> > 			&&
> >-			    ((mii_phy->phy_id1 & 0xFFF0) == 
> >mii_chip_table[i].phy_id1)){
> >+			    ((mii_phy->phy_id1 & 0xFFF0) == 
> >mii_chip_table[i].phy_id1))
> >+			{
> 
> CodingStyle:  the brace following an 'if' does not exist on a line by 
> itself.

The line is too long, and it is difficult to see where the if block
begins, I know it is an exception to the Golden Rules, but I thought a
line wasted in code readbility was well wasted.
However I don't think it really matters, will drop it.

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org

[PATCH 3/5] sis900 printk and stack usage audit

[Daniele Venzano]

[-- Attachment #1.1: Type: text/plain, Size: 278 bytes --]

Chip revision is now a member of sis_priv structure
Kill all calls to pci_read_config_byte but one
Change the code to use sis_priv->chipset_rev

Signed-off-by: Daniele Venzano <webvenza@libero.it>

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org


[-- Attachment #1.2: sis900-chipset-revision.diff --]
[-- Type: text/plain, Size: 5030 bytes --]

Index: sis900.c
===================================================================
--- a/drivers/net/sis900.c	(revision 41)
+++ b/drivers/net/sis900.c	(revision 42)
@@ -173,6 +173,7 @@
 
 	unsigned int tx_full; /* The Tx queue is full. */
 	u8 host_bridge_rev;
+	u8 chipset_rev;
 	u32 pci_state[16];
 };
 
@@ -387,7 +388,6 @@
 	void *ring_space;
 	long ioaddr;
 	int i, ret;
-	u8 revision;
 	char *card_name = card_names[pci_id->driver_data];
 
 	/* setup various bits in PCI command register */
@@ -455,20 +455,20 @@
 	if (ret)
 		goto err_unmap_rx;
 		
-	pci_read_config_byte(pci_dev, PCI_CLASS_REVISION, &revision);
+	pci_read_config_byte(pci_dev, PCI_CLASS_REVISION, &(sis_priv->chipset_rev));
 
 	if(sis900_debug > 0)
 		printk(KERN_DEBUG "%s: detected revision %2.2x,"
 				"trying to get MAC address...\n",
-				net_dev->name, revision);
+				net_dev->name, sis_priv->chipset_rev);
 
 	/* Get Mac address according to the chip revision */
 	ret = 0;
-	if (revision == SIS630E_900_REV)
+	if (sis_priv->chipset_rev == SIS630E_900_REV)
 		ret = sis630e_get_mac_addr(pci_dev, net_dev);
-	else if ((revision > 0x81) && (revision <= 0x90) )
+	else if ((sis_priv->chipset_rev > 0x81) && (sis_priv->chipset_rev <= 0x90))
 		ret = sis635_get_mac_addr(pci_dev, net_dev);
-	else if (revision == SIS96x_900_REV)
+	else if (sis_priv->chipset_rev == SIS96x_900_REV)
 		ret = sis96x_get_mac_addr(pci_dev, net_dev);
 	else
 		ret = sis900_get_mac_addr(pci_dev, net_dev);
@@ -480,7 +480,7 @@
 	}
 	
 	/* 630ET : set the mii access mode as software-mode */
-	if (revision == SIS630ET_900_REV)
+	if (sis_priv->chipset_rev == SIS630ET_900_REV)
 		outl(ACCESSMODE | inl(ioaddr + cr), ioaddr + cr);
 
 	/* probe for mii transceiver */
@@ -535,7 +535,6 @@
 	u16 poll_bit = MII_STAT_LINK, status = 0;
 	unsigned long timeout = jiffies + 5 * HZ;
 	int phy_addr;
-	u8 revision;
 
 	sis_priv->mii = NULL;
 
@@ -632,8 +631,7 @@
 		}
 	}
 
-	pci_read_config_byte(sis_priv->pci_dev, PCI_CLASS_REVISION, &revision);
-	if (revision == SIS630E_900_REV) {
+	if (sis_priv->chipset_rev == SIS630E_900_REV) {
 		/* SiS 630E has some bugs on default value of PHY registers */
 		mdio_write(net_dev, sis_priv->cur_phy, MII_ANADV, 0x05e1);
 		mdio_write(net_dev, sis_priv->cur_phy, MII_CONFIG1, 0x22);
@@ -948,15 +946,13 @@
 {
 	struct sis900_private *sis_priv = net_dev->priv;
 	long ioaddr = net_dev->base_addr;
-	u8 revision;
 	int ret;
 
 	/* Soft reset the chip. */
 	sis900_reset(net_dev);
 
 	/* Equalizer workaround Rule */
-	pci_read_config_byte(sis_priv->pci_dev, PCI_CLASS_REVISION, &revision);
-	sis630_set_eq(net_dev, revision);
+	sis630_set_eq(net_dev, sis_priv->chipset_rev);
 
 	ret = request_irq(net_dev->irq, &sis900_interrupt, SA_SHIRQ,
 						net_dev->name, net_dev);
@@ -1224,7 +1220,6 @@
 	struct mii_phy *mii_phy = sis_priv->mii;
 	static int next_tick = 5*HZ;
 	u16 status;
-	u8 revision;
 
 	if (!sis_priv->autong_complete){
 		int speed, duplex = 0;
@@ -1232,9 +1227,7 @@
 		sis900_read_mode(net_dev, &speed, &duplex);
 		if (duplex){
 			sis900_set_mode(net_dev->base_addr, speed, duplex);
-			pci_read_config_byte(sis_priv->pci_dev,
-						PCI_CLASS_REVISION, &revision);
-			sis630_set_eq(net_dev, revision);
+			sis630_set_eq(net_dev, sis_priv->chipset_rev);
 			netif_start_queue(net_dev);
 		}
 
@@ -1268,9 +1261,7 @@
 			    ((mii_phy->phy_id1 & 0xFFF0) == 0x8000))
                			sis900_reset_phy(net_dev,  sis_priv->cur_phy);
   
-                	pci_read_config_byte(sis_priv->pci_dev,
-					PCI_CLASS_REVISION, &revision);
-			sis630_set_eq(net_dev, revision);
+			sis630_set_eq(net_dev, sis_priv->chipset_rev);
   
                 	goto LookForLink;
                 }
@@ -2102,11 +2093,10 @@
 	u16 mc_filter[16] = {0};	/* 256/128 bits multicast hash table */
 	int i, table_entries;
 	u32 rx_mode;
-	u8 revision;
 
 	/* 635 Hash Table entires = 256(2^16) */
-	pci_read_config_byte(sis_priv->pci_dev, PCI_CLASS_REVISION, &revision);
-	if((revision >= SIS635A_900_REV) || (revision == SIS900B_900_REV))
+	if((sis_priv->chipset_rev >= SIS635A_900_REV) ||
+			(sis_priv->chipset_rev == SIS900B_900_REV))
 		table_entries = 16;
 	else
 		table_entries = 8;
@@ -2132,7 +2122,7 @@
 			mclist && i < net_dev->mc_count;
 			i++, mclist = mclist->next) {
 			unsigned int bit_nr =
-				sis900_mcast_bitnr(mclist->dmi_addr, revision);
+				sis900_mcast_bitnr(mclist->dmi_addr, sis_priv->chipset_rev);
 			mc_filter[bit_nr >> 4] |= (1 << (bit_nr & 0xf));
 		}
 	}
@@ -2178,7 +2168,6 @@
 	long ioaddr = net_dev->base_addr;
 	int i = 0;
 	u32 status = TxRCMP | RxRCMP;
-	u8  revision;
 
 	outl(0, ioaddr + ier);
 	outl(0, ioaddr + imr);
@@ -2191,8 +2180,8 @@
 		status ^= (inl(isr + ioaddr) & status);
 	}
 
-	pci_read_config_byte(sis_priv->pci_dev, PCI_CLASS_REVISION, &revision);
-	if( (revision >= SIS635A_900_REV) || (revision == SIS900B_900_REV) )
+	if( (sis_priv->chipset_rev >= SIS635A_900_REV) ||
+			(sis_priv->chipset_rev == SIS900B_900_REV) )
 		outl(PESEL | RND_CNT, ioaddr + cfg);
 	else
 		outl(PESEL, ioaddr + cfg);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/5] sis900 printk and stack usage audit

[Jeff Garzik]

Daniele Venzano wrote:
> Chip revision is now a member of sis_priv structure
> Kill all calls to pci_read_config_byte but one
> Change the code to use sis_priv->chipset_rev

OK (but not applied, since previous patches in series were not applied)

[PATCH 4/5] sis900 printk and stack usage audit

[Daniele Venzano]

[-- Attachment #1.1: Type: text/plain, Size: 220 bytes --]

Don't do useless calls of sis630_set_eq
Kill now unneeded paramenter of sis630_set_eq

Signed-off-by: Daniele Venzano <webvenza@libero.it>

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org


[-- Attachment #1.2: sis900-sis630-set-eq.diff --]
[-- Type: text/plain, Size: 4032 bytes --]

Index: sis900.c
===================================================================
--- a/drivers/net/sis900.c	(revision 42)
+++ b/drivers/net/sis900.c	(revision 43)
@@ -209,7 +209,7 @@
 static u16 sis900_mcast_bitnr(u8 *addr, u8 revision);
 static void set_rx_mode(struct net_device *net_dev);
 static void sis900_reset(struct net_device *net_dev);
-static void sis630_set_eq(struct net_device *net_dev, u8 revision);
+static void sis630_set_eq(struct net_device *net_dev);
 static int sis900_set_config(struct net_device *dev, struct ifmap *map);
 static u16 sis900_default_phy(struct net_device * net_dev);
 static void sis900_set_capability( struct net_device *net_dev ,struct mii_phy *phy);
@@ -952,7 +952,11 @@
 	sis900_reset(net_dev);
 
 	/* Equalizer workaround Rule */
-	sis630_set_eq(net_dev, sis_priv->chipset_rev);
+	if (sis_priv->chipset_rev == SIS630E_900_REV ||
+			sis_priv->chipset_rev == SIS630EA1_900_REV ||
+			sis_priv->chipset_rev == SIS630A_900_REV ||
+			sis_priv->chipset_rev ==  SIS630ET_900_REV)
+		sis630_set_eq(net_dev);
 
 	ret = request_irq(net_dev->irq, &sis900_interrupt, SA_SHIRQ,
 						net_dev->name, net_dev);
@@ -1141,16 +1145,12 @@
  *	max >= 15      --> set equalizer to max+5 or set equalizer to max+6 if max == min
  */
 
-static void sis630_set_eq(struct net_device *net_dev, u8 revision)
+static void sis630_set_eq(struct net_device *net_dev)
 {
 	struct sis900_private *sis_priv = net_dev->priv;
 	u16 reg14h, eq_value=0, max_value=0, min_value=0;
 	int i, maxcount=10;
 
-	if ( !(revision == SIS630E_900_REV || revision == SIS630EA1_900_REV ||
-	       revision == SIS630A_900_REV || revision ==  SIS630ET_900_REV) )
-		return;
-
 	if (netif_carrier_ok(net_dev)) {
 		reg14h = mdio_read(net_dev, sis_priv->cur_phy, MII_RESV);
 		mdio_write(net_dev, sis_priv->cur_phy, MII_RESV,
@@ -1166,8 +1166,9 @@
 						eq_value : min_value;
 		}
 		/* 630E rule to determine the equalizer value */
-		if (revision == SIS630E_900_REV || revision == SIS630EA1_900_REV ||
-		    revision == SIS630ET_900_REV) {
+		if (sis_priv->chipset_rev == SIS630E_900_REV ||
+				sis_priv->chipset_rev == SIS630EA1_900_REV ||
+				sis_priv->chipset_rev == SIS630ET_900_REV) {
 			if (max_value < 5)
 				eq_value = max_value;
 			else if (max_value >= 5 && max_value < 15)
@@ -1178,7 +1179,7 @@
 						max_value+6 : max_value+5;
 		}
 		/* 630B0&B1 rule to determine the equalizer value */
-		if (revision == SIS630A_900_REV && 
+		if (sis_priv->chipset_rev == SIS630A_900_REV && 
 		    (sis_priv->host_bridge_rev == SIS630B0 || 
 		     sis_priv->host_bridge_rev == SIS630B1)) {
 			if (max_value == 0)
@@ -1193,7 +1194,7 @@
 		mdio_write(net_dev, sis_priv->cur_phy, MII_RESV, reg14h);
 	} else {
 		reg14h = mdio_read(net_dev, sis_priv->cur_phy, MII_RESV);
-		if (revision == SIS630A_900_REV && 
+		if (sis_priv->chipset_rev == SIS630A_900_REV && 
 		    (sis_priv->host_bridge_rev == SIS630B0 || 
 		     sis_priv->host_bridge_rev == SIS630B1)) 
 			mdio_write(net_dev, sis_priv->cur_phy, MII_RESV,
@@ -1227,7 +1228,11 @@
 		sis900_read_mode(net_dev, &speed, &duplex);
 		if (duplex){
 			sis900_set_mode(net_dev->base_addr, speed, duplex);
-			sis630_set_eq(net_dev, sis_priv->chipset_rev);
+			if (sis_priv->chipset_rev == SIS630E_900_REV ||
+					sis_priv->chipset_rev == SIS630EA1_900_REV ||
+					sis_priv->chipset_rev == SIS630A_900_REV ||
+					sis_priv->chipset_rev ==  SIS630ET_900_REV)
+				sis630_set_eq(net_dev);
 			netif_start_queue(net_dev);
 		}
 
@@ -1260,9 +1265,13 @@
                 	if ((mii_phy->phy_id0 == 0x001D) && 
 			    ((mii_phy->phy_id1 & 0xFFF0) == 0x8000))
                			sis900_reset_phy(net_dev,  sis_priv->cur_phy);
+
+			if (sis_priv->chipset_rev == SIS630E_900_REV ||
+					sis_priv->chipset_rev == SIS630EA1_900_REV ||
+					sis_priv->chipset_rev == SIS630A_900_REV ||
+					sis_priv->chipset_rev ==  SIS630ET_900_REV)
+				sis630_set_eq(net_dev);
   
-			sis630_set_eq(net_dev, sis_priv->chipset_rev);
-  
                 	goto LookForLink;
                 }
 	}

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/5] sis900 printk and stack usage audit

[Jeff Garzik]

Daniele Venzano wrote:
> @@ -952,7 +952,11 @@
>  	sis900_reset(net_dev);
>  
>  	/* Equalizer workaround Rule */
> -	sis630_set_eq(net_dev, sis_priv->chipset_rev);
> +	if (sis_priv->chipset_rev == SIS630E_900_REV ||
> +			sis_priv->chipset_rev == SIS630EA1_900_REV ||
> +			sis_priv->chipset_rev == SIS630A_900_REV ||
> +			sis_priv->chipset_rev ==  SIS630ET_900_REV)
> +		sis630_set_eq(net_dev);
>  
>  	ret = request_irq(net_dev->irq, &sis900_interrupt, SA_SHIRQ,
>  						net_dev->name, net_dev);
> @@ -1141,16 +1145,12 @@
>   *	max >= 15      --> set equalizer to max+5 or set equalizer to max+6 if max == min
>   */
>  
> -static void sis630_set_eq(struct net_device *net_dev, u8 revision)
> +static void sis630_set_eq(struct net_device *net_dev)
>  {
>  	struct sis900_private *sis_priv = net_dev->priv;
>  	u16 reg14h, eq_value=0, max_value=0, min_value=0;
>  	int i, maxcount=10;
>  
> -	if ( !(revision == SIS630E_900_REV || revision == SIS630EA1_900_REV ||
> -	       revision == SIS630A_900_REV || revision ==  SIS630ET_900_REV) )
> -		return;
> -
>  	if (netif_carrier_ok(net_dev)) {
>  		reg14h = mdio_read(net_dev, sis_priv->cur_phy, MII_RESV);
>  		mdio_write(net_dev, sis_priv->cur_phy, MII_RESV,


This is a step backwards.

You are _adding_ multiple copies of the same piece of code, just to 
avoid calling a function.

	Jeff

Re: [PATCH 4/5] sis900 printk and stack usage audit

[Daniele Venzano]

On Sat, Dec 11, 2004 at 02:11:13PM -0500, Jeff Garzik wrote:
> This is a step backwards.
> 
> You are _adding_ multiple copies of the same piece of code, just to 
> avoid calling a function.

Ok, I'm learning what is good and what is bad. Patch dropped.

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org

[PATCH 5/5] sis900 printk and stack usage audit

[Daniele Venzano]

[-- Attachment #1.1: Type: text/plain, Size: 199 bytes --]

Change comment to reflect changes in parameters od sis630_set_eq

Signed-off-by: Daniele Venzano <webvenza@libero.it>

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org


[-- Attachment #1.2: sis900-sis630-docs.diff --]
[-- Type: text/plain, Size: 425 bytes --]

Index: sis900.c
===================================================================
--- a/drivers/net/sis900.c	(revision 43)
+++ b/drivers/net/sis900.c	(revision 44)
@@ -1121,7 +1121,6 @@
 /**
  *	sis630_set_eq - set phy equalizer value for 630 LAN
  *	@net_dev: the net device to set equalizer value
- *	@revision: 630 LAN revision number
  *
  *	630E equalizer workaround rule(Cyrus Huang 08/15)
  *	PHY register 14h(Test)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 5/5] sis900 printk and stack usage audit

[Jeff Garzik]

Daniele Venzano wrote:
> Change comment to reflect changes in parameters od sis630_set_eq

OK, but not applied due to comments on previous patches

Re: [PATCH 5/5] sis900 printk and stack usage audit

[Daniele Venzano]

On Sat, Dec 11, 2004 at 02:11:30PM -0500, Jeff Garzik wrote:
> Daniele Venzano wrote:
> >Change comment to reflect changes in parameters od sis630_set_eq
> 
> OK, but not applied due to comments on previous patches

Dropped, since I dropped patch 4/5.
In the end I'll do new patches for the debugging stuff following netif
standard (I didn't know of those :-( ) and rediff the chipset revision
stuff.

I hope to send these two or three patches later on today.

Thanks, bye.

-- 
-----------------------------
Daniele Venzano
Web: http://teg.homeunix.org