[PATCH 0/2] atl1: Introduce CONFIG_ATL1_EXPERIMENTAL

[PATCH 0/2] atl1: Introduce CONFIG_ATL1_EXPERIMENTAL

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

	-- Chris

[PATCH 1/2] atl1: add CONFIG_ATL1_EXPERIMENTAL to kconfig

[Chris Snook]

From: Chris Snook <csnook@redhat.com>

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 <csnook@redhat.com>
Acked-by: Jay Cliburn <jacliburn@bellsouth.net>

--- a/drivers/net/Kconfig	2007-09-04 10:12:38.000000000 -0400
+++ b/drivers/net/Kconfig	2007-09-04 10:37:34.000000000 -0400
@@ -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
 
 #

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

[Chris Snook]

From: Chris Snook <csnook@redhat.com>

Make certain problematic optimizations build-time configurable.

Signed-off-by: Chris Snook <csnook@redhat.com>
Acked-by: Jay Cliburn <jacliburn@bellsouth.net>

--- a/drivers/net/atl1/atl1_main.c	2007-09-04 10:12:38.000000000 -0400
+++ b/drivers/net/atl1/atl1_main.c	2007-09-04 11:23:26.000000000 -0400
@@ -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;
 

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

[Luca]

On 9/8/07, Chris Snook <csnook@redhat.com> wrote:
> From: Chris Snook <csnook@redhat.com>
>
> Make certain problematic optimizations build-time configurable.
>
> Signed-off-by: Chris Snook <csnook@redhat.com>
> Acked-by: Jay Cliburn <jacliburn@bellsouth.net>
>
> --- a/drivers/net/atl1/atl1_main.c      2007-09-04 10:12:38.000000000 -0400
> +++ b/drivers/net/atl1/atl1_main.c      2007-09-04 11:23:26.000000000 -0400
> @@ -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 */

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

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

[Chris Snook]

Luca wrote:
> On 9/8/07, Chris Snook <csnook@redhat.com> wrote:
>> From: Chris Snook <csnook@redhat.com>
>>
>> Make certain problematic optimizations build-time configurable.
>>
>> Signed-off-by: Chris Snook <csnook@redhat.com>
>> Acked-by: Jay Cliburn <jacliburn@bellsouth.net>
>>
>> --- a/drivers/net/atl1/atl1_main.c      2007-09-04 10:12:38.000000000 -0400
>> +++ b/drivers/net/atl1/atl1_main.c      2007-09-04 11:23:26.000000000 -0400
>> @@ -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 */
> 
> 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

Re: [PATCH 0/2] atl1: Introduce CONFIG_ATL1_EXPERIMENTAL

[Jeff Garzik]

Chris Snook wrote:
> 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

Re: [PATCH 0/2] atl1: Introduce CONFIG_ATL1_EXPERIMENTAL

[Chris Snook]

Jeff Garzik wrote:
> Chris Snook wrote:
>> 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

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