atl1: Introduce CONFIG_ATL1_EXPERIMENTAL

PATCH In review Chris Snook 3 participants 7 messages Sep 7, 2007 – Sep 8, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Chris Snook

author

2007-09-07 23:41 UTC #0 lore raw

cover atl1: Introduce CONFIG_ATL1_EXPERIMENTAL

The atl1 driver is currently marked EXPERIMENTAL, because a few supposedly performance-enhancing features still have problems. When these features are disabled, the driver is completely stable, fully functional, and performs well.

Patch 1/2 Creates the kconfig option CONFIG_ATL1_EXPERIMENTAL, and removes the EXPERIMENTAL designation from CONFIG_ATL1

Patch 2/2 Wraps some currently-disabled features in #ifdef CONFIG_ATL1_EXPERIMENTAL, so developers and testers can play with these features more easily, and distributions will still get a fast, stable driver with existing .config files.

We'll also be using this to wrap around various new features we'll be experimenting with in coming months. Instead of using a half dozen different kconfig options for each of them, like some drivers do, we'll just use this, and make sure things are safe for everyone before we take them out of the experimental wrapper.

-- Chris

Chris Snook

author

2007-09-07 23:47 UTC #1 lore raw

PATCH 1/2 atl1: add CONFIG_ATL1_EXPERIMENTAL to kconfig

From: Chris Snook <[email protected]>

Introduce Kconfig ATL1_EXPERIMENTAL to separate mature code from less mature code in the atl1 driver, and remove EXPERIMENTAL designation for ATL1.

Signed-off-by Chris Snook <[email protected]>

Acked-by Jay Cliburn <[email protected]>

drivers/net/Kconfig +12 -2

@@ -2329,8 +2329,8 @@ config QLA3XXX
 	  will be called qla3xxx.
 
 config ATL1
-	tristate "Attansic L1 Gigabit Ethernet support (EXPERIMENTAL)"
-	depends on PCI && EXPERIMENTAL
+	tristate "Attansic L1 Gigabit Ethernet support"
+	depends on PCI
 	select CRC32
 	select MII
 	help

@@ -2339,6 +2339,16 @@ config ATL1
 	  To compile this driver as a module, choose M here.  The module
 	  will be called atl1.
 
+config ATL1_EXPERIMENTAL
+	bool "atl1 experimental features"
+	depends on ATL1 && EXPERIMENTAL
+	help
+	  This option enables various features that have not yet reached
+	  the maturity of the rest of the atl1 driver.  The driver will
+	  still work fine without this option enabled.
+
+	  If unsure, say N.
+
 endif # NETDEV_1000
 
 #

Chris Snook

author

2007-09-07 23:52 UTC #2 lore raw

PATCH 2/2 atl1: wrap problematic optimizations in CONFIG_ATL1_EXPERIMENTAL

In reply to Chris Snook

From: Chris Snook <[email protected]>

Make certain problematic optimizations build-time configurable.

Signed-off-by Chris Snook <[email protected]>

Acked-by Jay Cliburn <[email protected]>

drivers/net/atl1/atl1_main.c +16 ~~-10~~

@@ -2203,22 +2203,26 @@ static int __devinit atl1_probe(struct p
 	struct net_device *netdev;
 	struct atl1_adapter *adapter;
 	static int cards_found = 0;
-	bool pci_using_64 = true;
+	bool pci_using_64 = false;
 	int err;
 
 	err = pci_enable_device(pdev);
 	if (err)
 		return err;
 
+#ifdef CONFIG_ATL1_EXPERIMENTAL
 	err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
+	if (!err) {
+		pci_using_64 = true;
+		goto dma_ok;
+	}
+#endif /* CONFIG_ATL1_EXPERIMENTAL */
+	err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
 	if (err) {
-		err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
-		if (err) {
-			dev_err(&pdev->dev, "no usable DMA configuration\n");
-			goto err_dma;
-		}
-		pci_using_64 = false;
+		dev_err(&pdev->dev, "no usable DMA configuration\n");
+		goto err_dma;
 	}
+dma_ok:
 	/* Mark all PCI regions associated with PCI device
 	 * pdev as being reserved by owner atl1_driver_name
 	 */

@@ -2294,11 +2298,13 @@ static int __devinit atl1_probe(struct p
 	netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 
 	/*
-	 * FIXME - Until tso performance gets fixed, disable the feature.
+	 * TSO currently has performance problems,
+	 * so let's disable it by default.
 	 * Enable it with ethtool -K if desired.
 	 */
-	/* netdev->features |= NETIF_F_TSO; */
-
+#ifdef CONFIG_ATL1_EXPERIMENTAL
+	netdev->features |= NETIF_F_TSO;
+#endif
 	if (pci_using_64)
 		netdev->features |= NETIF_F_HIGHDMA;

Luca

2007-09-08 00:21 UTC #3 lore raw

Re: [atl1-devel] [PATCH 2/2] atl1: wrap problematic optimizations in CONFIG_ATL1_EXPERIMENTAL

Quoted 18 lines · Chris Snook

From: Chris Snook <[email protected]>
Make certain problematic optimizations build-time configurable.
Signed-off-by Chris Snook <[email protected]>
Acked-by Jay Cliburn <[email protected]>
drivers/net/atl1/atl1_main.c 2007-09-04 11:23:26.000000000 -0400 renamed from drivers/net/atl1/atl1_main.c 2007-09-04 10:12:38.000000000 -0400 +1 -1
@@ -2203,22 +2203,26 @@ static int __devinit atl1_probe(struct p
        struct net_device *netdev;
        struct atl1_adapter *adapter;
        static int cards_found = 0;
-       bool pci_using_64 = true;
+       bool pci_using_64 = false;
        int err;
 
        err = pci_enable_device(pdev);

drivers/net/atl1/atl1_main.c 2007-09-04 11:23:26.000000000 -0400 static int __devinit atl1_probe(struct p · line 2219

Quoted 10 lines

drivers/net/atl1/atl1_main.c 2007-09-04 11:23:26.000000000 -0400 renamed from drivers/net/atl1/atl1_main.c 2007-09-04 10:12:38.000000000 -0400 +6 -0

        if (err)
                return err;
 
+#ifdef CONFIG_ATL1_EXPERIMENTAL
        err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
+       if (!err) {
+               pci_using_64 = true;
+               goto dma_ok;
+       }
+#endif /* CONFIG_ATL1_EXPERIMENTAL */

This is more like CONFIG_ATL1_PLEASE_KILL_MY_MACHINE; I really don't see the problem with just limiting the DMA mask:
- if you don't have physical mem over the 4GB boundary limiting DMA doesn't make any difference
- if you have more than 4GB of memory the machine won't survive long without it

Luca

Chris Snook

author

2007-09-08 00:27 UTC #4 lore raw

Re: [atl1-devel] [PATCH 2/2] atl1: wrap problematic optimizations in CONFIG_ATL1_EXPERIMENTAL

Quoted 22 lines · Luca

Quoted 21 lines · Chris Snook
From: Chris Snook <[email protected]>
Make certain problematic optimizations build-time configurable.
Signed-off-by Chris Snook <[email protected]>
Acked-by Jay Cliburn <[email protected]>
drivers/net/atl1/atl1_main.c 2007-09-04 11:23:26.000000000 -0400 renamed from drivers/net/atl1/atl1_main.c 2007-09-04 10:12:38.000000000 -0400 +1 -1
@@ -2203,22 +2203,26 @@ static int __devinit atl1_probe(struct p
        struct net_device *netdev;
        struct atl1_adapter *adapter;
        static int cards_found = 0;
-       bool pci_using_64 = true;
+       bool pci_using_64 = false;
        int err;
 
        err = pci_enable_device(pdev);
        if (err)
                return err;
 

Quoted 13 lines

drivers/net/atl1/atl1_main.c 2007-09-04 11:23:26.000000000 -0400 static int __devinit atl1_probe(struct p · line 2219
Quoted 7 lines
drivers/net/atl1/atl1_main.c 2007-09-04 11:23:26.000000000 -0400 renamed from drivers/net/atl1/atl1_main.c 2007-09-04 10:12:38.000000000 -0400 +6 -0
+#ifdef CONFIG_ATL1_EXPERIMENTAL
        err = pci_set_dma_mask(pdev, DMA_64BIT_MASK);
+       if (!err) {
+               pci_using_64 = true;
+               goto dma_ok;
+       }
+#endif /* CONFIG_ATL1_EXPERIMENTAL */
This is more like CONFIG_ATL1_PLEASE_KILL_MY_MACHINE; I really don't see the problem with just limiting the DMA mask:
- if you don't have physical mem over the 4GB boundary limiting DMA doesn't make any difference
- if you have more than 4GB of memory the machine won't survive long without it

Atheros is still working on this, and we plan to fix it. 64-bit DMA *should* work. I just resubmitted your patch with the comment Jeff requested. I still may want to revisit CONFIG_ATL1_EXPERIMENTAL soon when I start playing around with more features.

-- Chris

Jeff Garzik

2007-09-07 23:54 UTC #5 lore raw

In reply to Chris Snook

Quoted 7 lines · Chris Snook

The atl1 driver is currently marked EXPERIMENTAL, because a few supposedly performance-enhancing features still have problems. When these features are disabled, the driver is completely stable, fully functional, and performs well.
Patch 1/2 Creates the kconfig option CONFIG_ATL1_EXPERIMENTAL, and removes the EXPERIMENTAL designation from CONFIG_ATL1
Patch 2/2 Wraps some currently-disabled features in #ifdef CONFIG_ATL1_EXPERIMENTAL, so developers and testers can play with these features more easily, and distributions will still get a fast, stable driver with existing .config files.
We'll also be using this to wrap around various new features we'll be experimenting with in coming months. Instead of using a half dozen different kconfig options for each of them, like some drivers do, we'll just use this, and make sure things are safe for everyone before we take them out of the experimental wrapper.

Well, I haven't received patch #2 yet, but in general a runtime switch (module option?) is greatly preferred.

Jeff

Chris Snook

author

2007-09-08 00:06 UTC #6 lore raw

Quoted 5 lines · Jeff Garzik

Quoted 4 lines · Chris Snook
The atl1 driver is currently marked EXPERIMENTAL, because a few supposedly performance-enhancing features still have problems. When these features are disabled, the driver is completely stable, fully functional, and performs well.
Patch 1/2 Creates the kconfig option CONFIG_ATL1_EXPERIMENTAL, and removes the EXPERIMENTAL designation from CONFIG_ATL1

Quoted 7 lines

Quoted 3 lines
Patch 2/2 Wraps some currently-disabled features in #ifdef CONFIG_ATL1_EXPERIMENTAL, so developers and testers can play with these features more easily, and distributions will still get a fast, stable driver with existing .config files.
We'll also be using this to wrap around various new features we'll be experimenting with in coming months. Instead of using a half dozen different kconfig options for each of them, like some drivers do, we'll just use this, and make sure things are safe for everyone before we take them out of the experimental wrapper.
Well, I haven't received patch #2 yet, but in general a runtime switch (module option?) is greatly preferred.
Jeff

Okay, I'll think about how we want to parameterize this. I don't want users expecting development options to be around forever. I'll resubmit something once I have more of these experimental features ready to submit.

-- Chris