Prev: disabling kernel preemption from user space
Next: [PATCH 0/2] DMAENGINE: generic slave, generic status
From: Mark Brown on 25 Mar 2010 11:10
On Thu, Mar 25, 2010 at 04:03:01PM +0100, Linus Walleij wrote:
> Convert the device_is_tx_complete() operation on the
> DMA engine to a generic device_tx_status()operation which
> can return three states, DMA_TX_RUNNING, DMA_TX_COMPLETE,
> DMA_TX_PAUSED.

FWIW (I've never actually used DMAENGINE...)

Acked-by: Mark Brown <broonie(a)opensource.wolfsonmicro.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Guennadi Liakhovetski on 25 Mar 2010 17:00
On Thu, 25 Mar 2010, Linus Walleij wrote:

> Convert the device_is_tx_complete() operation on the
> DMA engine to a generic device_tx_status()operation which
> can return three states, DMA_TX_RUNNING, DMA_TX_COMPLETE,
> DMA_TX_PAUSED.
>
> Signed-off-by: Linus Walleij <linus.walleij(a)stericsson.com>
> Cc: Maciej Sosnowski <maciej.sosnowski(a)intel.com>
> Cc: Nicolas Ferre <nicolas.ferre(a)atmel.com>
> Cc: Pavel Machek <pavel(a)ucw.cz>
> Cc: Li Yang <leoli(a)freescale.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski(a)gmx.de>
> Cc: Paul Mundt <lethal(a)linux-sh.org>
> Cc: Ralf Baechle <ralf(a)linux-mips.org>
> Cc: Haavard Skinnemoen <haavard.skinnemoen(a)atmel.com>
> Cc: Magnus Damm <damm(a)opensource.se>
> Cc: Liam Girdwood <lrg(a)slimlogic.co.uk>
> Cc: Mark Brown <broonie(a)opensource.wolfsonmicro.com>
> Cc: Joe Perches <joe(a)perches.com>
> Cc: Roland Dreier <rdreier(a)cisco.com>
> ---

[snip]

General: you converted all drivers to the new .device_tx_status() API, but
since they don't implement "residue," you left it uninitialised
everywhere. Wouldn't it be better to set it to 0 or total length,
depending on the complete / not complete status?

> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 0731802..c9f2c67 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -40,11 +40,13 @@ typedef s32 dma_cookie_t;
> * enum dma_status - DMA transaction status
> * @DMA_SUCCESS: transaction completed successfully
> * @DMA_IN_PROGRESS: transaction not yet processed
> + * @DMA_PAUSED: transaction is paused
> * @DMA_ERROR: transaction failed
> */
> enum dma_status {
> DMA_SUCCESS,
> DMA_IN_PROGRESS,
> + DMA_PAUSED,
> DMA_ERROR,
> };
>
> @@ -249,6 +251,21 @@ struct dma_async_tx_descriptor {
> };
>
> /**
> + * struct dma_tx_state - filled in to report the status of
> + * a transfer.
> + * @last: last completed DMA cookie
> + * @used: last issued DMA cookie (i.e. the one in progress)
> + * @residue: the remaining number of bytes left to transmit
> + * on the selected transfer for states DMA_IN_PROGRESS and
> + * DMA_PAUSED if this is implemented in the driver, else 0
> + */
> +struct dma_tx_state {
> + dma_cookie_t last;
> + dma_cookie_t used;
> + u32 residue;

In the original proposal by Dan Williams the last member was "unsigned
long pos." I don't think, even on 64-bit systems anyone would kick off a >
4GB transfer, but who knows... And - I don't particularly like the name
"pos," but I do like the idea of returning bytes transfered better, than
bytes left. Can we change this?

> +};
> +
> +/**
> * struct dma_device - info on the entity supplying DMA services
> * @chancnt: how many DMA channels are supported
> * @privatecnt: how many DMA channels are requested by dma_request_channel
> @@ -276,7 +293,10 @@ struct dma_async_tx_descriptor {
> * @device_prep_slave_sg: prepares a slave dma operation
> * @device_control: manipulate all pending operations on a channel, returns
> * zero or error code
> - * @device_is_tx_complete: poll for transaction completion
> + * @device_tx_status: poll for transaction completion, the optional
> + * txstate parameter can be supplied with a pointer to get a
> + * struct with some transfer information, else the call will just

Maybe "with auxiliary transfer status information, otherwise..."

> + * return a simple status code
> * @device_issue_pending: push pending transactions to hardware
> */
> struct dma_device {
> @@ -329,9 +349,9 @@ struct dma_device {
> unsigned long flags);
> int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd cmd);
>
> - enum dma_status (*device_is_tx_complete)(struct dma_chan *chan,
> - dma_cookie_t cookie, dma_cookie_t *last,
> - dma_cookie_t *used);
> + enum dma_status (*device_tx_status)(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate);
> void (*device_issue_pending)(struct dma_chan *chan);
> };
>
> @@ -572,7 +592,13 @@ static inline void dma_async_issue_pending(struct dma_chan *chan)
> static inline enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
> dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
> {
> - return chan->device->device_is_tx_complete(chan, cookie, last, used);
> + struct dma_tx_state state;
> + enum dma_status status;
> +
> + status = chan->device->device_tx_status(chan, cookie, &state);
> + *last = state.last;
> + *used = state.used;

Both last and used can be NULL.

> + return status;
> }
>
> #define dma_async_memcpy_complete(chan, cookie, last, used)\
> --
> 1.6.3.3

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dan Williams on 25 Mar 2010 18:10
On Thu, Mar 25, 2010 at 1:58 PM, Guennadi Liakhovetski
<g.liakhovetski(a)gmx.de> wrote:
> On Thu, 25 Mar 2010, Linus Walleij wrote:
>
>> Convert the device_is_tx_complete() operation on the
>> DMA engine to a generic device_tx_status()operation which
>> can return three states, DMA_TX_RUNNING, DMA_TX_COMPLETE,
>> DMA_TX_PAUSED.
>>
[..]
> General: you converted all drivers to the new .device_tx_status() API, but
> since they don't implement "residue," you left it uninitialised
> everywhere. Wouldn't it be better to set it to 0 or total length,
> depending on the complete / not complete status?

Agree that it should not be uninitialized. At the same time I do not
want to require drivers that don't need it to go through the hassle of
looking up a byte count, so perhaps all but the drivers that want this
support can return a 'max byte count'??

>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 0731802..c9f2c67 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -40,11 +40,13 @@ typedef s32 dma_cookie_t;
>> � * enum dma_status - DMA transaction status
>> � * @DMA_SUCCESS: transaction completed successfully
>> � * @DMA_IN_PROGRESS: transaction not yet processed
>> + * @DMA_PAUSED: transaction is paused
>> � * @DMA_ERROR: transaction failed
>> � */
>> �enum dma_status {
>> � � � DMA_SUCCESS,
>> � � � DMA_IN_PROGRESS,
>> + � � DMA_PAUSED,
>> � � � DMA_ERROR,
>> �};
>>
>> @@ -249,6 +251,21 @@ struct dma_async_tx_descriptor {
>> �};
>>
>> �/**
>> + * struct dma_tx_state - filled in to report the status of
>> + * a transfer.
>> + * @last: last completed DMA cookie
>> + * @used: last issued DMA cookie (i.e. the one in progress)
>> + * @residue: the remaining number of bytes left to transmit
>> + * � on the selected transfer for states DMA_IN_PROGRESS and
>> + * � DMA_PAUSED if this is implemented in the driver, else 0
>> + */
>> +struct dma_tx_state {
>> + � � dma_cookie_t last;
>> + � � dma_cookie_t used;
>> + � � u32 residue;
>
> In the original proposal by Dan Williams the last member was "unsigned
> long pos." I don't think, even on 64-bit systems anyone would kick off a >
> 4GB transfer, but who knows... And - I don't particularly like the name
> "pos," but I do like the idea of returning bytes transfered better, than
> bytes left. Can we change this?

Things like copy_from_user() and the scsi subsystem return residue, so
only for consistency with other areas would I side with the "bytes
left" camp.

Ack to all your other comments.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Guennadi Liakhovetski on 25 Mar 2010 18:40
On Thu, 25 Mar 2010, Dan Williams wrote:

> On Thu, Mar 25, 2010 at 1:58 PM, Guennadi Liakhovetski
> <g.liakhovetski(a)gmx.de> wrote:
> > On Thu, 25 Mar 2010, Linus Walleij wrote:
> >
> >> Convert the device_is_tx_complete() operation on the
> >> DMA engine to a generic device_tx_status()operation which
> >> can return three states, DMA_TX_RUNNING, DMA_TX_COMPLETE,
> >> DMA_TX_PAUSED.
> >>
> [..]
> > General: you converted all drivers to the new .device_tx_status() API, but
> > since they don't implement "residue," you left it uninitialised
> > everywhere. Wouldn't it be better to set it to 0 or total length,
> > depending on the complete / not complete status?
>
> Agree that it should not be uninitialized. At the same time I do not
> want to require drivers that don't need it to go through the hassle of
> looking up a byte count, so perhaps all but the drivers that want this
> support can return a 'max byte count'??

Why not assign one of the two - 0 if the transfer is complete (DMA_SUCCESS
status returned) or whatever max count otherwise? Except I do think this
might confuse some users - seeing a residue larger than the total transfer
length...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 25 Mar 2010 19:20
2010/3/25 Guennadi Liakhovetski <g.liakhovetski(a)gmx.de>:

> General: you converted all drivers to the new .device_tx_status() API, but
> since they don't implement "residue," you left it uninitialised
> everywhere. Wouldn't it be better to set it to 0 or total length,
> depending on the complete / not complete status?

The total length is a bit hard to know without understanding all drivers
I'm afraid, but I can sure add txtstatus->residue = 0; everywhere,
I'll fix.

>> +struct dma_tx_state {
>> + � � dma_cookie_t last;
>> + � � dma_cookie_t used;
>> + � � u32 residue;
>
> In the original proposal by Dan Williams the last member was "unsigned
> long pos." I don't think, even on 64-bit systems anyone would kick off a >
> 4GB transfer, but who knows... And - I don't particularly like the name
> "pos," but I do like the idea of returning bytes transfered better, than
> bytes left. Can we change this?

As Dan says consistency with other systems suggests something
residue or bytes_left.

>> - * @device_is_tx_complete: poll for transaction completion
>> + * @device_tx_status: poll for transaction completion, the optional
>> + * � txstate parameter can be supplied with a pointer to get a
>> + * � struct with some transfer information, else the call will just
>
> Maybe "with auxiliary transfer status information, otherwise..."

OK

>> @@ -572,7 +592,13 @@ static inline void dma_async_issue_pending(struct dma_chan *chan)
>> �static inline enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
>> � � � dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
>> �{
>> - � � return chan->device->device_is_tx_complete(chan, cookie, last, used);
>> + � � struct dma_tx_state state;
>> + � � enum dma_status status;
>> +
>> + � � status = chan->device->device_tx_status(chan, cookie, &state);
>> + � � *last = state.last;
>> + � � *used = state.used;
>
> Both last and used can be NULL.

Good you spotted this! Thanks.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: disabling kernel preemption from user space
Next: [PATCH 0/2] DMAENGINE: generic slave, generic status