[PATCH 1/5] r8169: auto detect 32-bit slot

[PATCH 1/5] r8169: auto detect 32-bit slot

[Stephen Hemminger]

Change driver to auto-detect when the board is in a 32-bit PCI slot and
avoid setting 64-bit dma mask. The module parameter method is no longer needed.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/drivers/net/r8169.c b/drivers/net/r8169.c
--- a/drivers/net/r8169.c	2005-03-09 11:23:58 -08:00
+++ b/drivers/net/r8169.c	2005-03-09 11:23:58 -08:00
@@ -182,7 +182,6 @@
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
 static int rx_copybreak = 200;
-static int use_dac;
 
 enum RTL8169_registers {
 	MAC0 = 0,		/* Ethernet hardware address. */
@@ -419,8 +418,6 @@
 MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
 module_param_array(media, int, &num_media, 0);
 module_param(rx_copybreak, int, 0);
-module_param(use_dac, int, 0);
-MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot.");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(RTL8169_VERSION);
 
@@ -1224,18 +1221,6 @@
 
 	tp->cp_cmd = PCIMulRW | RxChkSum;
 
-	if ((sizeof(dma_addr_t) > 4) &&
-	    !pci_set_dma_mask(pdev, DMA_64BIT_MASK) && use_dac) {
-		tp->cp_cmd |= PCIDAC;
-		dev->features |= NETIF_F_HIGHDMA;
-	} else {
-		rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
-		if (rc < 0) {
-			printk(KERN_ERR PFX "DMA configuration failed.\n");
-			goto err_out_free_res;
-		}
-	}
-
 	pci_set_master(pdev);
 
 	/* ioremap MMIO region */
@@ -1278,6 +1263,19 @@
 		i++;
 	}
 	tp->chipset = i;
+
+	if ((sizeof(dma_addr_t) > 4) &&
+	    (RTL_R32(Config2) & (1<<3)) && 
+	    !pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
+		tp->cp_cmd |= PCIDAC;
+		dev->features |= NETIF_F_HIGHDMA;
+	} else {
+		rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+		if (rc < 0) {
+			printk(KERN_ERR PFX "DMA configuration failed.\n");
+			goto err_out_free_res;
+		}
+	}
 
 	*ioaddr_out = ioaddr;
 	*dev_out = dev;

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Jeff Garzik]

Stephen Hemminger wrote:
> Change driver to auto-detect when the board is in a 32-bit PCI slot and
> avoid setting 64-bit dma mask. The module parameter method is no longer needed.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

need to set consistent DMA mask too

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Andi Kleen]

Stephen Hemminger <shemminger@osdl.org> writes:

> Change driver to auto-detect when the board is in a 32-bit PCI slot and
> avoid setting 64-bit dma mask. The module parameter method is no longer needed.

Hmm? It doesn't support DAC? 

Normally on PCI a 64bit slot is not needed for 64bit addresses.

-Andi

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Francois Romieu]

Andi Kleen <ak@muc.de> :
[...]
> Hmm? It doesn't support DAC? 

It does but it is unstable for everybody (except Jon Mason, go figure) on
amd64.

Stephen, on which kind of system was this change tested ?

--
Ueimor

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Stephen Hemminger]

On Wed, 9 Mar 2005 22:40:23 +0100
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Andi Kleen <ak@muc.de> :
> [...]
> > Hmm? It doesn't support DAC? 
> 
> It does but it is unstable for everybody (except Jon Mason, go figure) on
> amd64.
> 
> Stephen, on which kind of system was this change tested ?

I tested on old Celeron with Netgear card.  I could try Amd and/or Xeon
if you want.

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Francois Romieu]

Stephen Hemminger <shemminger@osdl.org> :
[...]
> I tested on old Celeron with Netgear card.  I could try Amd and/or Xeon
> if you want.

If the systems are 64 bit, one would need to know how the driver behaves
when DAC is forced and:
1) the Netgear card is in a 32 bit slot, dma to highmem is enabled and used
2) the Netgear card is in a 32 bit slot, dma to highmem is enabled and unused
3) the Netgear card is in a 32 bit slot, dma to highmem is disabled
4) the Netgear card is in a 64 bit slot, dma to highmem is enabled and used
5) the Netgear card is in a 64 bit slot, dma to highmem is enabled and unused
6) the Netgear card is in a 64 bit slot, dma to highmem is disabled

+ the content of the Config2 register for 1-6.

If the amd64 hosts a built-in 8169, add the behavior with highmem disabled or
enabled + variable amount of memory.

As it still leaves some archs in the cold + DAC ought not to be dependant
on the bus width + a few "broken" adapters exist, the patch makes me a bit
sceptical.

--
Ueimor

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Stephen Hemminger]

On Thu, 10 Mar 2005 00:08:09 +0100
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Stephen Hemminger <shemminger@osdl.org> :
> [...]
> > I tested on old Celeron with Netgear card.  I could try Amd and/or Xeon
> > if you want.
> 
> If the systems are 64 bit, one would need to know how the driver behaves
> when DAC is forced and:
> 1) the Netgear card is in a 32 bit slot, dma to highmem is enabled and used
> 2) the Netgear card is in a 32 bit slot, dma to highmem is enabled and unused
> 3) the Netgear card is in a 32 bit slot, dma to highmem is disabled
> 4) the Netgear card is in a 64 bit slot, dma to highmem is enabled and used
> 5) the Netgear card is in a 64 bit slot, dma to highmem is enabled and unused
> 6) the Netgear card is in a 64 bit slot, dma to highmem is disabled
> 
> + the content of the Config2 register for 1-6.
> 
> If the amd64 hosts a built-in 8169, add the behavior with highmem disabled or
> enabled + variable amount of memory.
> 
> As it still leaves some archs in the cold + DAC ought not to be dependant
> on the bus width + a few "broken" adapters exist, the patch makes me a bit
> sceptical.

Okay, the motivation was to try and get rid of the module parameter.
I hate to see more parameters, and really don't like it when different drivers
start to do things in different ways.  Don't really have enough background
to know which adapters and arch combinations are broken; so just drop the patch.

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Andi Kleen]

On Thu, Mar 10, 2005 at 12:08:09AM +0100, Francois Romieu wrote:
> Stephen Hemminger <shemminger@osdl.org> :
> [...]
> > I tested on old Celeron with Netgear card.  I could try Amd and/or Xeon
> > if you want.
> 
> If the systems are 64 bit, one would need to know how the driver behaves
> when DAC is forced and:

The concept of "64bit systems" is useless here anyways. Even on a 32bit
system with highmem like i386 you can get >32bit addresses to deal
with. One usecase for that would be sendfile() out of user space.
This is widely used these days.

> 1) the Netgear card is in a 32 bit slot, dma to highmem is enabled and used
> 2) the Netgear card is in a 32 bit slot, dma to highmem is enabled and unused
> 3) the Netgear card is in a 32 bit slot, dma to highmem is disabled
> 4) the Netgear card is in a 64 bit slot, dma to highmem is enabled and used
> 5) the Netgear card is in a 64 bit slot, dma to highmem is enabled and unused
> 6) the Netgear card is in a 64 bit slot, dma to highmem is disabled

Again are you sure it depends on 64bit slots? Normally 64bit slots
only offer more bandwidth, but the address protocol is the same
as 32bit and both support DAC in the same way.

> 
> + the content of the Config2 register for 1-6.
> 
> If the amd64 hosts a built-in 8169, add the behavior with highmem disabled or
> enabled + variable amount of memory.

You really should forget about amd64 or not, all that counts is
sizeof(dma_addr_t) 

-Andi

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Francois Romieu]

Andi Kleen <ak@muc.de> :
[...]
> The concept of "64bit systems" is useless here anyways. Even on a 32bit
> system with highmem like i386 you can get >32bit addresses to deal
> with. One usecase for that would be sendfile() out of user space.
> This is widely used these days.

The combination "32 bit system with weird addressing mode + r8169 adapter"
has accounted for 0 bug/success report so far and I do not know a single
tester for it anyway. Whence my narrow-minded view of the issue :o)

[...]
> Again are you sure it depends on 64bit slots? Normally 64bit slots
> only offer more bandwidth, but the address protocol is the same
> as 32bit and both support DAC in the same way.

I do not know. The 8169 includes a register which tells (various things +)
the bus width. Without further documentation, I do not see how the relevance
of this register can be established/discarded if the aforementioned
test-cases are not experienced.

[...]
> You really should forget about amd64 or not, all that counts is
> sizeof(dma_addr_t) 

(assuming the on-board 8169 is wired correctly, be it on-board or not, yes)

--
Ueimor

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Jon Mason]

On Wednesday 09 March 2005 03:40 pm, Francois Romieu wrote:
> Andi Kleen <ak@muc.de> :
> [...]
>
> > Hmm? It doesn't support DAC?
>
> It does but it is unstable for everybody (except Jon Mason, go figure) on
> amd64.

Just lucky, I guess.  I can give it a try again.

> Stephen, on which kind of system was this change tested ?
>
> --
> Ueimor

-- 
Jon Mason
jdmason@us.ibm.com

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
[...
> Just lucky, I guess.  I can give it a try again.

The content of the Config2 register could be interesting.

--
Ueimor

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Jon Mason]

[...]
> The content of the Config2 register could be interesting.

I did a quick test and my 2 adapters (integrated and stand-alone) fail the DAC 
test.

+       if ((sizeof(dma_addr_t) > 4) &&
+           (RTL_R32(Config2) & (1<<3)) && 
+           !pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {

For the above, I get 8 0 1 respectively.  So, my adapters fail because of the 
64bit slot test.  

If you like, I can try my stand-alone adapter in a 64bit slot of a ppc64 
system.

Re: [PATCH 1/5] r8169: auto detect 32-bit slot

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
[...]
> +       if ((sizeof(dma_addr_t) > 4) &&
> +           (RTL_R32(Config2) & (1<<3)) && 
> +           !pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
> 
> For the above, I get 8 0 1 respectively.  So, my adapters fail because of the 
> 64bit slot test.  

Config2 can not be a replacement for "use_dac" then.

> If you like, I can try my stand-alone adapter in a 64bit slot of a ppc64 
> system.

It is not strictly necessary. Config2 ought to identify the 64bit slot at best.

Thanks.

--
Ueimor