On Wed, Jul 29, 2020 at 12: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.
The lifetime for devm_kcalloc may not be what we want here. devm allocs are freed on probe fail or remove, not on freeing the device (there is a just in case free there too though).
Rob
On Wed, Jul 29, 2020 at 10:28 AM Rob Herring robh+dt@kernel.org wrote:
On Wed, Jul 29, 2020 at 12: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.
The lifetime for devm_kcalloc may not be what we want here. devm allocs are freed on probe fail or remove, not on freeing the device (there is a just in case free there too though).
What do you suggest doing as an alternative?
Jim
Rob
On Thu, 2020-07-30 at 12:44 -0400, Jim Quinlan wrote:
On Wed, Jul 29, 2020 at 10:28 AM Rob Herring robh+dt@kernel.org wrote:
On Wed, Jul 29, 2020 at 12: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.
The lifetime for devm_kcalloc may not be what we want here. devm allocs are freed on probe fail or remove, not on freeing the device (there is a just in case free there too though).
What do you suggest doing as an alternative?
I haven't given the idea much thought, but how about using a kref in the first element of your bus_dma_region array. It should be increased upon assigning it to a device and decreased it upon destroying it (triggering the free once it hits 0). It would take care of this memory leak, but also useful where you're sharing bus_dma_regions between devices.
Regards, Nicolas
On Thu, Jul 30, 2020 at 10:44 AM Jim Quinlan james.quinlan@broadcom.com wrote:
On Wed, Jul 29, 2020 at 10:28 AM Rob Herring robh+dt@kernel.org wrote:
On Wed, Jul 29, 2020 at 12: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.
The lifetime for devm_kcalloc may not be what we want here. devm allocs are freed on probe fail or remove, not on freeing the device (there is a just in case free there too though).
What do you suggest doing as an alternative?
I believe I gave it already. Put a kfree in the struct device release function. But you can't be passing the pointer from device to device if you go that route. You'd have to either copy the struct or check if it's the same as the parent's and skip the kfree in that case.
Rob
dri-devel@lists.freedesktop.org
-
Jim Quinlan -
Nicolas Saenz Julienne -
Rob Herring