From: Christoph Hellwig <hch@lst.de>
To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
Robin Murphy <robin.murphy@arm.com>,
Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
Date: Tue, 21 May 2019 15:15:03 +0200 [thread overview]
Message-ID: <20190521131503.GA5258@lst.de> (raw)
In-Reply-To: <20190521130436.bgt53bf7nshz62ip@shell.armlinux.org.uk>
On Tue, May 21, 2019 at 02:04:37PM +0100, Russell King - ARM Linux admin wrote:
> So how does the driver negotiation for >32bit addresses work if we don't
> fail for large masks?
>
> I'm thinking about all those PCI drivers that need DAC cycles for >32bit
> addresses, such as e1000, which negotiate via (eg):
>
> /* there is a workaround being applied below that limits
> * 64-bit DMA addresses to 64-bit hardware. There are some
> * 32-bit adapters that Tx hang when given 64-bit DMA addresses
> */
> pci_using_dac = 0;
> if ((hw->bus_type == e1000_bus_type_pcix) &&
> !dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
> pci_using_dac = 1;
> } else {
> err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> if (err) {
> pr_err("No usable DMA config, aborting\n");
> goto err_dma;
> }
> }
>
> and similar. If we blindly trunate the 64-bit to 32-bit, aren't we
> going to end up with PCI cards using DAC cycles to a host bridge that
> do not support DAC cycles?
In general PCI devices just use DAC cycles when they need it. I only
know of about a handful of devices that need to negotiate their
addressing mode, and those already use the proper API for that, which
is dma_get_required_mask.
The e1000 example is a good case of how the old API confused people.
First it only sets the 64-bit mask for devices which can support it,
which is good, but then it sets the NETIF_F_HIGHDMA flag only if we
set a 64-bit mask, which is completely unrelated to the DMA mask,
it just means the driver can handle sk_buff fragments that do not
have a kernel mapping, which really is a driver and not a hardware
issue.
So what this driver really should do is something like:
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 551de8c2fef2..d9236083da94 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -925,7 +925,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
static int cards_found;
static int global_quad_port_a; /* global ksp3 port a indication */
- int i, err, pci_using_dac;
+ int i, err;
u16 eeprom_data = 0;
u16 tmp = 0;
u16 eeprom_apme_mask = E1000_EEPROM_APME;
@@ -996,16 +996,11 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
* 64-bit DMA addresses to 64-bit hardware. There are some
* 32-bit adapters that Tx hang when given 64-bit DMA addresses
*/
- pci_using_dac = 0;
- if ((hw->bus_type == e1000_bus_type_pcix) &&
- !dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
- pci_using_dac = 1;
- } else {
- err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
- if (err) {
- pr_err("No usable DMA config, aborting\n");
- goto err_dma;
- }
+ err = dma_set_mask_and_coherent(&pdev->dev,
+ DMA_BIT_MASK(hw->bus_type == e1000_bus_type_pcix ? 64 : 32));
+ if (err) {
+ pr_err("No usable DMA config, aborting\n");
+ goto err_dma;
}
netdev->netdev_ops = &e1000_netdev_ops;
@@ -1047,19 +1042,15 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
netdev->priv_flags |= IFF_SUPP_NOFCS;
- netdev->features |= netdev->hw_features;
+ netdev->features |= netdev->hw_features | NETIF_F_HIGHDMA;
netdev->hw_features |= (NETIF_F_RXCSUM |
NETIF_F_RXALL |
NETIF_F_RXFCS);
- if (pci_using_dac) {
- netdev->features |= NETIF_F_HIGHDMA;
- netdev->vlan_features |= NETIF_F_HIGHDMA;
- }
-
netdev->vlan_features |= (NETIF_F_TSO |
NETIF_F_HW_CSUM |
- NETIF_F_SG);
+ NETIF_F_SG |
+ NETIF_F_HIGHDMA);
/* Do not set IFF_UNICAST_FLT for VMWare's 82545EM */
if (hw->device_id != E1000_DEV_ID_82545EM_COPPER ||
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: Christoph Hellwig <hch@lst.de>,
iommu@lists.linux-foundation.org,
Marek Szyprowski <m.szyprowski@samsung.com>,
Robin Murphy <robin.murphy@arm.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
Date: Tue, 21 May 2019 15:15:03 +0200 [thread overview]
Message-ID: <20190521131503.GA5258@lst.de> (raw)
In-Reply-To: <20190521130436.bgt53bf7nshz62ip@shell.armlinux.org.uk>
On Tue, May 21, 2019 at 02:04:37PM +0100, Russell King - ARM Linux admin wrote:
> So how does the driver negotiation for >32bit addresses work if we don't
> fail for large masks?
>
> I'm thinking about all those PCI drivers that need DAC cycles for >32bit
> addresses, such as e1000, which negotiate via (eg):
>
> /* there is a workaround being applied below that limits
> * 64-bit DMA addresses to 64-bit hardware. There are some
> * 32-bit adapters that Tx hang when given 64-bit DMA addresses
> */
> pci_using_dac = 0;
> if ((hw->bus_type == e1000_bus_type_pcix) &&
> !dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
> pci_using_dac = 1;
> } else {
> err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> if (err) {
> pr_err("No usable DMA config, aborting\n");
> goto err_dma;
> }
> }
>
> and similar. If we blindly trunate the 64-bit to 32-bit, aren't we
> going to end up with PCI cards using DAC cycles to a host bridge that
> do not support DAC cycles?
In general PCI devices just use DAC cycles when they need it. I only
know of about a handful of devices that need to negotiate their
addressing mode, and those already use the proper API for that, which
is dma_get_required_mask.
The e1000 example is a good case of how the old API confused people.
First it only sets the 64-bit mask for devices which can support it,
which is good, but then it sets the NETIF_F_HIGHDMA flag only if we
set a 64-bit mask, which is completely unrelated to the DMA mask,
it just means the driver can handle sk_buff fragments that do not
have a kernel mapping, which really is a driver and not a hardware
issue.
So what this driver really should do is something like:
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 551de8c2fef2..d9236083da94 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -925,7 +925,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
static int cards_found;
static int global_quad_port_a; /* global ksp3 port a indication */
- int i, err, pci_using_dac;
+ int i, err;
u16 eeprom_data = 0;
u16 tmp = 0;
u16 eeprom_apme_mask = E1000_EEPROM_APME;
@@ -996,16 +996,11 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
* 64-bit DMA addresses to 64-bit hardware. There are some
* 32-bit adapters that Tx hang when given 64-bit DMA addresses
*/
- pci_using_dac = 0;
- if ((hw->bus_type == e1000_bus_type_pcix) &&
- !dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
- pci_using_dac = 1;
- } else {
- err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
- if (err) {
- pr_err("No usable DMA config, aborting\n");
- goto err_dma;
- }
+ err = dma_set_mask_and_coherent(&pdev->dev,
+ DMA_BIT_MASK(hw->bus_type == e1000_bus_type_pcix ? 64 : 32));
+ if (err) {
+ pr_err("No usable DMA config, aborting\n");
+ goto err_dma;
}
netdev->netdev_ops = &e1000_netdev_ops;
@@ -1047,19 +1042,15 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
netdev->priv_flags |= IFF_SUPP_NOFCS;
- netdev->features |= netdev->hw_features;
+ netdev->features |= netdev->hw_features | NETIF_F_HIGHDMA;
netdev->hw_features |= (NETIF_F_RXCSUM |
NETIF_F_RXALL |
NETIF_F_RXFCS);
- if (pci_using_dac) {
- netdev->features |= NETIF_F_HIGHDMA;
- netdev->vlan_features |= NETIF_F_HIGHDMA;
- }
-
netdev->vlan_features |= (NETIF_F_TSO |
NETIF_F_HW_CSUM |
- NETIF_F_SG);
+ NETIF_F_SG |
+ NETIF_F_HIGHDMA);
/* Do not set IFF_UNICAST_FLT for VMWare's 82545EM */
if (hw->device_id != E1000_DEV_ID_82545EM_COPPER ||
next prev parent reply other threads:[~2019-05-21 13:15 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-21 12:47 fixups for the dma_set_mask beahvior change in 5.1 Christoph Hellwig
2019-05-21 12:47 ` Christoph Hellwig
2019-05-21 12:47 ` [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold Christoph Hellwig
2019-05-21 12:47 ` Christoph Hellwig
2019-05-21 13:04 ` Russell King - ARM Linux admin
2019-05-21 13:04 ` Russell King - ARM Linux admin
2019-05-21 13:15 ` Christoph Hellwig [this message]
2019-05-21 13:15 ` Christoph Hellwig
2019-05-29 12:22 ` Christoph Hellwig
2019-05-29 12:22 ` Christoph Hellwig
2019-06-14 7:46 ` Christoph Hellwig
2019-06-14 7:46 ` Christoph Hellwig
2019-06-25 5:54 ` Christoph Hellwig
2019-06-25 5:54 ` Christoph Hellwig
2019-05-21 12:47 ` [PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported Christoph Hellwig
2019-05-21 12:47 ` Christoph Hellwig
2019-05-21 13:00 ` Russell King - ARM Linux admin
2019-05-21 13:00 ` Russell King - ARM Linux admin
2019-05-21 13:04 ` Christoph Hellwig
2019-05-21 13:04 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190521131503.GA5258@lst.de \
--to=hch@lst.de \
--cc=iommu@lists.linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=robin.murphy@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.