On Mon, May 10, 2021 at 11:03 PM Christoph Hellwig <hch@xxxxxx> 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.
