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