dmaengine.c: question about device_alloc_chan_resources

dmaengine.c: question about device_alloc_chan_resources

[Timur Tabi]

Dan, Haavard, et al,

I am making some fixes to the Freescale DMA driver (drivers/dma/fsldma.c), and
I've come across a situation I don't understand.  Specifically, I have an issue
with the return values from fsl_dma_alloc_chan_resources(), which is fsldma's
device_alloc_chan_resources() function.

fsldma calls dma_async_device_register() for each DMA controller it finds.  The
problem is that when I use the dmatest driver, this results in
device_alloc_chan_resources() being called multiple times for the same DMA
channel.  When this happens, fsl_dma_alloc_chan_resources() must return -1
otherwise fsldma will hang during unload if the dmatest driver is also loaded.
The hang occurs in dma_async_device_unregister(), when it calls
wait_for_completion().

Here's what happens:

1) fsldma finds one DMA controller with four DMA channels.  It creates all the
channel objects and then calls dma_async_device_register().

2) Via dma_clients_notify_available(), dmatest is notified that there's a new
DMA resource.  It stores those four channels in its dmatest_channels object.

3) fsldma fins another DMA controller with four DMA channels.  Again, it creates
all the channel objects and then calls dma_async_device_register().

4) dma_clients_notify_available() is called again.  This time, however, it tells
dmatest about *eight* channels, not just the four additional ones.  It does this
because fsl_dma_alloc_chan_resources() returns 1 every time it's called.

The result is that the some DMA channel will appear twice in dmatest's channel
list.  For example, I added a bunch of printks to dma_test_add_channnel:

dmatest: Started 1 threads using dma2chan1
dmatest_add_channel:372 chan=df1f2a50
dmatest_add_channel:382 dtc->chan=df1f2898
dmatest_add_channel:382 dtc->chan=df1f2a50
dmatest_add_channel:382 dtc->chan=df1f2898
dmatest_add_channel:382 dtc->chan=df1f2a50

As you can see, df1f2898 appears twice, as does df1f2a50.

So my fix was to add this code to fsl_dma_alloc_chan_resources():

	/* Has this channel already been allocated? */
	if (fsl_chan->desc_pool)
		return -ENOMEM;

This fixes the hang, but I have a suspicion that it's still wrong.  When I look
at the other DMA device drivers, they don't do this.  For instance,
ioat_dma_alloc_chan_resources() does this:

	/* have we already been set up? */
	if (!list_empty(&ioat_chan->free_desc))
		return ioat_chan->desccount;

In other words, it returns the resources that have been allocated already.  But
if I do that, my driver hangs during an unload (if dmatest is loaded).

I think what's happening is that fsldma is the only DMA driver that calls
dma_async_device_register() more than once, and so it's exposing a bug in
dmaengine.c.  Can anyone confirm that?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: dmaengine.c: question about device_alloc_chan_resources

[Dan Williams]

On Tue, Sep 16, 2008 at 2:30 PM, Timur Tabi <timur@freescale.com> wrote:
> Dan, Haavard, et al,
>
> I am making some fixes to the Freescale DMA driver (drivers/dma/fsldma.c), and
> I've come across a situation I don't understand.  Specifically, I have an issue
> with the return values from fsl_dma_alloc_chan_resources(), which is fsldma's
> device_alloc_chan_resources() function.
>
> fsldma calls dma_async_device_register() for each DMA controller it finds.  The
> problem is that when I use the dmatest driver, this results in
> device_alloc_chan_resources() being called multiple times for the same DMA
> channel.  When this happens, fsl_dma_alloc_chan_resources() must return -1
> otherwise fsldma will hang during unload if the dmatest driver is also loaded.
> The hang occurs in dma_async_device_unregister(), when it calls
> wait_for_completion().
>
> Here's what happens:
>
> 1) fsldma finds one DMA controller with four DMA channels.  It creates all the
> channel objects and then calls dma_async_device_register().
>
> 2) Via dma_clients_notify_available(), dmatest is notified that there's a new
> DMA resource.  It stores those four channels in its dmatest_channels object.
>
> 3) fsldma fins another DMA controller with four DMA channels.  Again, it creates
> all the channel objects and then calls dma_async_device_register().
>
> 4) dma_clients_notify_available() is called again.  This time, however, it tells
> dmatest about *eight* channels, not just the four additional ones.  It does this
> because fsl_dma_alloc_chan_resources() returns 1 every time it's called.
>
> The result is that the some DMA channel will appear twice in dmatest's channel
> list.  For example, I added a bunch of printks to dma_test_add_channnel:
>

I believe the problem is that dmatest is telling the core to take
multiple references on a channel.  Take a look at
net/core/dev.c:netdev_dma_event and
crypto:asynx_tx/async_tx.c:dma_channel_add_remove both of those check
to see if they already have a refence to the channel.

> dmatest: Started 1 threads using dma2chan1
> dmatest_add_channel:372 chan=df1f2a50
> dmatest_add_channel:382 dtc->chan=df1f2898
> dmatest_add_channel:382 dtc->chan=df1f2a50

^...right here we should be returning DMA_DUP rather than DMA_ACK.

> dmatest_add_channel:382 dtc->chan=df1f2898

--
Dan

Re: dmaengine.c: question about device_alloc_chan_resources

[Timur Tabi]

On Wed, Sep 17, 2008 at 5:36 PM, Dan Williams <dan.j.williams@intel.com> wrote:

> I believe the problem is that dmatest is telling the core to take
> multiple references on a channel.  Take a look at
> net/core/dev.c:netdev_dma_event and
> crypto:asynx_tx/async_tx.c:dma_channel_add_remove both of those check
> to see if they already have a refence to the channel.

Ok, I see what these functions are doing, and dmatest_add_channel() is
not doing it.  dmatest_add_channel() should not assume that it will
only receive one and only one DMA_RESOURCE_AVAILABLE for each channel.

I'll post a patch, if Haavard doesn't do it first.  Thanks for looking
into this and getting back to me.  I've spent the entire week
debugging this.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: dmaengine.c: question about device_alloc_chan_resources

[Haavard Skinnemoen]

"Timur Tabi" <timur@freescale.com> wrote:
> On Wed, Sep 17, 2008 at 5:36 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > I believe the problem is that dmatest is telling the core to take
> > multiple references on a channel.  Take a look at
> > net/core/dev.c:netdev_dma_event and
> > crypto:asynx_tx/async_tx.c:dma_channel_add_remove both of those check
> > to see if they already have a refence to the channel.  
> 
> Ok, I see what these functions are doing, and dmatest_add_channel() is
> not doing it.  dmatest_add_channel() should not assume that it will
> only receive one and only one DMA_RESOURCE_AVAILABLE for each channel.

Wouldn't it be better if the dmaengine layer made sure it didn't pass
the same channel several times to a client?

I mean, you seem concerned that the memcpy() API should be transparent
and easy to use, but the whole registration interface is just
ridiculously complicated...

> I'll post a patch, if Haavard doesn't do it first.  Thanks for looking
> into this and getting back to me.  I've spent the entire week
> debugging this.

I'm sorry I didn't respond earlier, but I'm having difficulties
figuring out what's going on during registration/unregistration
myself...

Haavard

Re: dmaengine.c: question about device_alloc_chan_resources

[Timur Tabi]

Haavard Skinnemoen wrote:

> Wouldn't it be better if the dmaengine layer made sure it didn't pass
> the same channel several times to a client?

Wouldn't that require it to keep track of which clients have already seen which
channels?  It might make more sense, but it's probably easier the current way.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: dmaengine.c: question about device_alloc_chan_resources

[Haavard Skinnemoen]

Timur Tabi <timur@freescale.com> wrote:
> Haavard Skinnemoen wrote:
> 
> > Wouldn't it be better if the dmaengine layer made sure it didn't pass
> > the same channel several times to a client?  
> 
> Wouldn't that require it to keep track of which clients have already seen which
> channels?  It might make more sense, but it's probably easier the current way.

I guess so. What would be even more simple is to simply prevent other
clients from taking a channel once someone has acked it, which would be
perfect for my purposes, but perhaps not everyone else's...

How common is for several clients to use the same channel anyway
though? Wouldn't you risk stalling the network stack if the md subsystem
has submitted a large xor operation on the same channel that the
network stack was about to use?

Haavard

Re: dmaengine.c: question about device_alloc_chan_resources

[Timur Tabi]

Haavard Skinnemoen wrote:

> I guess so. What would be even more simple is to simply prevent other
> clients from taking a channel once someone has acked it, which would be
> perfect for my purposes, but perhaps not everyone else's...

I'm not clear on how your driver works, but doesn't it run continuous tests on
all channels all the time?  If so, wouldn't your proposal prevent other drivers
from ever getting a DMA channel?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: dmaengine.c: question about device_alloc_chan_resources

[Haavard Skinnemoen]

Timur Tabi <timur@freescale.com> wrote:
> Haavard Skinnemoen wrote:
> 
> > I guess so. What would be even more simple is to simply prevent other
> > clients from taking a channel once someone has acked it, which would be
> > perfect for my purposes, but perhaps not everyone else's...  
> 
> I'm not clear on how your driver works, but doesn't it run continuous tests on
> all channels all the time?  If so, wouldn't your proposal prevent other drivers
> from ever getting a DMA channel?

You can explicitly tell it which channel(s) to use. If you run it on
all channels, it will already prevent any dmaslave clients from ever
getting a channel.

dmatest is meant for testing only, and has a major impact on system
performance when it's running, so it's IMO not a very interesting use
case.

Haavard

Re: dmaengine.c: question about device_alloc_chan_resources

[Dan Williams]

On Thu, Sep 18, 2008 at 7:45 AM, Haavard Skinnemoen
<haavard.skinnemoen@atmel.com> wrote:
> Timur Tabi <timur@freescale.com> wrote:
>> Haavard Skinnemoen wrote:
>>
>> > Wouldn't it be better if the dmaengine layer made sure it didn't pass
>> > the same channel several times to a client?
>>
>> Wouldn't that require it to keep track of which clients have already seen which
>> channels?  It might make more sense, but it's probably easier the current way.
>
> I guess so. What would be even more simple is to simply prevent other
> clients from taking a channel once someone has acked it, which would be
> perfect for my purposes, but perhaps not everyone else's...

Does not seem too bad to have something like a DMA_EXCL flag in
dma_chan to tell the core not to show this channel to any other
clients.

>
> How common is for several clients to use the same channel anyway
> though? Wouldn't you risk stalling the network stack if the md subsystem
> has submitted a large xor operation on the same channel that the
> network stack was about to use?

It's analogous to running the network stack on the same cpu as md.
Yes, they effect each other but the operations are short-lived and can
interleave.  One potential problem could be bouncing the channel lock
across multiple cpu's.  This is mitigated if net_dma is changed to use
async_memcpy(), but it's not a priority.  iop_adma and mv_xor live on
single cpu platforms where net_dma is already turned off due to the
expense of get_user_pages().

--
Dan

Re: dmaengine.c: question about device_alloc_chan_resources

[Haavard Skinnemoen]

"Dan Williams" <dan.j.williams@intel.com> wrote:
> On Thu, Sep 18, 2008 at 7:45 AM, Haavard Skinnemoen
> <haavard.skinnemoen@atmel.com> wrote:
> > Timur Tabi <timur@freescale.com> wrote:  
> >> Haavard Skinnemoen wrote:
> >>  
> >> > Wouldn't it be better if the dmaengine layer made sure it didn't pass
> >> > the same channel several times to a client?  
> >>
> >> Wouldn't that require it to keep track of which clients have already seen which
> >> channels?  It might make more sense, but it's probably easier the current way.  
> >
> > I guess so. What would be even more simple is to simply prevent other
> > clients from taking a channel once someone has acked it, which would be
> > perfect for my purposes, but perhaps not everyone else's...  
> 
> Does not seem too bad to have something like a DMA_EXCL flag in
> dma_chan to tell the core not to show this channel to any other
> clients.

Yes, or maybe provide an interface for simply requesting a channel
without having to register any callbacks. With some drivers, the number
of required channels is fixed, and not using DMA just isn't an option.
A simple "get a channel which satisfies these constraints" interface
would simplify these drivers a lot.

Haavard

Re: dmaengine.c: question about device_alloc_chan_resources

[Timur Tabi]

Haavard Skinnemoen wrote:

> Yes, or maybe provide an interface for simply requesting a channel
> without having to register any callbacks. 

I could use this feature.  The sound drivers for our MPC8610 processor use DMA,
but the drivers need to control the DMA hardware directly, so I can't use
dmaengine.  I would like to be able to just reserve the channels and program
them as I see fit.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: dmaengine.c: question about device_alloc_chan_resources

[linux-os (Dick Johnson)]

On Fri, 19 Sep 2008, Timur Tabi wrote:

> Haavard Skinnemoen wrote:
>
>> Yes, or maybe provide an interface for simply requesting a channel
>> without having to register any callbacks.
>
> I could use this feature.  The sound drivers for our MPC8610 processor use DMA,
> but the drivers need to control the DMA hardware directly, so I can't use
> dmaengine.  I would like to be able to just reserve the channels and program
> them as I see fit.
>
> -- 
> Timur Tabi
> Linux kernel developer at Freescale

The hardware on the PC/AT machines won't allow you to
reserve a specific channel and program it as you see fit,
because there are two devices, one for the low DMA
channels and another for the high ones. Even the low
DMA device has a channel in use as a cascade so, if
you were to touch any of its registers while a DMA
was in progress, the result will be undefined. That's
why there needs to be a central procedure to handle
this shared resource and why drivers need to interface
through it and not touch the hardware. Bus Master
DMA from PCI boards is an entirely different matter.
Nobody but the driver for the board is going to muck
with its DMA so it does not need to be protected as a
shared resource.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.25.17 on an i686 machine (4786.70 BogoMips).
My book : http://www.AbominableFirebug.com/
_


****************************************************************
The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: dmaengine.c: question about device_alloc_chan_resources

[Timur Tabi]

linux-os (Dick Johnson) wrote:
> On Fri, 19 Sep 2008, Timur Tabi wrote:
> 
>> Haavard Skinnemoen wrote:
>>
>>> Yes, or maybe provide an interface for simply requesting a channel
>>> without having to register any callbacks.
>> I could use this feature.  The sound drivers for our MPC8610 processor use DMA,
>> but the drivers need to control the DMA hardware directly, so I can't use
>> dmaengine.  I would like to be able to just reserve the channels and program
>> them as I see fit.
>>
>> -- 
>> Timur Tabi
>> Linux kernel developer at Freescale
> 
> The hardware on the PC/AT machines won't allow you to
> reserve a specific channel and program it as you see fit,
> because there are two devices, one for the low DMA
> channels and another for the high ones. 

True, but that restriction doesn't apply to all processors.  So we could still
provide the capability of reserving channels for those systems where it makes
sense.  This is the case for the DMA controller I'm working with.

My solution for now is to permanently reserve the channels I need.  This is safe
and probably okay, because there are 6 other channels still available.  However,
if there was a mechanism where I could dynamically reserve channels, I would use it.

> The information transmitted in this message is confidential and may be privileged. 

Really?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: dmaengine.c: question about device_alloc_chan_resources

[Alan Cox]

> > I could use this feature.  The sound drivers for our MPC8610 processor use DMA,
> > but the drivers need to control the DMA hardware directly, so I can't use
> > dmaengine.  I would like to be able to just reserve the channels and program
> > them as I see fit.


> The hardware on the PC/AT machines won't allow you to
> reserve a specific channel and program it as you see fit,

And since when have MPC8610 processors been used in PC/AT systems ?

PC type ISA DMA is a special (mostly PC specific) interface of its own.

Alan

Re: dmaengine.c: question about device_alloc_chan_resources

[Dan Williams]

On Fri, Sep 19, 2008 at 7:34 AM, Timur Tabi <timur@freescale.com> wrote:
> Haavard Skinnemoen wrote:
>
>> Yes, or maybe provide an interface for simply requesting a channel
>> without having to register any callbacks.
>
> I could use this feature.  The sound drivers for our MPC8610 processor use DMA,
> but the drivers need to control the DMA hardware directly, so I can't use
> dmaengine.  I would like to be able to just reserve the channels and program
> them as I see fit.
>

I think its a good idea especially since it would be best not to
needlessly proliferate client implementations with competing channel
allocation schemes.  However it would need to be more descriptive
than:

struct dma_chan *dma_request_channel(dma_cap_mask_t request_mask);

Why:
1/  What if the requester initializes before a dmaengine device has
been registered?  What if a device is never registered?
2/ What about platform specific concerns where dma_cap_mask_t is not
descriptive enough e.g. only one memcpy channel can address a certain
bus?  Currently a client implementation can have some intelligence to
return DMA_DUP for channels that do not have the platform capability.


At the very least clients should be allowed to set an 'exclusive' bit
to prevent the channel from leaking elsewhere.

--
Dan

Re: dmaengine.c: question about device_alloc_chan_resources

[Haavard Skinnemoen]

"Dan Williams" <dan.j.williams@intel.com> wrote:
> On Fri, Sep 19, 2008 at 7:34 AM, Timur Tabi <timur@freescale.com> wrote:
> > Haavard Skinnemoen wrote:
> >
> >> Yes, or maybe provide an interface for simply requesting a channel
> >> without having to register any callbacks.
> >
> > I could use this feature.  The sound drivers for our MPC8610 processor use DMA,
> > but the drivers need to control the DMA hardware directly, so I can't use
> > dmaengine.  I would like to be able to just reserve the channels and program
> > them as I see fit.
> >
> 
> I think its a good idea especially since it would be best not to
> needlessly proliferate client implementations with competing channel
> allocation schemes.  However it would need to be more descriptive
> than:
> 
> struct dma_chan *dma_request_channel(dma_cap_mask_t request_mask);

Yes. It would definitely require a struct dma_slave too, unless we
create a separate function for that.

> Why:
> 1/  What if the requester initializes before a dmaengine device has
> been registered?  What if a device is never registered?

It should just return NULL if it cannot satisfy the request. That might
cause problems since the requesting module normally won't depend on the
dmaengine driver, but I think it's better than requiring drivers to
deal with channels that may come and go at any time. Those drivers will
inevitably be buggy.

> 2/ What about platform specific concerns where dma_cap_mask_t is not
> descriptive enough e.g. only one memcpy channel can address a certain
> bus?  Currently a client implementation can have some intelligence to
> return DMA_DUP for channels that do not have the platform capability.

Currently, clients requesting the DMA_SLAVE capability can specify
which particular DMA device they need. Would it make sense to allow
other clients to do that as well?

Also, struct dma_slave can be extended with
controller-/platform-specific fields. Maybe we need a similar mechanism
for passing platform-specific constraints when requesting "regular"
channels?

> At the very least clients should be allowed to set an 'exclusive' bit
> to prevent the channel from leaking elsewhere.

Agreed. Perhaps we should allow the DMA engine driver to set that flag
too, in case certain parameters make channels difficult to share on
certain controllers.

Haavard

Re: dmaengine.c: question about device_alloc_chan_resources

[Guennadi Liakhovetski]

On Sun, 21 Sep 2008, Haavard Skinnemoen wrote:

> > 2/ What about platform specific concerns where dma_cap_mask_t is not
> > descriptive enough e.g. only one memcpy channel can address a certain
> > bus?  Currently a client implementation can have some intelligence to
> > return DMA_DUP for channels that do not have the platform capability.
> 
> Currently, clients requesting the DMA_SLAVE capability can specify
> which particular DMA device they need. Would it make sense to allow
> other clients to do that as well?

This is how it is documented in dmaengine.h:

 * @slave: data for preparing slave transfer. Must be non-NULL iff the
 *  DMA_SLAVE capability is requested.

But, looking at dma_client_chan_alloc() it seems, any client requesting a 
channel can provide a slave and link it to a specific dma_dev, regardless 
what capabilities the client is requesting, or am I missing something? If 
so, then yes, please, let's allow all do this. Wouldn't it be better to 
move the .dma_dev member to dma_client?

> Also, struct dma_slave can be extended with
> controller-/platform-specific fields. Maybe we need a similar mechanism
> for passing platform-specific constraints when requesting "regular"
> channels?

As far as I understand, this extension can only be done by "wrapping" 
dma_slave with driver-specific data:

 * If dma_dev is non-NULL, the client can not be bound to other DMA
 * masters than the one corresponding to this device. The DMA master
 * driver may use this to determine if there is controller-specific
 * data wrapped around this struct. Drivers of platform code that sets
 * the dma_dev field must therefore make sure to use an appropriate
 * controller-specific dma slave structure wrapping this struct.

i.e., there is no "void *priv" or similar. So, the same "wrapping" can be 
used with dma_client, even more conveniently so, if we move .dma_dev into 
it.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: dmaengine.c: question about device_alloc_chan_resources

[Haavard Skinnemoen]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Sun, 21 Sep 2008, Haavard Skinnemoen wrote:
> 
> > > 2/ What about platform specific concerns where dma_cap_mask_t is not
> > > descriptive enough e.g. only one memcpy channel can address a certain
> > > bus?  Currently a client implementation can have some intelligence to
> > > return DMA_DUP for channels that do not have the platform capability.
> > 
> > Currently, clients requesting the DMA_SLAVE capability can specify
> > which particular DMA device they need. Would it make sense to allow
> > other clients to do that as well?
> 
> This is how it is documented in dmaengine.h:
> 
>  * @slave: data for preparing slave transfer. Must be non-NULL iff the
>  *  DMA_SLAVE capability is requested.
> 
> But, looking at dma_client_chan_alloc() it seems, any client requesting a 
> channel can provide a slave and link it to a specific dma_dev, regardless 
> what capabilities the client is requesting, or am I missing something? If 
> so, then yes, please, let's allow all do this. Wouldn't it be better to 
> move the .dma_dev member to dma_client?

Yes, that's basically what I'm suggesting.

> > Also, struct dma_slave can be extended with
> > controller-/platform-specific fields. Maybe we need a similar mechanism
> > for passing platform-specific constraints when requesting "regular"
> > channels?
> 
> As far as I understand, this extension can only be done by "wrapping" 
> dma_slave with driver-specific data:

Correct.

>  * If dma_dev is non-NULL, the client can not be bound to other DMA
>  * masters than the one corresponding to this device. The DMA master
>  * driver may use this to determine if there is controller-specific
>  * data wrapped around this struct. Drivers of platform code that sets
>  * the dma_dev field must therefore make sure to use an appropriate
>  * controller-specific dma slave structure wrapping this struct.
> 
> i.e., there is no "void *priv" or similar. So, the same "wrapping" can be 
> used with dma_client, even more conveniently so, if we move .dma_dev into 
> it.

Except that usually, struct dma_client is already embedded into a
client-specific struct. So you can't really extend it with
controller-specific data without tying the client to one specific DMA
engine at compile time.

Since struct dma_slave is accessed through a pointer, the client driver
can get a controller-specific struct through device.platform_data or
something similar.

We could probably solve this by adding a "void *controller_data" field
to struct dma_client though.

Haavard