Messages in this thread | | | Subject | RE: [PATCH] DMA: Fix broken device refcounting | Date | Mon, 29 Oct 2007 09:02:34 -0700 | From | "Nelson, Shannon" <> |
| |
>From: Williams, Dan J >On Sat, 2007-10-27 at 06:49 -0700, Haavard Skinnemoen wrote: >> On Fri, 26 Oct 2007 09:36:17 -0700 >> Dan Williams <dan.j.williams@intel.com> wrote: >> >> > @@ -221,7 +220,6 @@ void dma_chan_cleanup(struct kref *kref) >> > { >> > struct dma_chan *chan = container_of(kref, struct >dma_chan, refcount); >> > chan->device->device_free_chan_resources(chan); >> > - kref_put(&chan->device->refcount, dma_async_device_cleanup); >> > } >> > EXPORT_SYMBOL(dma_chan_cleanup); >> >> While I can't see any problems with the rest of the patch, I >think this >> part is wrong for the same reasons removing the kref_put() from the >> class device cleanup function is. I don't see any constraint that >> guarantees that dma_chan_cleanup() will always be called before >> dma_dev_release(), which means that "chan" may have been freed before >> this function gets a chance to run. Please correct me if I'm wrong. > >Absolutely right, the driver, not dmaengine, frees the memory so there >must be a per channel reference on the device to hold off the driver's >remove routine. >> >> Håvard > >So how about this... > >---snip--- >dmaengine: Fix broken device refcounting > >From: Haavard Skinnemoen <hskinnemoen@atmel.com> > >When a DMA device is unregistered, its reference count is decremented >twice for each channel: Once dma_class_dev_release() and once in >dma_chan_cleanup(). This may result in the DMA device driver's >remove() function completing before all channels have been cleaned >up, causing lots of use-after-free fun. > >Fix it by incrementing the device's reference count twice for each >channel during registration. > >Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com> >[dan.j.williams@intel.com: kill unnecessary client refcounting] >Signed-off-by: Dan Williams <dan.j.williams@intel.com> >--- > > drivers/dma/dmaengine.c | 17 ++++++----------- > 1 files changed, 6 insertions(+), 11 deletions(-) > >diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c >index 84257f7..ec7e871 100644 >--- a/drivers/dma/dmaengine.c >+++ b/drivers/dma/dmaengine.c >@@ -186,10 +186,9 @@ static void dma_client_chan_alloc(struct >dma_client *client) > /* we are done once this client rejects > * an available resource > */ >- if (ack == DMA_ACK) { >+ if (ack == DMA_ACK) > dma_chan_get(chan); >- kref_get(&device->refcount); >- } else if (ack == DMA_NAK) >+ else if (ack == DMA_NAK) > return; > } > } >@@ -276,11 +275,8 @@ static void >dma_clients_notify_removed(struct dma_chan *chan) > /* client was holding resources for this channel so > * free it > */ >- if (ack == DMA_ACK) { >+ if (ack == DMA_ACK) > dma_chan_put(chan); >- kref_put(&chan->device->refcount, >- dma_async_device_cleanup); >- } > } > > mutex_unlock(&dma_list_mutex); >@@ -320,11 +316,8 @@ void dma_async_client_unregister(struct >dma_client *client) > ack = client->event_callback(client, chan, > DMA_RESOURCE_REMOVED); > >- if (ack == DMA_ACK) { >+ if (ack == DMA_ACK) > dma_chan_put(chan); >- kref_put(&chan->device->refcount, >- dma_async_device_cleanup); >- } > } > > list_del(&client->global_node); >@@ -401,6 +394,8 @@ int dma_async_device_register(struct >dma_device *device) > goto err_out; > } > >+ /* One for the channel, one of the class device */ >+ kref_get(&device->refcount); > kref_get(&device->refcount); > kref_init(&chan->refcount); > chan->slow_ref = 0; > >
I tested this in my ioatdma setup and no longer get the panic. I'm good with this if you two are happy with it.
Signed-off-by: Shannon Nelson <shannon.nelson@intel.com>
sln - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |