Re: [PATCH v3 09/13] device core: Introduce multiple dma pfn offsets

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Jun 03, 2020 at 03:20:41PM -0400, Jim Quinlan wrote:
> @@ -786,7 +787,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>  	const struct sun4i_backend_quirks *quirks;
>  	struct resource *res;
>  	void __iomem *regs;
> -	int i, ret;
> +	int i, ret = 0;

No need for this.

>  
>  	backend = devm_kzalloc(dev, sizeof(*backend), GFP_KERNEL);
>  	if (!backend)
> @@ -812,7 +813,9 @@ static int sun4i_backend_bind(struct device *dev, struct device *master,
>  		 * on our device since the RAM mapping is at 0 for the DMA bus,
>  		 * unlike the CPU.
>  		 */
> -		drm->dev->dma_pfn_offset = PHYS_PFN_OFFSET;
> +		ret = attach_uniform_dma_pfn_offset(dev, PHYS_PFN_OFFSET);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	backend->engine.node = dev->of_node;
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 04fbd4bf0ff9..e9cc1c2d47cd 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -754,7 +754,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg)
>  	if (cfg->oas > ARM_LPAE_MAX_ADDR_BITS)
>  		return NULL;
>  
> -	if (!selftest_running && cfg->iommu_dev->dma_pfn_offset) {
> +	if (!selftest_running && cfg->iommu_dev->dma_pfn_offset_map) {
>  		dev_err(cfg->iommu_dev, "Cannot accommodate DMA offset for IOMMU page tables\n");
>  		return NULL;
>  	}
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index eff34ded6305..7212da5e1076 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -183,7 +184,9 @@ static int sun4i_csi_probe(struct platform_device *pdev)
>  			return ret;
>  	} else {
>  #ifdef PHYS_PFN_OFFSET
> -		csi->dev->dma_pfn_offset = PHYS_PFN_OFFSET;
> +		ret = attach_uniform_dma_pfn_offset(dev, PHYS_PFN_OFFSET);
> +		if (ret)
> +			return ret;
>  #endif
>  	}
>  
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> index 055eb0b8e396..2d66d415b6c3 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -898,7 +898,10 @@ static int sun6i_csi_probe(struct platform_device *pdev)
>  
>  	sdev->dev = &pdev->dev;
>  	/* The DMA bus has the memory mapped at 0 */
> -	sdev->dev->dma_pfn_offset = PHYS_OFFSET >> PAGE_SHIFT;
> +	ret = attach_uniform_dma_pfn_offset(sdev->dev,
> +					    PHYS_OFFSET >> PAGE_SHIFT);
> +	if (ret)
> +		return ret;
>  
>  	ret = sun6i_csi_resource_request(sdev, pdev);
>  	if (ret)
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 96d8cfb14a60..c89333b0a5fb 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -918,6 +918,70 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index,
>  }
>  EXPORT_SYMBOL(of_io_request_and_map);
>  
> +static int attach_dma_pfn_offset_map(struct device *dev,
> +				     struct device_node *node, int num_ranges)
> +{
> +	struct of_range_parser parser;
> +	struct of_range range;
> +	struct dma_pfn_offset_region *r;
> +
> +	r = devm_kcalloc(dev, num_ranges + 1,
> +			 sizeof(struct dma_pfn_offset_region), GFP_KERNEL);
> +	if (!r)
> +		return -ENOMEM;
> +	dev->dma_pfn_offset_map = r;
> +	of_dma_range_parser_init(&parser, node);
> +
> +	/*
> +	 * Record all info for DMA ranges array.  We could
> +	 * just use the of_range struct, but if we did that it
> +	 * would require more calculations for phys_to_dma and
> +	 * dma_to_phys conversions.
> +	 */
> +	for_each_of_range(&parser, &range) {
> +		r->cpu_start = range.cpu_addr;
> +		r->cpu_end = r->cpu_start + range.size - 1;
> +		r->dma_start = range.bus_addr;
> +		r->dma_end = r->dma_start + range.size - 1;
> +		r->pfn_offset = PFN_DOWN(range.cpu_addr)
> +			- PFN_DOWN(range.bus_addr);
> +		r++;
> +	}
> +	return 0;
> +}
> +
> +
> +
> +/**
> + * attach_dma_pfn_offset - Assign scalar offset for all addresses.
> + * @dev:	device pointer; only needed for a corner case.
> + * @dma_pfn_offset:	offset to apply when converting from phys addr
      ^^^^^^^^^^^^^^^
This parameter name does not match.

> + *			to dma addr and vice versa.
> + *
> + * It returns -ENOMEM if out of memory, otherwise 0.

It can also return -ENODEV.  Why are we passing NULL dev pointers to
all these functions anyway?

> + */
> +int attach_uniform_dma_pfn_offset(struct device *dev, unsigned long pfn_offset)
> +{
> +	struct dma_pfn_offset_region *r;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	if (!pfn_offset)
> +		return 0;
> +
> +	r = devm_kcalloc(dev, 1, sizeof(struct dma_pfn_offset_region),
> +			 GFP_KERNEL);

Use:	r = devm_kzalloc(dev, sizeof(*r), GFP_KERNEL);


> +	if (!r)
> +		return -ENOMEM;
> +
> +	r->uniform_offset = true;
> +	r->pfn_offset = pfn_offset;
> +
> +	return 0;
> +}

This function doesn't seem to do anything useful.  Is part of it
missing?

> +EXPORT_SYMBOL_GPL(attach_uniform_dma_pfn_offset);
> +

regards,
dan carpenter

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