[PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Jim Quinlan]

Hi Andy,


On Tue, Aug 25, 2020 at 5:54 AM Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Mon, Aug 24, 2020 at 03:30:20PM -0400, Jim Quinlan wrote:
> > The new field 'dma_range_map' in struct device is used to facilitate the
> > use of single or multiple offsets between mapping regions of cpu addrs and
> > dma addrs.  It subsumes the role of "dev->dma_pfn_offset" which was only
> > capable of holding a single uniform offset and had no region bounds
> > checking.
> >
> > The function of_dma_get_range() has been modified so that it takes a single
> > argument -- the device node -- and returns a map, NULL, or an error code.
> > The map is an array that holds the information regarding the DMA regions.
> > Each range entry contains the address offset, the cpu_start address, the
> > dma_start address, and the size of the region.
> >
> > of_dma_configure() is the typical manner to set range offsets but there are
> > a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
> > driver code.  These cases now invoke the function
> > dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
>
> ...
>
> > +     /*
> > +      * Record all info in the generic DMA ranges array for struct device.
> > +      */
> > +     *map = r;
> > +     for_each_of_range(&parser, &range) {
> > +             pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
> > +                      range.bus_addr, range.cpu_addr, range.size);
> > +             r->cpu_start = range.cpu_addr;
> > +             r->dma_start = range.bus_addr;
> > +             r->size = range.size;
>
> > +             r->offset = (u64)range.cpu_addr - (u64)range.bus_addr;
>
> What's the point in explicit castings to the same type?
No point.  If I have to send out another version I will fix this.

>
> > +             r++;
> > +     }
>
> ...
>
> > +             phys_addr_t     paddr;
> > +             dma_addr_t      dma_addr;
> > +             struct device   dev_bogus;
>
> >               unittest(paddr == expect_paddr,
> > -                      "of_dma_get_range wrong phys addr (%llx) on node %pOF", paddr, np);
> > +                      "of_dma_get_range: wrong phys addr %llx (expecting %llx) on node %pOF\n",
> > +                      (u64)paddr, expect_paddr, np);
>
> %llx -> %pap
This was intentional -- I'm aware of %pap and %pad.  The problem is
that %pa[pd]  print out a zero-filled 16 character number regardless
of what the number is.  For example, 1 is "0x0000000000000001",
whereas using %llx yields "1".

>
> >               unittest(dma_addr == expect_dma_addr,
> > -                      "of_dma_get_range wrong DMA addr (%llx) on node %pOF", dma_addr, np);
> > +                      "of_dma_get_range: wrong DMA addr %llx (expecting %llx) on node %pOF\n",
> > +                      (u64)dma_addr, expect_dma_addr, np);
>
> %llx -> %pad
>
> ...
>
> > +     if (mem->use_dev_dma_pfn_offset) {
> > +             u64 base_addr = PFN_PHYS((u64)mem->pfn_base);
>
> Do we need explicit casting here?
I don't think it is needed.  However, the "(u64)" is useless though
since the macro recasts it to a phys_addr_t.

If there is another version of this submission I will change this.
>
> > +
> > +             return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> > +     }
>
> ...
>
> > +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> > +                      dma_addr_t dma_start, u64 size)
> > +{
> > +     struct bus_dma_region *map;
> > +     u64 offset = (u64)cpu_start - (u64)dma_start;
> > +
> > +     if (dev->dma_range_map) {
> > +             dev_err(dev, "attempt to add DMA range to existing map\n");
> > +             return -EINVAL;
> > +     }
>
> Wouldn't be better to do an assignment of offset here?
IIRC this was what Christoph requested.  It has actually gone back and
forth over the versions of this submission.
>
> > +     if (!offset)
> > +             return 0;
> > +
> > +     map = kcalloc(2, sizeof(*map), GFP_KERNEL);
> > +     if (!map)
> > +             return -ENOMEM;
> > +     map[0].cpu_start = cpu_start;
> > +     map[0].dma_start = dma_start;
> > +     map[0].offset = offset;
> > +     map[0].size = size;
> > +     dev->dma_range_map = map;
> > +
> > +     return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks again,
Jim
>
>