On Mon, May 10, 2021 at 11:03 PM Christoph Hellwig hch@lst.de wrote:

...

+#ifdef CONFIG_DMA_RESTRICTED_POOL +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_fdt.h> +#include <linux/of_reserved_mem.h> +#include <linux/slab.h> +#endif

I don't think any of this belongs into swiotlb.c. Marking swiotlb_init_io_tlb_mem non-static and having all this code in a separate file is probably a better idea.

Will do in the next version.

...

+#ifdef CONFIG_DMA_RESTRICTED_POOL +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,

•                             struct device *dev)
  

+{

• struct io_tlb_mem *mem = rmem->priv;
  

• unsigned long nslabs = rmem->size >> IO_TLB_SHIFT;
  

• if (dev->dma_io_tlb_mem)
  

•         return 0;
  

• /* Since multiple devices can share the same pool, the private data,
  

•  * io_tlb_mem struct, will be initialized by the first device attached
  

•  * to it.
  

•  */
  

This is not the normal kernel comment style.

Will fix this in the next version.

...

+#ifdef CONFIG_ARM

•         if (!PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))) {
  

•                 kfree(mem);
  

•                 return -EINVAL;
  

•         }
  

+#endif /* CONFIG_ARM */

And this is weird. Why would ARM have such a restriction? And if we have such rstrictions it absolutely belongs into an arch helper.

Now I think the CONFIG_ARM can just be removed? The goal here is to make sure we're using linear map and can safely use phys_to_dma/dma_to_phys.

...

•         swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, false);
  

•         rmem->priv = mem;
  

+#ifdef CONFIG_DEBUG_FS

•         if (!debugfs_dir)
  

•                 debugfs_dir = debugfs_create_dir("swiotlb", NULL);
  

•         swiotlb_create_debugfs(mem, rmem->name, debugfs_dir);
  

Doesn't the debugfs_create_dir belong into swiotlb_create_debugfs? Also please use IS_ENABLEd or a stub to avoid ifdefs like this.

Will move it into swiotlb_create_debugfs and use IS_ENABLED in the next version.