On Tue, Jul 28, 2020 at 11:05 AM Rob Herring <robh+dt@xxxxxxxxxx> wrote: > > On Fri, Jul 24, 2020 at 2:45 PM Jim Quinlan <james.quinlan@xxxxxxxxxxxx> 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). > > > > Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx> > > --- > > [...] > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index 8eea3f6e29a4..4b718d199efe 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -918,33 +918,33 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index, > > } > > EXPORT_SYMBOL(of_io_request_and_map); > > > > +#ifdef CONFIG_HAS_DMA > > /** > > - * of_dma_get_range - Get DMA range info > > + * of_dma_get_range - Get DMA range info and put it into a map array > > * @np: device node to get DMA range info > > - * @dma_addr: pointer to store initial DMA address of DMA range > > - * @paddr: pointer to store initial CPU address of DMA range > > - * @size: pointer to store size of DMA range > > + * @map: dma range structure to return > > * > > * Look in bottom up direction for the first "dma-ranges" property > > - * and parse it. > > - * dma-ranges format: > > + * and parse it. Put the information into a DMA offset map array. > > + * > > + * dma-ranges format: > > * DMA addr (dma_addr) : naddr cells > > * CPU addr (phys_addr_t) : pna cells > > * size : nsize cells > > * > > - * It returns -ENODEV if "dma-ranges" property was not found > > - * for this device in DT. > > + * It returns -ENODEV if "dma-ranges" property was not found for this > > + * device in the DT. > > */ > > -int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size) > > +int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > > { > > struct device_node *node = of_node_get(np); > > const __be32 *ranges = NULL; > > - int len; > > - int ret = 0; > > bool found_dma_ranges = false; > > struct of_range_parser parser; > > struct of_range range; > > - u64 dma_start = U64_MAX, dma_end = 0, dma_offset = 0; > > + struct bus_dma_region *r; > > + int len, num_ranges = 0; > > + int ret; > > > > while (node) { > > ranges = of_get_property(node, "dma-ranges", &len); > > @@ -970,44 +970,35 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz > > } > > > > of_dma_range_parser_init(&parser, node); > > + for_each_of_range(&parser, &range) > > + num_ranges++; > > + > > + of_dma_range_parser_init(&parser, node); > > + > > + ret = -ENOMEM; > > + r = kcalloc(num_ranges + 1, sizeof(*r), GFP_KERNEL); > > + if (!r) > > + goto out; > > AFAICT, you have the error cases covered, but you are leaking memory > if the device is removed. Hi Rob, I started using devm_kcalloc() but at least two reviewers convinced me to just use kcalloc(). In addition, when I was using devm_kcalloc() it was awkward because 'dev' is not available to this function. It comes down to whether unbind/binding the device N times is actually a reasonable usage. As for my experience I've seen two cases: (1) my overnight "bind/unbind the PCIe RC driver" script, and we have a customer who does an unbind/bind as a hail mary to bring back life to their dead EP device. If the latter case happens repeatedly, there are bigger problems. > > > [...] >b > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 9f04c30c4aaf..49242dd6176e 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -519,7 +519,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc, > > /* Initialise vdev subdevice */ > > snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index); > > rvdev->dev.parent = &rproc->dev; > > - rvdev->dev.dma_pfn_offset = rproc->dev.parent->dma_pfn_offset; > > + rvdev->dev.dma_range_map = rproc->dev.parent->dma_range_map; > > But doing this means you can't just free the dma_range_map. You need > to do a copy here or you'd have to refcount it. Or I suppose you could > check if it the child has a different dma_range_map ptr than the > parent. I don't believe the code here attempts to free the dma_range_map or needs to. Assuming that we devm_kcalloc'd() the dev->dma_range_map -- which we are not currently doing -- my reasoning is that this device does not need to free anything since the dev->dma_range_map belongs to a device higher up in the bus hierarchy, and the lower device will be removed long before the higher device is removed and dev->dma_range_map goes away. Regards, Jim Regards, Jim > > > Rob _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel