Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
On Fri, Jun 14, 2019 at 09:46:48AM +0200, Christoph Hellwig wrote: > If I don't hear anything back in the next days I will just merge > these patches, please comment. Applied to the dma-mapping for-next tree now.
Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
If I don't hear anything back in the next days I will just merge
these patches, please comment.
On Wed, May 29, 2019 at 02:22:19PM +0200, Christoph Hellwig wrote:
> Russell,
>
> any additional comments on this series?
>
> On Tue, May 21, 2019 at 03:15:03PM +0200, Christoph Hellwig wrote:
> > 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(>dev, DMA_BIT_MASK(64))) {
> > > pci_using_dac = 1;
> > > } else {
> > > err = dma_set_mask_and_coherent(>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(>dev, DMA_BIT_MASK(64))) {
> > - pci_using_dac = 1;
> > - } else {
> > - err = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
> > - if (err) {
> > - pr_err("No usable DMA config, aborting\n");
> > - goto err_dma;
> > - }
> > + err = dma_set_mask_and_coherent(>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 = _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 ||
> >
> ---end quoted text---
---end quoted text---
Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
Russell,
any additional comments on this series?
On Tue, May 21, 2019 at 03:15:03PM +0200, Christoph Hellwig wrote:
> 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(>dev, DMA_BIT_MASK(64))) {
> > pci_using_dac = 1;
> > } else {
> > err = dma_set_mask_and_coherent(>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(>dev, DMA_BIT_MASK(64))) {
> - pci_using_dac = 1;
> - } else {
> - err = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
> - if (err) {
> - pr_err("No usable DMA config, aborting\n");
> - goto err_dma;
> - }
> + err = dma_set_mask_and_coherent(>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 = _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 ||
>
---end quoted text---
Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
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(>dev, DMA_BIT_MASK(64))) {
> pci_using_dac = 1;
> } else {
> err = dma_set_mask_and_coherent(>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(>dev, DMA_BIT_MASK(64))) {
- pci_using_dac = 1;
- } else {
- err = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
- if (err) {
- pr_err("No usable DMA config, aborting\n");
- goto err_dma;
- }
+ err = dma_set_mask_and_coherent(>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 = _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
Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold
On Tue, May 21, 2019 at 02:47:28PM +0200, Christoph Hellwig wrote:
> The dma masks in struct device are always 64-bits wide. But for builds
> using a 32-bit dma_addr_t we need to ensure we don't store an
> unsupportable value. Before Linux 5.0 this was handled at least by
> the ARM dma mapping code by never allowing to set a larger dma_mask,
> but these days we allow the driver to just set the largest supported
> value and never fall back to a smaller one. Ensure this always works
> by truncating the value.
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(>dev, DMA_BIT_MASK(64))) {
pci_using_dac = 1;
} else {
err = dma_set_mask_and_coherent(>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?
>
> Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
> Signed-off-by: Christoph Hellwig
> ---
> kernel/dma/mapping.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index f7afdadb6770..1f628e7ac709 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -317,6 +317,12 @@ void arch_dma_set_mask(struct device *dev, u64 mask);
>
> int dma_set_mask(struct device *dev, u64 mask)
> {
> + /*
> + * Truncate the mask to the actually supported dma_addr_t width to
> + * avoid generating unsupportable addresses.
> + */
> + mask = (dma_addr_t)mask;
> +
> if (!dev->dma_mask || !dma_supported(dev, mask))
> return -EIO;
>
> @@ -330,6 +336,12 @@ EXPORT_SYMBOL(dma_set_mask);
> #ifndef CONFIG_ARCH_HAS_DMA_SET_COHERENT_MASK
> int dma_set_coherent_mask(struct device *dev, u64 mask)
> {
> + /*
> + * Truncate the mask to the actually supported dma_addr_t width to
> + * avoid generating unsupportable addresses.
> + */
> + mask = (dma_addr_t)mask;
> +
> if (!dma_supported(dev, mask))
> return -EIO;
>
> --
> 2.20.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
