On Wed, Jul 29, 2020 at 2:19 AM Christoph Hellwig <hch@xxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
