| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Mar 2010 21:58:04 +0100 (CET)
From: Guennadi Liakhovetski <g.liakhovetski@....de>
To: Linus Walleij <linus.walleij@...ricsson.com>
cc: Dan Williams <dan.j.williams@...el.com>,
linux-kernel@...r.kernel.org,
Maciej Sosnowski <maciej.sosnowski@...el.com>,
Nicolas Ferre <nicolas.ferre@...el.com>,
Pavel Machek <pavel@....cz>, Li Yang <leoli@...escale.com>,
Paul Mundt <lethal@...ux-sh.org>,
Ralf Baechle <ralf@...ux-mips.org>,
Haavard Skinnemoen <haavard.skinnemoen@...el.com>,
Magnus Damm <damm@...nsource.se>,
Liam Girdwood <lrg@...mlogic.co.uk>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Joe Perches <joe@...ches.com>,
Roland Dreier <rdreier@...co.com>
Subject: Re: [PATCH 2/2] DMAENGINE: generic channel status
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@...ricsson.com>
> Cc: Maciej Sosnowski <maciej.sosnowski@...el.com>
> Cc: Nicolas Ferre <nicolas.ferre@...el.com>
> Cc: Pavel Machek <pavel@....cz>
> Cc: Li Yang <leoli@...escale.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@....de>
> Cc: Paul Mundt <lethal@...ux-sh.org>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> Cc: Haavard Skinnemoen <haavard.skinnemoen@...el.com>
> Cc: Magnus Damm <damm@...nsource.se>
> Cc: Liam Girdwood <lrg@...mlogic.co.uk>
> Cc: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Joe Perches <joe@...ches.com>
> Cc: Roland Dreier <rdreier@...co.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists