Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset

[Jim Quinlan <james.quinlan@broadcom.com>]

Hi Christoph,

On Tue, Jul 28, 2020 at 8:33 AM Christoph Hellwig <hch@lst.de> wrote:
>
> A few tiny nitpicks:
>
> The subject should have the dma-mapping prefix, this doesn't
> really touch the device core.
>
> > -     rc = of_dma_get_range(np, &dma_addr, &paddr, &size);
> > +     rc = of_dma_get_range(np, &map);
> > +     rc = PTR_ERR_OR_ZERO(map);
>
> I don't think you need the PTR_ERR_OR_ZERO line here, of_dma_get_range
> returns the error.

Yes, that link needs to be deleted.

>
> > +int dma_set_offset_range(struct device *dev, phys_addr_t cpu_start,
> > +                      dma_addr_t dma_start, u64 size)hh
> > +{
> > +     struct bus_dma_region *map;
> > +     u64 offset = (u64)cpu_start - (u64)dma_start;
> > +
> > +     if (!dev)
> > +             return -ENODEV;
>
> I don't think we need the NULL protection here, all DMA API calls
> expect a device.
Yes, your review-patch removed it but left the comment about the
function returning -ENODEV.  So I wasn't sure to leave it in or not.
>
> > +     if (!offset)
> > +             return 0;
> > +
> > +     /*
> > +      * See if a map already exists and we already encompass the new range:
> > +      */
> > +     if (dev->dma_range_map) {
> > +             if (dma_range_overlaps(dev, cpu_start, dma_start, size, offset))
> > +                     return 0;
> > +             dev_err(dev, "attempt to add conflicting DMA range to existing map\n");
> > +             return -EINVAL;
> > +     }
>
> And here why do we need the overlap check at all?  I'd be tempted to
> always return an error for this case.
I believe the overlap check was your suggestion or at least in your
review-patch?  I'm fine with just returning an error.

>
> What is the plan to merge this?  Do you want all this to go into one
> tree, or get as many bits into the applicable trees for 5.9 and then
> finish up for 5.10?  If the former I can apply it to the dma-mapping
> tree and just fix up the nitpicks.
Whatever you think is best -- I would be quite happy if you could
accept at least the "dma_range_map" commit.   Of course I'd be most
happy if the entire patchset were accepted, but perhaps you can just
apply the  "dma_range_map" commit, and I will continue to bang away at
getting the N-1 PCIe-related commits ack'd and accepted.

Thanks much!
Jim Quinlan
Broadcom STB
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel