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