>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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
>
>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 <[EMAIL PROTECTED]>
>[EMAIL PROTECTED]: kill unnecessary client refcounting]
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
>
> 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 <[EMAIL PROTECTED]>
sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
