lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 8 Feb 2024 20:02:39 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Howard Yen <howardyen@...gle.com>
Cc: hch@....de, m.szyprowski@...sung.com, robin.murphy@....com,
	gregkh@...uxfoundation.org, rafael@...nel.org, broonie@...nel.org,
	james@...iv.tech, james.clark@....com, masahiroy@...nel.org,
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Subject: Re: [PATCH v3] dma-coherent: add support for multi coherent rmems
 per dev

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



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ