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

[Jim Quinlan]

Hi Anday,


On Tue, Aug 18, 2020 at 4:14 AM Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Mon, Aug 17, 2020 at 05:53:09PM -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).
>
> ...
>
> > +     if (dev) {
> > +             phys_addr_t paddr = PFN_PHYS(pfn);
> > +
>
> > +             pfn -= (dma_offset_from_phys_addr(dev, paddr) >> PAGE_SHIFT);
>
> PFN_DOWN() ?
Yep.
>
> > +     }
>
> ...
>
> > +             pfn += (dma_offset_from_dma_addr(dev, addr) >> PAGE_SHIFT);
>
> Ditto.
Yep.
>
>
> ...
>
> > +static inline u64 dma_offset_from_dma_addr(struct device *dev, dma_addr_t dma_addr)
> > +{
> > +     const struct bus_dma_region *m = dev->dma_range_map;
> > +
> > +     if (!m)
> > +             return 0;
> > +     for (; m->size; m++)
> > +             if (dma_addr >= m->dma_start && dma_addr - m->dma_start < m->size)
> > +                     return m->offset;
> > +     return 0;
> > +}
> > +
> > +static inline u64 dma_offset_from_phys_addr(struct device *dev, phys_addr_t paddr)
> > +{
> > +     const struct bus_dma_region *m = dev->dma_range_map;
> > +
> > +     if (!m)
> > +             return 0;
> > +     for (; m->size; m++)
> > +             if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
> > +                     return m->offset;
> > +     return 0;
> > +}
>
> Perhaps for these the form with one return 0 is easier to read
>
>         if (m) {
>                 for (; m->size; m++)
>                         if (paddr >= m->cpu_start && paddr - m->cpu_start < m->size)
>                                 return m->offset;
>         }
>         return 0;
>
> ?
I see what you are saying but I don't think there is enough difference
between the two to justify changing it.
>
> ...
>
> > +     if (mem->use_dev_dma_pfn_offset) {
> > +             u64 base_addr = (u64)mem->pfn_base << PAGE_SHIFT;
>
> PFN_PHYS() ?
Yep.

>
> > +
> > +             return base_addr - dma_offset_from_phys_addr(dev, base_addr);
> > +     }
>
> ...
>
> > + * It returns -ENOMEM if out of memory, 0 otherwise.
>
> This doesn't describe cases dev->dma_range_map != NULL and offset == 0.
Okay, I'll fix this.

>
> > +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 (!offset)
> > +             return 0;
> > +
> > +     if (dev->dma_range_map) {
> > +             dev_err(dev, "attempt to add DMA range to existing map\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     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;
> > +}
>
> ...
>
> > +void *dma_copy_dma_range_map(const struct bus_dma_region *map)
> > +{
> > +     int num_ranges;
> > +     struct bus_dma_region *new_map;
> > +     const struct bus_dma_region *r = map;
> > +
> > +     for (num_ranges = 0; r->size; num_ranges++)
> > +             r++;
>
> > +     new_map = kcalloc(num_ranges + 1, sizeof(*map), GFP_KERNEL);
> > +     if (new_map)
> > +             memcpy(new_map, map, sizeof(*map) * num_ranges);
>
> Looks like krealloc() on the first glance...
It's not.  We are making a distinct copy of the original, not resizing it.
>
> > +
> > +     return new_map;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks again,
Jim
>
>