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.