Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

[Larry Finger]

On 6/11/19 1:05 AM, Christoph Hellwig wrote:
> On Mon, Jun 10, 2019 at 11:09:47AM -0500, Larry Finger wrote:
>
> What might be confusing in your output is that dev->dma_mask is a pointer,
> and we are setting it in dma_set_mask.  That is before we only check
> if the pointer is set, and later we override it.  Of course this doesn't
> actually explain the failure.  But what is even more strange to me
> is that you get a return value from dma_supported() that isn't 0 or 1,
> as that function is supposed to return a boolean, and I really can't see
> how mask >= __phys_to_dma(dev, min_mask), would return anything but 0
> or 1.  Does the output change if you use the correct printk specifiers?
>
> i.e. with a debug patch like this:
>
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 2c2772e9702a..9e5b30b12b10 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -378,6 +378,7 @@ EXPORT_SYMBOL(dma_direct_map_resource);
>   int dma_direct_supported(struct device *dev, u64 mask)
>   {
>   	u64 min_mask;
> +	bool ret;
>   
>   	if (IS_ENABLED(CONFIG_ZONE_DMA))
>   		min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
> @@ -391,7 +392,12 @@ int dma_direct_supported(struct device *dev, u64 mask)
>   	 * use __phys_to_dma() here so that the SME encryption mask isn't
>   	 * part of the check.
>   	 */
> -	return mask >= __phys_to_dma(dev, min_mask);
> +	ret = (mask >= __phys_to_dma(dev, min_mask));
> +	if (!ret)
> +		dev_info(dev,
> +			"%s: failed (mask = 0x%llx, min_mask = 0x%llx/0x%llx, dma bits = %d\n",
> +			__func__, mask, min_mask, __phys_to_dma(dev, min_mask), ARCH_ZONE_DMA_BITS);
> +	return ret;
>   }
>   
>   size_t dma_direct_max_mapping_size(struct device *dev)
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index f7afdadb6770..6c57ccdee2ae 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -317,8 +317,14 @@ void arch_dma_set_mask(struct device *dev, u64 mask);
>   
>   int dma_set_mask(struct device *dev, u64 mask)
>   {
> -	if (!dev->dma_mask || !dma_supported(dev, mask))
> +	if (!dev->dma_mask) {
> +		dev_info(dev, "no DMA mask set!\n");
>   		return -EIO;
> +	}
> +	if (!dma_supported(dev, mask)) {
> +		printk("DMA not supported\n");
> +		return -EIO;
> +	}
>   
>   	arch_dma_set_mask(dev, mask);
>   	dma_check_mask(dev, mask);

After I got the correct formatting, the output with this patch only gives the 
following in dmesg:

b43-pci-bridge 0001:11:00.0: dma_direct_supported: failed (mask = 0x3fffffff, 
min_mask = 0x5ffff000/0x5ffff000, dma bits = 0x1f
DMA not supported
b43legacy-phy0 ERROR: The machine/kernel does not support the required 30-bit 
DMA mask

Your first patch did not work as the configuration does not have 
CONFIG_ZONE_DMA. As a result, the initial value of min_mask always starts at 32 
bits and is taken down to 31 with the maximum pfn minimization. When I forced 
the initial value of min_mask to 30 bits, the device worked.

It is obvious that the case of a mask smaller than min_mask should be handled by 
the IOMMU. In my system, CONFIG_IOMMU_SUPPORT is selected. All other CONFIG 
variables containing IOMMU are not selected. When dma_direct_supported() fails, 
should the system not try for an IOMMU solution? Is the driver asking for the 
wrong type of memory? It is doing a dma_and_set_mask_coherent() call.

Larry