Re: [PATCH v6 04/15] swiotlb: Add restricted DMA pool initialization

[Christoph Hellwig <hch@xxxxxx>]

> +#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.

> +#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.

> +#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.

> +             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.