Received-SPF: pass (google.com: domain of linux-kernel+bounces-58503-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 8 Feb 2024 20:02:39 +0200
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Howard Yen <howardyen@google.com>
Cc: hch@lst.de, m.szyprowski@samsung.com, robin.murphy@arm.com,
	gregkh@linuxfoundation.org, rafael@kernel.org, broonie@kernel.org,
	james@equiv.tech, james.clark@arm.com, masahiroy@kernel.org,
	linux-kernel@vger.kernel.org, iommu@lists.linux.dev
Subject: Re: [PATCH v3] dma-coherent: add support for multi coherent rmems
 per dev
Message-ID: <ZcUXP14Ng8g5vw1j@smile.fi.intel.com>
References: <20240208152808.3712149-1-howardyen@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240208152808.3712149-1-howardyen@google.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Thu, Feb 08, 2024 at 03:28:05PM +0000, Howard Yen wrote:
> Add support for multiple coherent rmems per device. This patch replaces
> original dma_mem with dma_mems list in device structure to store multiple
> rmems.
> 
> These multiple rmems can be assigned to the device one by one by
> of_reserved_mem_device_init_by_idx() with the memory-region
> declaration in device tree as below and store the rmem to the dma_mems
> list.
> 
> 	device1@0 {
> 		...
> 		memory-region = <&reserved_mem0>, <&reserved_mem1>;
> 		...
> 	};
> 
> When driver tries to allocate memory from the rmems, looks for the first
> available rmem and allocates the memory from this rmem.
> 
> Then if driver removed, of_reserved_mem_device_release() needs to be
> invoked to release all the rmems assigned to the device.

..

>  	struct dma_coherent_mem *mem;
> -	int ret;
> +	int retval;
>  
>  	mem = dma_init_coherent_memory(phys_addr, device_addr, size, false);
>  	if (IS_ERR(mem))
>  		return PTR_ERR(mem);
>  
> -	ret = dma_assign_coherent_memory(dev, mem);
> -	if (ret)
> +	retval = dma_assign_coherent_memory(dev, mem);
> +	if (retval)
>  		_dma_release_coherent_memory(mem);
> -	return ret;
> +	return retval;

This is unrelated change.

But why? Do you have retval in the _existing_ code elsewhere?


..

>  int dma_alloc_from_dev_coherent(struct device *dev, ssize_t size,
>  		dma_addr_t *dma_handle, void **ret)
>  {
> -	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> +	struct dma_coherent_mem *mem_tmp;
>  
> -	if (!mem)
> +	if (list_empty(&dev->dma_mems))
>  		return 0;
>  
> -	*ret = __dma_alloc_from_coherent(dev, mem, size, dma_handle);
> +	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> +		*ret = __dma_alloc_from_coherent(dev, mem_tmp, size, dma_handle);
> +		if (*ret)
> +			break;

This bails out on the first success. Moreover, if one calls this function
again, it will rewrite the existing allocation. Is this all expected?

OTOH, if you add multiple entries and bailing out on error condition it should
be clear if the previous allocations have to be released.

> +	}

>  	return 1;

>  }

..

>  int dma_release_from_dev_coherent(struct device *dev, int order, void *vaddr)
>  {
> -	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> +	struct dma_coherent_mem *mem_tmp;
> +	int retval = 0;
> +
> +	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> +		retval = __dma_release_from_coherent(mem_tmp, order, vaddr);
> +		if (retval == 1)
> +			break;

Same Q here.

> +	}
>  
> -	return __dma_release_from_coherent(mem, order, vaddr);
> +	return retval;
>  }

..

>  int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma,
>  			   void *vaddr, size_t size, int *ret)
>  {
> -	struct dma_coherent_mem *mem = dev_get_coherent_memory(dev);
> +	struct dma_coherent_mem *mem_tmp;
> +	int retval = 0;
>  
> -	return __dma_mmap_from_coherent(mem, vma, vaddr, size, ret);
> +	list_for_each_entry(mem_tmp, &dev->dma_mems, node) {
> +		retval = __dma_mmap_from_coherent(mem_tmp, vma, vaddr, size, ret);
> +		if (retval == 1)
> +			break;

And here.

> +	}
> +
> +	return retval;
>  }

..

With the above Q in mind, here is another one: Why can't we allocate all at once?

-- 
With Best Regards,
Andy Shevchenko