* [PATCH] ASYNC_TX: async_xor mapping fix
@ 2008-12-08 19:14 Yuri Tikhonov
2008-12-08 21:08 ` Dan Williams
0 siblings, 1 reply; 4+ messages in thread
From: Yuri Tikhonov @ 2008-12-08 19:14 UTC (permalink / raw)
To: linux-raid; +Cc: linuxppc-dev, Dan Williams, wd, dzu, Ilya Yanok
The destination address may be present in the source list, so we should
map the addresses from the source list first.
Otherwise, if page corresponding to destination is not marked as write-
through (with regards to CPU cache), then mapping it with DMA_FROM_DEVICE
may lead to data loss, and finally to an incorrect result of calculations.
Signed-off-by: Yuri Tikhonov <yur@emcraft.com>
---
crypto/async_tx/async_xor.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c
index 91dbf07..291f517 100644
--- a/crypto/async_tx/async_xor.c
+++ b/crypto/async_tx/async_xor.c
@@ -53,10 +53,10 @@ do_async_xor(struct dma_chan *chan, struct page *dest, struct page **src_list,
int xor_src_cnt;
dma_addr_t dma_dest;
- dma_dest = dma_map_page(dma->dev, dest, offset, len, DMA_FROM_DEVICE);
for (i = 0; i < src_cnt; i++)
dma_src[i] = dma_map_page(dma->dev, src_list[i], offset,
len, DMA_TO_DEVICE);
+ dma_dest = dma_map_page(dma->dev, dest, offset, len, DMA_FROM_DEVICE);
while (src_cnt) {
async_flags = flags;
--
1.5.6.1
--
Yuri Tikhonov, Senior Software Engineer
Emcraft Systems, www.emcraft.com
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ASYNC_TX: async_xor mapping fix
2008-12-08 19:14 [PATCH] ASYNC_TX: async_xor mapping fix Yuri Tikhonov
@ 2008-12-08 21:08 ` Dan Williams
2008-12-10 18:47 ` Dan Williams
0 siblings, 1 reply; 4+ messages in thread
From: Dan Williams @ 2008-12-08 21:08 UTC (permalink / raw)
To: Yuri Tikhonov
Cc: linux-raid@vger.kernel.org, linuxppc-dev@ozlabs.org, wd@denx.de,
dzu@denx.de, Ilya Yanok
On Mon, 2008-12-08 at 12:14 -0700, Yuri Tikhonov wrote:
> The destination address may be present in the source list, so we should
> map the addresses from the source list first.
> Otherwise, if page corresponding to destination is not marked as write-
> through (with regards to CPU cache), then mapping it with DMA_FROM_DEVICE
> may lead to data loss, and finally to an incorrect result of calculations.
>
Thanks Yuri. I think we should avoid mapping the destination twice
altogether, and for simplicity just always map it bidirectionally.
Something like this (untested):
diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c
index c029d3e..a6faa90 100644
--- a/crypto/async_tx/async_xor.c
+++ b/crypto/async_tx/async_xor.c
@@ -53,10 +53,17 @@ do_async_xor(struct dma_chan *chan, struct page *dest, struct page **src_list,
int xor_src_cnt;
dma_addr_t dma_dest;
- dma_dest = dma_map_page(dma->dev, dest, offset, len, DMA_FROM_DEVICE);
- for (i = 0; i < src_cnt; i++)
+ /* map the dest bidrectional in case it is re-used as a source */
+ dma_dest = dma_map_page(dma->dev, dest, offset, len, DMA_BIDIRECTIONAL);
+ for (i = 0; i < src_cnt; i++) {
+ /* only map the dest once */
+ if (src_list[i] == dest) {
+ dma_src[i] = dma_dest;
+ continue;
+ }
dma_src[i] = dma_map_page(dma->dev, src_list[i], offset,
len, DMA_TO_DEVICE);
+ }
while (src_cnt) {
async_flags = flags;
diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
index c7a9306..7bf4a2e 100644
--- a/drivers/dma/iop-adma.c
+++ b/drivers/dma/iop-adma.c
@@ -86,13 +86,20 @@ iop_adma_run_tx_complete_actions(struct iop_adma_desc_slot *desc,
u32 src_cnt;
dma_addr_t addr;
+ src_cnt = unmap->unmap_src_cnt;
if (!(flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
+ enum dma_data_direction dir;
+
addr = iop_desc_get_dest_addr(unmap, iop_chan);
- dma_unmap_page(dev, addr, len, DMA_FROM_DEVICE);
+ if (src_cnt > 1)
+ dir = DMA_BIDIRECTIONAL;
+ else
+ dir = DMA_FROM_DEVICE;
+
+ dma_unmap_page(dev, addr, len, dir);
}
if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
- src_cnt = unmap->unmap_src_cnt;
while (src_cnt--) {
addr = iop_desc_get_src_addr(unmap,
iop_chan,
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 0328da0..888fbe9 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -312,13 +312,19 @@ mv_xor_run_tx_complete_actions(struct mv_xor_desc_slot *desc,
u32 src_cnt;
dma_addr_t addr;
+ src_cnt = unmap->unmap_src_cnt;
if (!(flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
+ enum dma_data_direction dir;
+
addr = mv_desc_get_dest_addr(unmap);
- dma_unmap_page(dev, addr, len, DMA_FROM_DEVICE);
+ if (src_cnt > 1)
+ dir = DMA_BIDIRECTIONAL;
+ else
+ dir = DMA_FROM_DEVICE;
+ dma_unmap_page(dev, addr, len, dir);
}
if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
- src_cnt = unmap->unmap_src_cnt;
while (src_cnt--) {
addr = mv_desc_get_src_addr(unmap,
src_cnt);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ASYNC_TX: async_xor mapping fix
2008-12-08 21:08 ` Dan Williams
@ 2008-12-10 18:47 ` Dan Williams
2008-12-10 19:07 ` Re[2]: " Yuri Tikhonov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Williams @ 2008-12-10 18:47 UTC (permalink / raw)
To: Yuri Tikhonov
Cc: wd@denx.de, dzu@denx.de, Saeed Bishara,
linux-raid@vger.kernel.org, linuxppc-dev@ozlabs.org, Ilya Yanok
On Mon, 2008-12-08 at 14:08 -0700, Dan Williams wrote:
> On Mon, 2008-12-08 at 12:14 -0700, Yuri Tikhonov wrote:
> > The destination address may be present in the source list, so we should
> > map the addresses from the source list first.
> > Otherwise, if page corresponding to destination is not marked as write-
> > through (with regards to CPU cache), then mapping it with DMA_FROM_DEVICE
> > may lead to data loss, and finally to an incorrect result of calculations.
> >
>
> Thanks Yuri. I think we should avoid mapping the destination twice
> altogether, and for simplicity just always map it bidirectionally.
Yuri, Saeed, can I get your acked-by's for the following 2.6.28 patch:
Thanks,
Dan
----snip---->
async_xor: dma_map destination DMA_BIDIRECTIONAL
From: Dan Williams <dan.j.williams@intel.com>
Mapping the destination multiple times is a misuse of the dma-api.
Since the destination may be reused as a source, ensure that it is only
mapped once and that it is mapped bidirectionally. This appears to add
ugliness on the unmap side in that it always reads back the destination
address from the descriptor, but gcc can determine that dma_unmap is a
nop and not emit the code that calculates its arguments.
Cc: <stable@kernel.org>
Cc: Saeed Bishara <saeed@marvell.com>
Reported-by: Yuri Tikhonov <yur@emcraft.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
crypto/async_tx/async_xor.c | 11 +++++++++--
drivers/dma/iop-adma.c | 16 +++++++++++++---
drivers/dma/mv_xor.c | 15 ++++++++++++---
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c
index c029d3e..a6faa90 100644
--- a/crypto/async_tx/async_xor.c
+++ b/crypto/async_tx/async_xor.c
@@ -53,10 +53,17 @@ do_async_xor(struct dma_chan *chan, struct page *dest, struct page **src_list,
int xor_src_cnt;
dma_addr_t dma_dest;
- dma_dest = dma_map_page(dma->dev, dest, offset, len, DMA_FROM_DEVICE);
- for (i = 0; i < src_cnt; i++)
+ /* map the dest bidrectional in case it is re-used as a source */
+ dma_dest = dma_map_page(dma->dev, dest, offset, len, DMA_BIDIRECTIONAL);
+ for (i = 0; i < src_cnt; i++) {
+ /* only map the dest once */
+ if (src_list[i] == dest) {
+ dma_src[i] = dma_dest;
+ continue;
+ }
dma_src[i] = dma_map_page(dma->dev, src_list[i], offset,
len, DMA_TO_DEVICE);
+ }
while (src_cnt) {
async_flags = flags;
diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
index c7a9306..6be3172 100644
--- a/drivers/dma/iop-adma.c
+++ b/drivers/dma/iop-adma.c
@@ -85,18 +85,28 @@ iop_adma_run_tx_complete_actions(struct iop_adma_desc_slot *desc,
enum dma_ctrl_flags flags = desc->async_tx.flags;
u32 src_cnt;
dma_addr_t addr;
+ dma_addr_t dest;
+ src_cnt = unmap->unmap_src_cnt;
+ dest = iop_desc_get_dest_addr(unmap, iop_chan);
if (!(flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
- addr = iop_desc_get_dest_addr(unmap, iop_chan);
- dma_unmap_page(dev, addr, len, DMA_FROM_DEVICE);
+ enum dma_data_direction dir;
+
+ if (src_cnt > 1) /* is xor? */
+ dir = DMA_BIDIRECTIONAL;
+ else
+ dir = DMA_FROM_DEVICE;
+
+ dma_unmap_page(dev, dest, len, dir);
}
if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
- src_cnt = unmap->unmap_src_cnt;
while (src_cnt--) {
addr = iop_desc_get_src_addr(unmap,
iop_chan,
src_cnt);
+ if (addr == dest)
+ continue;
dma_unmap_page(dev, addr, len,
DMA_TO_DEVICE);
}
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 0328da0..bcda174 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -311,17 +311,26 @@ mv_xor_run_tx_complete_actions(struct mv_xor_desc_slot *desc,
enum dma_ctrl_flags flags = desc->async_tx.flags;
u32 src_cnt;
dma_addr_t addr;
+ dma_addr_t dest;
+ src_cnt = unmap->unmap_src_cnt;
+ dest = mv_desc_get_dest_addr(unmap);
if (!(flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
- addr = mv_desc_get_dest_addr(unmap);
- dma_unmap_page(dev, addr, len, DMA_FROM_DEVICE);
+ enum dma_data_direction dir;
+
+ if (src_cnt > 1) /* is xor ? */
+ dir = DMA_BIDIRECTIONAL;
+ else
+ dir = DMA_FROM_DEVICE;
+ dma_unmap_page(dev, dest, len, dir);
}
if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
- src_cnt = unmap->unmap_src_cnt;
while (src_cnt--) {
addr = mv_desc_get_src_addr(unmap,
src_cnt);
+ if (addr == dest)
+ continue;
dma_unmap_page(dev, addr, len,
DMA_TO_DEVICE);
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re[2]: [PATCH] ASYNC_TX: async_xor mapping fix
2008-12-10 18:47 ` Dan Williams
@ 2008-12-10 19:07 ` Yuri Tikhonov
0 siblings, 0 replies; 4+ messages in thread
From: Yuri Tikhonov @ 2008-12-10 19:07 UTC (permalink / raw)
To: Dan Williams
Cc: wd@denx.de, dzu@denx.de, Saeed Bishara,
linux-raid@vger.kernel.org, linuxppc-dev@ozlabs.org, Ilya Yanok
=0D=0A Hello Dan,
On Wednesday, December 10, 2008 you wrote:
> On Mon, 2008-12-08 at 14:08 -0700, Dan Williams wrote:
>> On Mon, 2008-12-08 at 12:14 -0700, Yuri Tikhonov wrote:
>> > The destination address may be present in the source list, so we should
>> > map the addresses from the source list first.
>> > Otherwise, if page corresponding to destination is not marked as writ=
e-
>> > through (with regards to CPU cache), then mapping it with DMA_FROM_DEV=
ICE
>> > may lead to data loss, and finally to an incorrect result of calculati=
ons.
>> >=20
>>=20
>> Thanks Yuri. I think we should avoid mapping the destination twice
>> altogether, and for simplicity just always map it bidirectionally.
> Yuri, Saeed, can I get your acked-by's for the following 2.6.28 patch:
I would do the "src_list[i] =3D=3D dest" check with unlikely(), since we=
=20
a-priori aware that only one of all src_cnt addresses is the possible=20
destination.
As for the rest:
Acked-by: Yuri Tikhonov <yur@emcraft.com>
> Thanks,
> Dan
----snip---->>
> async_xor: dma_map destination DMA_BIDIRECTIONAL
> From: Dan Williams <dan.j.williams@intel.com>
> Mapping the destination multiple times is a misuse of the dma-api.
> Since the destination may be reused as a source, ensure that it is only
> mapped once and that it is mapped bidirectionally. This appears to add
> ugliness on the unmap side in that it always reads back the destination
> address from the descriptor, but gcc can determine that dma_unmap is a
> nop and not emit the code that calculates its arguments.
> Cc: <stable@kernel.org>
> Cc: Saeed Bishara <saeed@marvell.com>
> Reported-by: Yuri Tikhonov <yur@emcraft.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> crypto/async_tx/async_xor.c | 11 +++++++++--
> drivers/dma/iop-adma.c | 16 +++++++++++++---
> drivers/dma/mv_xor.c | 15 ++++++++++++---
> 3 files changed, 34 insertions(+), 8 deletions(-)
> diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c
> index c029d3e..a6faa90 100644
> --- a/crypto/async_tx/async_xor.c
> +++ b/crypto/async_tx/async_xor.c
> @@ -53,10 +53,17 @@ do_async_xor(struct dma_chan *chan, struct page *dest=
, struct page **src_list,
> int xor_src_cnt;
> dma_addr_t dma_dest;
> =20
> - dma_dest =3D dma_map_page(dma->dev, dest, offset, len, DMA_FROM_D=
EVICE);
> - for (i =3D 0; i < src_cnt; i++)
> + /* map the dest bidrectional in case it is re-used as a source */
> + dma_dest =3D dma_map_page(dma->dev, dest, offset, len, DMA_BIDIRE=
CTIONAL);
> + for (i =3D 0; i < src_cnt; i++) {
> + /* only map the dest once */
> + if (src_list[i] =3D=3D dest) {
> + dma_src[i] =3D dma_dest;
> + continue;
> + }
> dma_src[i] =3D dma_map_page(dma->dev, src_list[i], offset,
> len, DMA_TO_DEVICE);
> + }
> =20
> while (src_cnt) {
> async_flags =3D flags;
> diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
> index c7a9306..6be3172 100644
> --- a/drivers/dma/iop-adma.c
> +++ b/drivers/dma/iop-adma.c
> @@ -85,18 +85,28 @@ iop_adma_run_tx_complete_actions(struct iop_adma_desc=
_slot *desc,
> enum dma_ctrl_flags flags =3D desc->async_tx.flag=
s;
> u32 src_cnt;
> dma_addr_t addr;
> + dma_addr_t dest;
> =20
> + src_cnt =3D unmap->unmap_src_cnt;
> + dest =3D iop_desc_get_dest_addr(unmap, iop_chan);
> if (!(flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> - addr =3D
> iop_desc_get_dest_addr(unmap, iop_chan);
> - dma_unmap_page(dev, addr, len, DMA_FROM_D=
EVICE);
> + enum dma_data_direction dir;
> +
> + if (src_cnt > 1) /* is xor? */
> + dir =3D DMA_BIDIRECTIONAL;
> + else
> + dir =3D DMA_FROM_DEVICE;
> +
> + dma_unmap_page(dev, dest, len, dir);
> }
> =20
> if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> - src_cnt =3D unmap->unmap_src_cnt;
> while (src_cnt--) {
> addr =3D iop_desc_get_src_addr(un=
map,
> iop_c=
han,
> src_c=
nt);
> + if (addr =3D=3D dest)
> + continue;
> dma_unmap_page(dev, addr, len,
> DMA_TO_DEVICE);
> }
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 0328da0..bcda174 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -311,17 +311,26 @@ mv_xor_run_tx_complete_actions(struct mv_xor_desc_s=
lot *desc,
> enum dma_ctrl_flags flags =3D desc->async_tx.flag=
s;
> u32 src_cnt;
> dma_addr_t addr;
> + dma_addr_t dest;
> =20
> + src_cnt =3D unmap->unmap_src_cnt;
> + dest =3D mv_desc_get_dest_addr(unmap);
> if (!(flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> - addr =3D mv_desc_get_dest_addr(unmap);
> - dma_unmap_page(dev, addr, len, DMA_FROM_D=
EVICE);
> + enum dma_data_direction dir;
> +
> + if (src_cnt > 1) /* is xor ? */
> + dir =3D DMA_BIDIRECTIONAL;
> + else
> + dir =3D DMA_FROM_DEVICE;
> + dma_unmap_page(dev, dest, len, dir);
> }
> =20
> if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> - src_cnt =3D unmap->unmap_src_cnt;
> while (src_cnt--) {
> addr =3D mv_desc_get_src_addr(unm=
ap,
> src_c=
nt);
> + if (addr =3D=3D dest)
> + continue;
> dma_unmap_page(dev, addr, len,
> DMA_TO_DEVICE);
> }
Regards, Yuri
--
Yuri Tikhonov, Senior Software Engineer
Emcraft Systems, www.emcraft.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-12-10 19:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-08 19:14 [PATCH] ASYNC_TX: async_xor mapping fix Yuri Tikhonov
2008-12-08 21:08 ` Dan Williams
2008-12-10 18:47 ` Dan Williams
2008-12-10 19:07 ` Re[2]: " Yuri Tikhonov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).