| Message ID | 1462952869-23498-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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?
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.
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.
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.
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;
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