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

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

On Wed, Jul 29, 2020 at 2:19 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Jul 28, 2020 at 02:24:51PM -0400, Jim Quinlan wrote:
> > 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.
>
> We can't just leak the allocations.  Do you have a pointer to the
> arguments against managed resources?  I'm generally not a huge fan
> of the managed resources, but for a case like this they actually seem
> useful.  If we don't use the managed resources we'll at leat need
> to explicitly free the resources when freeing the device.

Actually, there were no arguments for using an unmanaged kcalloc, just
comments [1], [2].  When it was rightly suggested to have 'dev'
dropped from of_dma_range() [3], I submitted v6 to accomplish this.
But now of_dma_range() would call kcalloc(), and then
of_dma_configure() was required to "dup" the result, requiring a
devm_kcalloc(), memcpy(), and kfree().  This was awkward, so
considering [1], [2], I dropped the devm_kcalloc() and submitted v7.

So I can easily revert to the awkward code of v6.  But I'm hoping you
have a more elegant solution :-)

Thanks,
Jim

[1]
[v6, Andy Shevchenko]
> +             /* We want the offset map to be device-managed, so alloc & copy */
> +             dev->dma_range_map = devm_kcalloc(dev, num_ranges + 1, sizeof(*r),
> +                                               GFP_KERNEL);
The question is how many times per device lifetime this can be called?

[2]
[v2, Andy Shevchenko]
> +     r = devm_kzalloc(dev, r_size, GFP_KERNEL);
devm_?!
Looking at r_size it should be rather kcalloc().

[3]
[v3, Nicolas Saenz Julienne]
> I agree with you.  The reason I needed a "struct device *"  in the call is
> because I wanted to make sure the memory that is alloc'd belongs to the
> device that needs it.  If I do a regular kzalloc(), this memory  will become
> a leak once someone starts unbinding/binding their device.  Also, in  all
> uses of of_dma_rtange() -- there is only one --  a dev is required as one
> can't attach an offset map to NULL.
>
> I do see that there are a number of functions in drivers/of/*.c that
> take 'struct device *dev' as an argument so there is precedent for
> something like this.  Regardless, I need an owner to the memory I
> alloc().
I understand the need for dev to be around, devm_*() is key. But also it's
important to keep the functions on purpose. And if of_dma_get_range() starts
setting ranges it calls, for the very least, for a function rename. Although
I'd rather split the parsing and setting of ranges as mentioned earlier. That
said, I get that's a more drastic move.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel