On Fri, 27 Feb 2009 00:24:36 +0900 (JST), Atsushi Nemoto <[email protected]> wrote:
> > Can you explain how reserved channels work? It looks like you are
> > working around the generic dma channel allocator, maybe it requires
> > updating to meet your needs.
...
> I need the reserved_chan to make channel 3 named "dma0chan3". If I
> can chose chan_id for each channels in dma_device, the reserved_chan
> is not needed.
So, how about this? If it was accepted, I can remove reserved_chan
from txx9dmac driver.
------------------------------------------------------
Subject: dmaengine: Use chan_id provided by DMA device driver
From: Atsushi Nemoto <[email protected]>
If chan_id was already given by the DMA device driver, use it.
Otherwise assign an incremental number for each channels.
This allows the DMA device driver to reserve some channel ID numbers.
Signed-off-by: Atsushi Nemoto <[email protected]>
---
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 280a9d2..a3679a7 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -609,6 +609,7 @@ EXPORT_SYMBOL(dmaengine_put);
int dma_async_device_register(struct dma_device *device)
{
int chancnt = 0, rc;
+ unsigned int chan_id = 0;
struct dma_chan* chan;
atomic_t *idr_ref;
@@ -663,7 +664,9 @@ int dma_async_device_register(struct dma_device *device)
continue;
}
- chan->chan_id = chancnt++;
+ if (!chan->chan_id)
+ chan->chan_id = chan_id++;
+ chancnt++;
chan->dev->device.class = &dma_devclass;
chan->dev->device.parent = device->dev;
chan->dev->chan = chan;
On Fri, 13 Mar 2009 01:19:50 +0900 (JST), Atsushi Nemoto <[email protected]> wrote:
> Subject: dmaengine: Use chan_id provided by DMA device driver
> From: Atsushi Nemoto <[email protected]>
>
> If chan_id was already given by the DMA device driver, use it.
> Otherwise assign an incremental number for each channels.
>
> This allows the DMA device driver to reserve some channel ID numbers.
...
> @@ -663,7 +664,9 @@ int dma_async_device_register(struct dma_device *device)
> continue;
> }
>
> - chan->chan_id = chancnt++;
> + if (!chan->chan_id)
> + chan->chan_id = chan_id++;
> + chancnt++;
> chan->dev->device.class = &dma_devclass;
> chan->dev->device.parent = device->dev;
> chan->dev->chan = chan;
This patch will fix another potential problem. Some driver, for
example ipu, assumes chan_id is an index of its internal array. But
dmaengine core does not guarantee it.
/* represent channels in sysfs. Probably want devs too */
list_for_each_entry(chan, &device->channels, device_node) {
chan->local = alloc_percpu(typeof(*chan->local));
if (chan->local == NULL)
continue;
chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
if (chan->dev == NULL) {
free_percpu(chan->local);
continue;
}
chan->chan_id = chancnt++;
...
}
device->chancnt = chancnt;
If alloc_percpu or kzalloc failed, chan_id does not match with its
position in device->channels list.
And above "continue" looks buggy anyway. Keeping incomplete channels
in device->channels list looks very dangerous...
---
Atsushi Nemoto
On Fri, 2009-03-13 at 07:16 -0700, Atsushi Nemoto wrote:
> On Fri, 13 Mar 2009 01:19:50 +0900 (JST), Atsushi Nemoto <[email protected]> wrote:
> > Subject: dmaengine: Use chan_id provided by DMA device driver
> > From: Atsushi Nemoto <[email protected]>
> >
> > If chan_id was already given by the DMA device driver, use it.
> > Otherwise assign an incremental number for each channels.
> >
> > This allows the DMA device driver to reserve some channel ID numbers.
> ...
> > @@ -663,7 +664,9 @@ int dma_async_device_register(struct dma_device *device)
> > continue;
> > }
> >
> > - chan->chan_id = chancnt++;
> > + if (!chan->chan_id)
> > + chan->chan_id = chan_id++;
> > + chancnt++;
> > chan->dev->device.class = &dma_devclass;
> > chan->dev->device.parent = device->dev;
> > chan->dev->chan = chan;
>
> This patch will fix another potential problem. Some driver, for
> example ipu, assumes chan_id is an index of its internal array. But
> dmaengine core does not guarantee it.
>
> /* represent channels in sysfs. Probably want devs too */
> list_for_each_entry(chan, &device->channels, device_node) {
> chan->local = alloc_percpu(typeof(*chan->local));
> if (chan->local == NULL)
> continue;
> chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
> if (chan->dev == NULL) {
> free_percpu(chan->local);
> continue;
> }
>
> chan->chan_id = chancnt++;
> ...
> }
> device->chancnt = chancnt;
>
> If alloc_percpu or kzalloc failed, chan_id does not match with its
> position in device->channels list.
>
>
> And above "continue" looks buggy anyway. Keeping incomplete channels
> in device->channels list looks very dangerous...
Yes it does. Here is the proposed fix:
----->
dmaengine: fail device registration if channel registration fails
From: Dan Williams <[email protected]>
Atsushi points out:
"If alloc_percpu or kzalloc failed, chan_id does not match with its
position in device->channels list.
And above "continue" looks buggy anyway. Keeping incomplete channels
in device->channels list looks very dangerous..."
Reported-by: Atsushi Nemoto <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
drivers/dma/dmaengine.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index a589930..fa14e8b 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -652,13 +652,15 @@ int dma_async_device_register(struct dma_device *device)
/* represent channels in sysfs. Probably want devs too */
list_for_each_entry(chan, &device->channels, device_node) {
+ rc = -ENOMEM;
chan->local = alloc_percpu(typeof(*chan->local));
if (chan->local == NULL)
- continue;
+ goto err_out;
chan->dev = kzalloc(sizeof(*chan->dev), GFP_KERNEL);
if (chan->dev == NULL) {
free_percpu(chan->local);
- continue;
+ chan->local = NULL;
+ goto err_out;
}
chan->chan_id = chancnt++;
@@ -675,6 +677,8 @@ int dma_async_device_register(struct dma_device *device)
if (rc) {
free_percpu(chan->local);
chan->local = NULL;
+ kfree(chan->dev);
+ atomic_dec(idr_ref);
goto err_out;
}
chan->client_count = 0;
On Mon, 16 Mar 2009 14:50:46 -0700, Dan Williams <[email protected]> wrote:
> > And above "continue" looks buggy anyway. Keeping incomplete channels
> > in device->channels list looks very dangerous...
>
> Yes it does. Here is the proposed fix:
> ----->
> dmaengine: fail device registration if channel registration fails
>
> From: Dan Williams <[email protected]>
>
> Atsushi points out:
> "If alloc_percpu or kzalloc failed, chan_id does not match with its
> position in device->channels list.
>
> And above "continue" looks buggy anyway. Keeping incomplete channels
> in device->channels list looks very dangerous..."
>
> Reported-by: Atsushi Nemoto <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Thanks, but it seems a hole sill exists. If alloc_percpu or kzalloc
for the first channel failed, when idr_ref will be freed ?
Hmm.. why idr_ref is dynamically allocated? Just putting it in
dma_device makes thing more simple, no?
---
Atsushi Nemoto
On Mon, Mar 16, 2009 at 7:20 PM, Atsushi Nemoto <[email protected]> wrote:
> On Mon, 16 Mar 2009 14:50:46 -0700, Dan Williams <[email protected]> wrote:
>> > And above "continue" looks buggy anyway. ?Keeping incomplete channels
>> > in device->channels list looks very dangerous...
>>
>> Yes it does. ?Here is the proposed fix:
>> ----->
>> dmaengine: fail device registration if channel registration fails
>>
>> From: Dan Williams <[email protected]>
>>
>> Atsushi points out:
>> "If alloc_percpu or kzalloc failed, chan_id does not match with its
>> position in device->channels list.
>>
>> And above "continue" looks buggy anyway. ?Keeping incomplete channels
>> in device->channels list looks very dangerous..."
>>
>> Reported-by: Atsushi Nemoto <[email protected]>
>> Signed-off-by: Dan Williams <[email protected]>
>
> Thanks, but it seems a hole sill exists. ?If alloc_percpu or kzalloc
> for the first channel failed, when idr_ref will be freed ?
>
True, we need a check like the following:
/* if we never registered a channel just release the idr */
if (atomic_read(idr_ref) == 0) {
mutex_lock(&dma_list_mutex);
idr_remove(&dma_idr, device->dev_id);
mutex_unlock(&dma_list_mutex);
kfree(idr_ref);
return rc;
}
> Hmm.. why idr_ref is dynamically allocated? ?Just putting it in
> dma_device makes thing more simple, no?
>
The sysfs device has a longer lifetime than dma_device. See commit
41d5e59c [1].
--
Dan
[1] http://git.kernel.org/?p=linux/kernel/git/djbw/async_tx.git;a=commitdiff;h=41d5e59c
> ---
> Atsushi Nemoto
> --
> 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/
>
On Mon, 16 Mar 2009 21:52:10 -0700, Dan Williams <[email protected]> wrote:
> > Hmm.. why idr_ref is dynamically allocated? ?Just putting it in
> > dma_device makes thing more simple, no?
>
> The sysfs device has a longer lifetime than dma_device. See commit
> 41d5e59c [1].
The sysfs device for dma_chan (dma_chan_dev) has a shorter lifetime
than dma_device, doesn't it?
I mean something like this (only compile tested):
------------------------------------------------------
Subject: dmaengine: Add idr_ref to dma_device
This fixes memory leak on some error path.
Signed-off-by: Atsushi Nemoto <[email protected]>
---
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 280a9d2..0708931 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -153,7 +153,6 @@ static void chan_dev_release(struct device *dev)
mutex_lock(&dma_list_mutex);
idr_remove(&dma_idr, chan_dev->dev_id);
mutex_unlock(&dma_list_mutex);
- kfree(chan_dev->idr_ref);
}
kfree(chan_dev);
}
@@ -610,7 +609,6 @@ int dma_async_device_register(struct dma_device *device)
{
int chancnt = 0, rc;
struct dma_chan* chan;
- atomic_t *idr_ref;
if (!device)
return -ENODEV;
@@ -637,10 +635,7 @@ int dma_async_device_register(struct dma_device *device)
BUG_ON(!device->device_issue_pending);
BUG_ON(!device->dev);
- idr_ref = kmalloc(sizeof(*idr_ref), GFP_KERNEL);
- if (!idr_ref)
- return -ENOMEM;
- atomic_set(idr_ref, 0);
+ atomic_set(&device->idr_ref, 0);
idr_retry:
if (!idr_pre_get(&dma_idr, GFP_KERNEL))
return -ENOMEM;
@@ -667,9 +662,9 @@ int dma_async_device_register(struct dma_device *device)
chan->dev->device.class = &dma_devclass;
chan->dev->device.parent = device->dev;
chan->dev->chan = chan;
- chan->dev->idr_ref = idr_ref;
+ chan->dev->idr_ref = &device->idr_ref;
chan->dev->dev_id = device->dev_id;
- atomic_inc(idr_ref);
+ atomic_inc(&device->idr_ref);
dev_set_name(&chan->dev->device, "dma%dchan%d",
device->dev_id, chan->chan_id);
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 1956c8d..9e99d82 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -234,6 +234,7 @@ struct dma_device {
int dev_id;
struct device *dev;
+ atomic_t idr_ref;
int (*device_alloc_chan_resources)(struct dma_chan *chan);
void (*device_free_chan_resources)(struct dma_chan *chan);
On Tue, Mar 17, 2009 at 9:09 AM, Atsushi Nemoto <[email protected]> wrote:
> On Mon, 16 Mar 2009 21:52:10 -0700, Dan Williams <[email protected]> wrote:
>> > Hmm.. why idr_ref is dynamically allocated? ?Just putting it in
>> > dma_device makes thing more simple, no?
>>
>> The sysfs device has a longer lifetime than dma_device. ?See commit
>> 41d5e59c [1].
>
> The sysfs device for dma_chan (dma_chan_dev) has a shorter lifetime
> than dma_device, doesn't it?
No, dma_async_device_unregister(), and the freeing of dma_device, may
finish before chan_dev_release is called. Userspace gates the final
release of dma_chan_dev objects.
On Tue, 17 Mar 2009 10:02:14 -0700, Dan Williams <[email protected]> wrote:
> >> The sysfs device has a longer lifetime than dma_device. ?See commit
> >> 41d5e59c [1].
> >
> > The sysfs device for dma_chan (dma_chan_dev) has a shorter lifetime
> > than dma_device, doesn't it?
>
> No, dma_async_device_unregister(), and the freeing of dma_device, may
> finish before chan_dev_release is called. Userspace gates the final
> release of dma_chan_dev objects.
You mean, if the sysfs device file was opened when
dma_async_device_unregister() was called, the sysfs device will not be
released until the sysfs device file is closed, right? If so I can
see.
BTW, there are another holes in dma_async_device_register. If
idr_pre_get or idr_get_new was failed, idr_ref will not be freed.
---
Atsushi Nemoto
On Tue, Mar 17, 2009 at 5:49 PM, Atsushi Nemoto <[email protected]> wrote:
> On Tue, 17 Mar 2009 10:02:14 -0700, Dan Williams <[email protected]> wrote:
> BTW, there are another holes in dma_async_device_register. ?If
> idr_pre_get or idr_get_new was failed, idr_ref will not be freed.
Thanks for these fixlets, I appreciate it.
Now, back to the issue at hand. Does your driver still need direct
control over chan->chan_id, or can it now rely on the fact that
dma_async_device_register() will fail if a channel is not initialized?
Or, just use some platform_data to identify the channel in the same
manner as atmel-mci?
Regards,
Dan
On Tue, 17 Mar 2009 18:23:46 -0700, Dan Williams <[email protected]> wrote:
> Now, back to the issue at hand. Does your driver still need direct
> control over chan->chan_id, or can it now rely on the fact that
> dma_async_device_register() will fail if a channel is not initialized?
> Or, just use some platform_data to identify the channel in the same
> manner as atmel-mci?
Yes, I still want to control chan->chan_id.
The atmel-mci does not select "channel". It just pick the first
usable channel of the dma_device specified by platform_data. I
suppose dw_dmac is symmetric (it can use any channel for any slave).
But TXx9 SoC DMAC channels are hardwired to each peripheral devices.
And I want to call Channel-3 of DMAC-0 "dma0chan3" even if Channel-2
was assigned to for public memcpy channel.
---
Atsushi Nemoto
On Tue, Mar 17, 2009 at 7:01 PM, Atsushi Nemoto <[email protected]> wrote:
> On Tue, 17 Mar 2009 18:23:46 -0700, Dan Williams <[email protected]> wrote:
>> ?Or, just use some platform_data to identify the channel in the same
>> manner as atmel-mci?
>
> Yes, I still want to control chan->chan_id.
>
> The atmel-mci does not select "channel". ?It just pick the first
> usable channel of the dma_device specified by platform_data. ?I
> suppose dw_dmac is symmetric (it can use any channel for any slave).
You are right, it does not hardwire the channel, but it does hardwire
the device, see at32_add_device_mci [1].
> But TXx9 SoC DMAC channels are hardwired to each peripheral devices.
I think creating a dma_device instance per channel and specifying that
device like atmel-mci is the more future-proof way to go.
> And I want to call Channel-3 of DMAC-0 "dma0chan3" even if Channel-2
> was assigned to for public memcpy channel.
The problem is you could pass in the chan_id to guarantee 'chan3', but
there is no guarantee that you will get 'dma0', as the driver has no
knowledge of what other dma devices may be in the system.
[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/avr32/mach-at32ap/at32ap700x.c;h=3fbfd1e32a9ee79af4f4545d95a9543b9070d189;hb=HEAD#l1327
On Wed, 18 Mar 2009 10:26:13 -0700, Dan Williams <[email protected]> wrote:
> > The atmel-mci does not select "channel". ?It just pick the first
> > usable channel of the dma_device specified by platform_data. ?I
> > suppose dw_dmac is symmetric (it can use any channel for any slave).
>
> You are right, it does not hardwire the channel, but it does hardwire
> the device, see at32_add_device_mci [1].
>
> > But TXx9 SoC DMAC channels are hardwired to each peripheral devices.
>
> I think creating a dma_device instance per channel and specifying that
> device like atmel-mci is the more future-proof way to go.
Well, I have considered it but it looks overkill for me at that time.
Maybe time to think again...
> > And I want to call Channel-3 of DMAC-0 "dma0chan3" even if Channel-2
> > was assigned to for public memcpy channel.
>
> The problem is you could pass in the chan_id to guarantee 'chan3', but
> there is no guarantee that you will get 'dma0', as the driver has no
> knowledge of what other dma devices may be in the system.
Yes, I do not expect 'dma0'. My filter function uses
dev_name(chan->device->dev), which is "txx9dmac.0" in this case.
Anyway, "one dma-device per channel" manner will make things much simpler.
---
Atsushi Nemoto
On Sat, 21 Mar 2009 21:29:20 +0900 (JST), Atsushi Nemoto <[email protected]> wrote:
> > I think creating a dma_device instance per channel and specifying that
> > device like atmel-mci is the more future-proof way to go.
>
> Well, I have considered it but it looks overkill for me at that time.
> Maybe time to think again...
>
> > > And I want to call Channel-3 of DMAC-0 "dma0chan3" even if Channel-2
> > > was assigned to for public memcpy channel.
> >
> > The problem is you could pass in the chan_id to guarantee 'chan3', but
> > there is no guarantee that you will get 'dma0', as the driver has no
> > knowledge of what other dma devices may be in the system.
>
> Yes, I do not expect 'dma0'. My filter function uses
> dev_name(chan->device->dev), which is "txx9dmac.0" in this case.
>
> Anyway, "one dma-device per channel" manner will make things much simpler.
Unfortunately, not so simple. If I created a dma_device for each
channel, all chan->chan_id will be 0 and all chan->device->dev points
same platform device. This makes client driver hard to select the
particular channel. Making a platform device for each channel will
solve this, but it looks wrong way to go for me.
So I will go back to multiple channels per device style, and try to
simplify "reservation". While all client should know correct channel
number, the reservation is just for detecting bad configuration.
---
Atsushi Nemoto
On Thu, Mar 26, 2009 at 7:12 AM, Atsushi Nemoto <[email protected]> wrote:
> On Sat, 21 Mar 2009 21:29:20 +0900 (JST), Atsushi Nemoto <[email protected]> wrote:
> Unfortunately, not so simple. ?If I created a dma_device for each
> channel, all chan->chan_id will be 0 and all chan->device->dev points
> same platform device. ?This makes client driver hard to select the
> particular channel. ?Making a platform device for each channel will
> solve this, but it looks wrong way to go for me.
Why? mv_xor, iop-adma, and ioat each have a platform or pci device
per channel so you would be in good company.
On Tue, 31 Mar 2009 12:34:59 -0700, Dan Williams <[email protected]> wrote:
> > Unfortunately, not so simple. ?If I created a dma_device for each
> > channel, all chan->chan_id will be 0 and all chan->device->dev points
> > same platform device. ?This makes client driver hard to select the
> > particular channel. ?Making a platform device for each channel will
> > solve this, but it looks wrong way to go for me.
>
> Why? mv_xor, iop-adma, and ioat each have a platform or pci device
> per channel so you would be in good company.
Hmm, indeed. I will try it. Thanks.
---
Atsushi Nemoto