| Message ID | 1462952869-23498-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 ? [...] > 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.
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?
Ard Biesheuvel <ard.biesheuvel@linaro.org> : > On 11 May 2016 at 22:31, Francois Romieu <romieu@fr.zoreil.com> wrote: [...] > 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. The lack of IOMMU surprizes me. [...] > OK, if you prefer. Should I send a v2? Don't bother unless someones comes with a more substantial request.
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 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/ ). 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.
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.
Ard Biesheuvel <ard.biesheuvel@linaro.org> : > On 12 May 2016 at 16:09, Francois Romieu <romieu@fr.zoreil.com> wrote: [...] > By resurrecting 353176888386, I mean the patch that changes the > default for PCI express devices, so I think we are in agreement here ? Almost 353176888386: please modify it so that there's no change of behavior for mac_version < RTL_GIGA_MAC_VER_18. It will avoid a bit more than the plain old PCI chipsets, including the one that caused 353176888386 reversal as well as a few one that I don't feel confortable with due to their vicinity with the original 8169. [...] > > 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. 8106e (RTL_GIGA_MAC_VER_39) doesn't exhibit any difference of behavior either.
> -----Original Message----- > From: Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Thursday, May 12, 2016 10:09 PM > To: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: David Miller <davem@davemloft.net>; netdev@vger.kernel.org; Ricardo > Salveti <ricardo.salveti@linaro.org>; Leo Duran <leo.duran@amd.com>; G > Gregory <graeme.gregory@linaro.org>; nic_swsd <nic_swsd@realtek.com>; > Hau <hau@realtek.com> > Subject: Re: [PATCH] r8169: default to 64-bit DMA on systems without memory > below 4 GB > > > 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/ ). > > 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 ?) You don't have to set CPlusCmd Dual Access Cycle for the PCI Express chipsets. ------Please consider the environment before printing this e-mail.
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(-)