fixups for the dma_set_mask beahvior change in 5.1

fixups for the dma_set_mask beahvior change in 5.1

[Christoph Hellwig]

Hi all,

in 5.1 I fixed up the DMA mapping API to always accept larger than
required DMA mask.  Except that I forgot about a check in the arm
code that is not required any more, and about the case where a
architecture only supports a 32-bit dma mask, but could potentially
generate large addresses due to offsets for the DMA address.

These two patches should fix up these issues (which were only found by
code inspection).

fixups for the dma_set_mask beahvior change in 5.1

[Christoph Hellwig]

Hi all,

in 5.1 I fixed up the DMA mapping API to always accept larger than
required DMA mask.  Except that I forgot about a check in the arm
code that is not required any more, and about the case where a
architecture only supports a 32-bit dma mask, but could potentially
generate large addresses due to offsets for the DMA address.

These two patches should fix up these issues (which were only found by
code inspection).
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Christoph Hellwig]

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.

Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

[PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Christoph Hellwig]

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.

Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

_______________________________________________
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

[Russell King - ARM Linux admin]

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(&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?

> 
> Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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

Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Russell King - ARM Linux admin]

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(&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?

> 
> Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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
_______________________________________________
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

[Christoph Hellwig]

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 ||

Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Christoph Hellwig]

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

Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Christoph Hellwig]

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(&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 ||
> 
---end quoted text---

Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Christoph Hellwig]

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(&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 ||
> 
---end quoted text---
_______________________________________________
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

[Christoph Hellwig]

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(&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 ||
> > 
> ---end quoted text---
---end quoted text---

Re: [PATCH 1/2] dma-mapping: truncate dma masks to what dma_addr_t can hold

[Christoph Hellwig]

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(&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 ||
> > 
> ---end quoted text---
---end quoted text---
_______________________________________________
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

[Christoph Hellwig]

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

[Christoph Hellwig]

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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported

[Christoph Hellwig]

Since Linux 5.1 we allow drivers to just set the largest DMA mask they
support instead of falling back to smaller ones.

When fixing up all the dma ops instances to allow for this behavior
the arm direct mapping code was missed.  Fix it up by removing the
sanity check, as all the actual mapping code handles this case just
fine.

Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm/mm/dma-mapping.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 0a75058c11f3..bdf0d236aaee 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -219,25 +219,7 @@ EXPORT_SYMBOL(arm_coherent_dma_ops);
 
 static int __dma_supported(struct device *dev, u64 mask, bool warn)
 {
-	unsigned long max_dma_pfn;
-
-	/*
-	 * If the mask allows for more memory than we can address,
-	 * and we actually have that much memory, then we must
-	 * indicate that DMA to this device is not supported.
-	 */
-	if (sizeof(mask) != sizeof(dma_addr_t) &&
-	    mask > (dma_addr_t)~0 &&
-	    dma_to_pfn(dev, ~0) < max_pfn - 1) {
-		if (warn) {
-			dev_warn(dev, "Coherent DMA mask %#llx is larger than dma_addr_t allows\n",
-				 mask);
-			dev_warn(dev, "Driver did not use or check the return value from dma_set_coherent_mask()?\n");
-		}
-		return 0;
-	}
-
-	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
+	unsigned long max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
 
 	/*
 	 * Translate the device's DMA mask to a PFN limit.  This
-- 
2.20.1

[PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported

[Christoph Hellwig]

Since Linux 5.1 we allow drivers to just set the largest DMA mask they
support instead of falling back to smaller ones.

When fixing up all the dma ops instances to allow for this behavior
the arm direct mapping code was missed.  Fix it up by removing the
sanity check, as all the actual mapping code handles this case just
fine.

Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm/mm/dma-mapping.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 0a75058c11f3..bdf0d236aaee 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -219,25 +219,7 @@ EXPORT_SYMBOL(arm_coherent_dma_ops);
 
 static int __dma_supported(struct device *dev, u64 mask, bool warn)
 {
-	unsigned long max_dma_pfn;
-
-	/*
-	 * If the mask allows for more memory than we can address,
-	 * and we actually have that much memory, then we must
-	 * indicate that DMA to this device is not supported.
-	 */
-	if (sizeof(mask) != sizeof(dma_addr_t) &&
-	    mask > (dma_addr_t)~0 &&
-	    dma_to_pfn(dev, ~0) < max_pfn - 1) {
-		if (warn) {
-			dev_warn(dev, "Coherent DMA mask %#llx is larger than dma_addr_t allows\n",
-				 mask);
-			dev_warn(dev, "Driver did not use or check the return value from dma_set_coherent_mask()?\n");
-		}
-		return 0;
-	}
-
-	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
+	unsigned long max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
 
 	/*
 	 * Translate the device's DMA mask to a PFN limit.  This
-- 
2.20.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported

[Russell King - ARM Linux admin]

On Tue, May 21, 2019 at 02:47:29PM +0200, Christoph Hellwig wrote:
> Since Linux 5.1 we allow drivers to just set the largest DMA mask they
> support instead of falling back to smaller ones.

This doesn't make sense.  "they" is confusing - why would a driver set
a DMA mask larger than the driver supports?  Or is "they" not
referring to the drivers (in which case, what is it referring to?)

> When fixing up all the dma ops instances to allow for this behavior
> the arm direct mapping code was missed.  Fix it up by removing the
> sanity check, as all the actual mapping code handles this case just
> fine.
> 
> Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm/mm/dma-mapping.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 0a75058c11f3..bdf0d236aaee 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -219,25 +219,7 @@ EXPORT_SYMBOL(arm_coherent_dma_ops);
>  
>  static int __dma_supported(struct device *dev, u64 mask, bool warn)
>  {
> -	unsigned long max_dma_pfn;
> -
> -	/*
> -	 * If the mask allows for more memory than we can address,
> -	 * and we actually have that much memory, then we must
> -	 * indicate that DMA to this device is not supported.
> -	 */
> -	if (sizeof(mask) != sizeof(dma_addr_t) &&
> -	    mask > (dma_addr_t)~0 &&
> -	    dma_to_pfn(dev, ~0) < max_pfn - 1) {
> -		if (warn) {
> -			dev_warn(dev, "Coherent DMA mask %#llx is larger than dma_addr_t allows\n",
> -				 mask);
> -			dev_warn(dev, "Driver did not use or check the return value from dma_set_coherent_mask()?\n");
> -		}
> -		return 0;
> -	}

The point of this check is to trap the case where we have, for example,
8GB of memory, but dma_addr_t is 32-bit.  We can allocate in the high
4GB, but we can't represent the address in a dma_addr_t.

> -
> -	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
> +	unsigned long max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
>  
>  	/*
>  	 * Translate the device's DMA mask to a PFN limit.  This
> -- 
> 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

Re: [PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported

[Russell King - ARM Linux admin]

On Tue, May 21, 2019 at 02:47:29PM +0200, Christoph Hellwig wrote:
> Since Linux 5.1 we allow drivers to just set the largest DMA mask they
> support instead of falling back to smaller ones.

This doesn't make sense.  "they" is confusing - why would a driver set
a DMA mask larger than the driver supports?  Or is "they" not
referring to the drivers (in which case, what is it referring to?)

> When fixing up all the dma ops instances to allow for this behavior
> the arm direct mapping code was missed.  Fix it up by removing the
> sanity check, as all the actual mapping code handles this case just
> fine.
> 
> Fixes: 9eb9e96e97b3 ("Documentation/DMA-API-HOWTO: update dma_mask sections")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm/mm/dma-mapping.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 0a75058c11f3..bdf0d236aaee 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -219,25 +219,7 @@ EXPORT_SYMBOL(arm_coherent_dma_ops);
>  
>  static int __dma_supported(struct device *dev, u64 mask, bool warn)
>  {
> -	unsigned long max_dma_pfn;
> -
> -	/*
> -	 * If the mask allows for more memory than we can address,
> -	 * and we actually have that much memory, then we must
> -	 * indicate that DMA to this device is not supported.
> -	 */
> -	if (sizeof(mask) != sizeof(dma_addr_t) &&
> -	    mask > (dma_addr_t)~0 &&
> -	    dma_to_pfn(dev, ~0) < max_pfn - 1) {
> -		if (warn) {
> -			dev_warn(dev, "Coherent DMA mask %#llx is larger than dma_addr_t allows\n",
> -				 mask);
> -			dev_warn(dev, "Driver did not use or check the return value from dma_set_coherent_mask()?\n");
> -		}
> -		return 0;
> -	}

The point of this check is to trap the case where we have, for example,
8GB of memory, but dma_addr_t is 32-bit.  We can allocate in the high
4GB, but we can't represent the address in a dma_addr_t.

> -
> -	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
> +	unsigned long max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
>  
>  	/*
>  	 * Translate the device's DMA mask to a PFN limit.  This
> -- 
> 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported

[Christoph Hellwig]

On Tue, May 21, 2019 at 02:00:47PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 21, 2019 at 02:47:29PM +0200, Christoph Hellwig wrote:
> > Since Linux 5.1 we allow drivers to just set the largest DMA mask they
> > support instead of falling back to smaller ones.
> 
> This doesn't make sense.  "they" is confusing - why would a driver set
> a DMA mask larger than the driver supports?  Or is "they" not
> referring to the drivers (in which case, what is it referring to?)

The current plan is:

 - the driver sets whatever the device supports, and unless that mask
   is too small to be supportable it will always succeed

Which replaces the previous scheme of:

 - the driver tries to set whatever the device supports.  If there are
   addressing limitation outside the device (e.g. the PCIe root port,
   or the AXI interconnect) that will fail, and the device will set
   a 32-bit mask instead which it assumes will generally work.

> The point of this check is to trap the case where we have, for example,
> 8GB of memory, but dma_addr_t is 32-bit.  We can allocate in the high
> 4GB, but we can't represent the address in a dma_addr_t.

Yep, and that is what patch 1/2 should handle by truncating the
dma mask to something that can work.  I don't actually have hardware
I could test this scenario on, though.

Re: [PATCH 2/2] ARM: dma-mapping: allow larger DMA mask than supported

[Christoph Hellwig]

On Tue, May 21, 2019 at 02:00:47PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 21, 2019 at 02:47:29PM +0200, Christoph Hellwig wrote:
> > Since Linux 5.1 we allow drivers to just set the largest DMA mask they
> > support instead of falling back to smaller ones.
> 
> This doesn't make sense.  "they" is confusing - why would a driver set
> a DMA mask larger than the driver supports?  Or is "they" not
> referring to the drivers (in which case, what is it referring to?)

The current plan is:

 - the driver sets whatever the device supports, and unless that mask
   is too small to be supportable it will always succeed

Which replaces the previous scheme of:

 - the driver tries to set whatever the device supports.  If there are
   addressing limitation outside the device (e.g. the PCIe root port,
   or the AXI interconnect) that will fail, and the device will set
   a 32-bit mask instead which it assumes will generally work.

> The point of this check is to trap the case where we have, for example,
> 8GB of memory, but dma_addr_t is 32-bit.  We can allocate in the high
> 4GB, but we can't represent the address in a dma_addr_t.

Yep, and that is what patch 1/2 should handle by truncating the
dma mask to something that can work.  I don't actually have hardware
I could test this scenario on, though.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu