r8169: default to 64-bit DMA on systems without memory below 4 GB

Message ID 1462952869-23498-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel May 11, 2016, 7:47 a.m.
The current logic around the 'use_dac' module parameter prevents the
r81969 driver from being loadable on 64-bit systems without any RAM
below 4 GB when the parameter is left at its default value.
So introduce a new default value -1 which indicates that 64-bit DMA
should be enabled implicitly, but only if setting a 32-bit DMA mask
has failed earlier. This should prevent any regressions like the ones
caused by previous attempts to change this code.

Cc: Realtek linux nic maintainers <nic_swsd@realtek.com> 
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 drivers/net/ethernet/realtek/r8169.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.7.4

Comments

Ard Biesheuvel May 11, 2016, 8:34 p.m. | #1
On 11 May 2016 at 22:31, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> :

>> The current logic around the 'use_dac' module parameter prevents the

>> r81969 driver from being loadable on 64-bit systems without any RAM

>> below 4 GB when the parameter is left at its default value.

>> So introduce a new default value -1 which indicates that 64-bit DMA

>> should be enabled implicitly, but only if setting a 32-bit DMA mask

>> has failed earlier. This should prevent any regressions like the ones

>> caused by previous attempts to change this code.

>

> I am not a huge fan but if you really need it...

>

> Which current kernel arches do exhibit the interesting

> f*ck-legacy-32-bits-only-devices property you just described ?

>


It has little to do with f*cking legacy 32-bits-only-devices if DRAM
simply starts at 0x80_0000_0000. This is on an AMD arm64 chip.

> [...]

>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c

>> index 94f08f1e841c..a49e8a58e539 100644

>> --- a/drivers/net/ethernet/realtek/r8169.c

>> +++ b/drivers/net/ethernet/realtek/r8169.c

> [...]

>> @@ -859,7 +859,8 @@ struct rtl8169_private {

>>  MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");

>>  MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");

>>  module_param(use_dac, int, 0);

>> -MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot.");

>> +MODULE_PARM_DESC(use_dac,

>> +     "Enable PCI DAC. Unsafe on 32 bit PCI slot (default -1: enable on 64-bit archs only if needed");

>

> Nit: the parameter is bizarre enough that you could leave the original

> description.

>


OK, if you prefer. Should I send a v2?
David Miller May 11, 2016, 11:58 p.m. | #2
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Date: Wed, 11 May 2016 09:47:49 +0200

> The current logic around the 'use_dac' module parameter prevents the

> r81969 driver from being loadable on 64-bit systems without any RAM

> below 4 GB when the parameter is left at its default value.

> So introduce a new default value -1 which indicates that 64-bit DMA

> should be enabled implicitly, but only if setting a 32-bit DMA mask

> has failed earlier. This should prevent any regressions like the ones

> caused by previous attempts to change this code.

> 

> Cc: Realtek linux nic maintainers <nic_swsd@realtek.com> 

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


I think we should just seriously consider changing the default, it's
a really outdated reasoning behind the current default setting.  Maybe
relevant a decade ago, but probably not now.

And if the card is completely disfunctional in said configuration, the
default is definitely wrong.
Ard Biesheuvel May 12, 2016, 7:58 a.m. | #3
On 12 May 2016 at 01:58, David Miller <davem@davemloft.net> wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Date: Wed, 11 May 2016 09:47:49 +0200

>

>> The current logic around the 'use_dac' module parameter prevents the

>> r81969 driver from being loadable on 64-bit systems without any RAM

>> below 4 GB when the parameter is left at its default value.

>> So introduce a new default value -1 which indicates that 64-bit DMA

>> should be enabled implicitly, but only if setting a 32-bit DMA mask

>> has failed earlier. This should prevent any regressions like the ones

>> caused by previous attempts to change this code.

>>

>> Cc: Realtek linux nic maintainers <nic_swsd@realtek.com>

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> I think we should just seriously consider changing the default, it's

> a really outdated reasoning behind the current default setting.  Maybe

> relevant a decade ago, but probably not now.

>

> And if the card is completely disfunctional in said configuration, the

> default is definitely wrong.


The card is indeed completely disfunctional. So we could try to
resurrect 353176888386 ("r8169: enable 64-bit DMA by default for PCI
Express devices"), and instead of backing it out again if regressions
are reported, blacklist the particular chips. This is a much better
approach, since then we can also print some kind of diagnostic on
those arm64 systems why such a blacklisted NIC is not supported.
Ard Biesheuvel May 12, 2016, 5:53 p.m. | #4
On 12 May 2016 at 16:09, Francois Romieu <romieu@fr.zoreil.com> wrote:
>> On 12 May 2016 at 01:58, David Miller <davem@davemloft.net> wrote:

>> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> > Date: Wed, 11 May 2016 09:47:49 +0200

> [...]

>> > I think we should just seriously consider changing the default, it's

>> > a really outdated reasoning behind the current default setting.  Maybe

>> > relevant a decade ago, but probably not now.

>> >

>> > And if the card is completely disfunctional in said configuration, the

>> > default is definitely wrong.

>>

>> The card is indeed completely disfunctional. So we could try to

>> resurrect 353176888386 ("r8169: enable 64-bit DMA by default for PCI

>> Express devices"), and instead of backing it out again if regressions

>> are reported, blacklist the particular chips. This is a much better

>> approach, since then we can also print some kind of diagnostic on

>> those arm64 systems why such a blacklisted NIC is not supported.

>

> I doubt there will be much *reporting* from broken systems that

> include plain old PCI realtek chipsets (r8169.c::RTL_CFG_0). Changing

> the default for those is imnsho asking for troubles without clear

> benefit (experimental evidence suggests that smsc etherpower II grows

> older more easily than plain pci 8169 :o/ ).

>


By resurrecting 353176888386, I mean the patch that changes the
default for PCI express devices, so I think we are in agreement here?

> I'd rather leave these alone and change the default for the PCI Express

> chipsets. Btw, while it does not seem to hurt, they should not need any

> CPlusCmd Dual Access Cycle tweak either. Realtek may establish it (Lin ?)

>

> A few news from the "pathologically better safe than sorry" squad:

> I have switched the default on a couple of non-critical production

> servers that include 8168c (RTL_GIGA_MAC_VER_22). It should give an hint

> for hardware from 2008 ~ 2009. I'll do some basic sanity testing with

> different chipsets.

>


Thanks for testing that. In the mean time, I will dust off the patch
and rebase it to today's -next.

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 94f08f1e841c..a49e8a58e539 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -345,7 +345,7 @@  static const struct pci_device_id rtl8169_pci_tbl[] = {
 MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
 
 static int rx_buf_sz = 16383;
-static int use_dac;
+static int use_dac = -1;
 static struct {
 	u32 msg_enable;
 } debug = { -1 };
@@ -859,7 +859,8 @@  struct rtl8169_private {
 MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
 MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
 module_param(use_dac, int, 0);
-MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot.");
+MODULE_PARM_DESC(use_dac,
+	"Enable PCI DAC. Unsafe on 32 bit PCI slot (default -1: enable on 64-bit archs only if needed");
 module_param_named(debug, debug.msg_enable, int, 0);
 MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 16=all)");
 MODULE_LICENSE("GPL");
@@ -8226,12 +8227,16 @@  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	tp->cp_cmd = 0;
 
+	/* Tentatively set the DMA mask to 32 bits. This may fail
+	 * on 64-bit architectures without any RAM below 4 GB.
+	 */
+	rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
 	if ((sizeof(dma_addr_t) > 4) &&
-	    !pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) && use_dac) {
+	    (use_dac == 1 || (use_dac == -1 && rc < 0)) &&
+	    !pci_set_dma_mask(pdev, DMA_BIT_MASK(64))) {
 		tp->cp_cmd |= PCIDAC;
 		dev->features |= NETIF_F_HIGHDMA;
 	} else {
-		rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
 		if (rc < 0) {
 			netif_err(tp, probe, dev, "DMA configuration failed\n");
 			goto err_out_free_res_3;