Re: [PATCH v3 6/8] DMA: Freescale: change descriptor release process for supporting async_tx
On Fri, 2014-04-11 at 16:33 +0800, Hongbo Zhang wrote: > >>> + * hardware channel, subsequent descriptors are either in > >>> + * process or have not been submitted > >> Dot at the eol. Check in all comments. > > > > Even though I saw there are other comments without the dots, I think > > it is better to have it. > > Thanks, all. > > > Hmm... think it again, it it really necessary to have it? > Even I have it in my patch, there are already so many comments exists > without it. For my opinion is better to keep style in your patches. Better if it commonly used style in the driver. But comment against comments is really minor thing. -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 6/8] DMA: Freescale: change descriptor release process for supporting async_tx
On 04/11/2014 04:00 PM, Hongbo Zhang wrote:
On 04/10/2014 07:56 PM, Andy Shevchenko wrote:
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
From: Hongbo Zhang
Fix the potential risk when enable config NET_DMA and ASYNC_TX.
Async_tx is
lack of support in current release process of dma descriptor, all
descriptors
will be released whatever is acked or no-acked by async_tx, so there
is a
potential race condition when dma engine is uesd by others clients
(e.g. when
enable NET_DMA to offload TCP).
In our case, a race condition which is raised when use both of
talitos and
dmaengine to offload xor is because napi scheduler will sync all
pending
requests in dma channels, it affects the process of raid operations
due to
ack_tx is not checked in fsl dma. The no-acked descriptor is freed
which is
submitted just now, as a dependent tx, this freed descriptor trigger
BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf4 CPU: 0
GPR00: 0001 ecf41ca0 ee44/921a94a0 003f 0001 c00593e4
0001
GPR08: a7a7a7a7 0001 045/92002 42028042 100a38d4
ed576d98
GPR16: ed5a11b0 2b162000 0200 046/92000 2d555000
ed3015e8 c15a7aa0
GPR24: c155fc40 ecb63220 ecf41d28 e47/92f640bb0
ef640c30 ecf41ca0
NIP [c02b048c] async_tx_submit+0x6c/0x2b4
LR [c02b068c] async_tx_submit+0x26c/0x2b4
Call Trace:
[ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
[ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c
[ecf41d20] [c0421064] async_copy_data+0xa0/0x17c
[ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10
[ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8
[ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
[ecf41f40] [c04329b8] md_thread+0x138/0x16c
[ecf41f90] [c008277c] kthread+0x8c/0x90
[ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
Another modification in this patch is the change of completed
descriptors,
there is a potential risk which caused by exception interrupt, all
descriptors
in ld_running list are seemed completed when an interrupt raised, it
works fine
under normal condition, but if there is an exception occured, it
cannot work as
our excepted. Hardware should not be depend on s/w list, the right
way is to
read current descriptor address register to find the last completed
descriptor.
If an interrupt is raised by an error, all descriptors in ld_running
should not
be seemed finished, or these unfinished descriptors in ld_running
will be
released wrongly.
A simple way to reproduce:
Enable dmatest first, then insert some bad descriptors which can
trigger
Programming Error interrupts before the good descriptors. Last, the
good
descriptors will be freed before they are processsed because of the
exception
intrerrupt.
Note: the bad descriptors are only for simulating an exception
interrupt. This
case can illustrate the potential risk in current fsl-dma very well.
Signed-off-by: Hongbo Zhang
Signed-off-by: Qiang Liu
Signed-off-by: Ira W. Snyder
---
drivers/dma/fsldma.c | 195
--
drivers/dma/fsldma.h | 17 -
2 files changed, 158 insertions(+), 54 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 968877f..f8eee60 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -459,6 +459,87 @@ static struct fsl_desc_sw
*fsl_dma_alloc_descriptor(struct fsldma_chan *chan)
}
/**
+ * fsldma_clean_completed_descriptor - free all descriptors which
+ * has been completed and acked
IIRC the summary should be oneliner.
Check the rest of the code as well.
I don't think so.
See this Documentation/kernel-doc-nano-HOWTO.txt, and you can find
this sentence "The short description following the subject can span
multiple lines"
+ * @chan: Freescale DMA channel
+ *
+ * This function is used on all completed and acked descriptors.
+ * All descriptors should only be freed in this function.
+ */
+static void fsldma_clean_completed_descriptor(struct fsldma_chan
*chan)
+{
+struct fsl_desc_sw *desc, *_desc;
+
+/* Run the callback for each descriptor, in order */
+list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node)
+if (async_tx_test_ack(&desc->async_tx))
+fsl_dma_free_descriptor(chan, desc);
+}
+
+/**
+ * fsldma_run_tx_complete_actions - cleanup a single link descriptor
+ * @chan: Freescale DMA channel
+ * @desc: descriptor to cleanup and free
+ * @cookie: Freescale DMA transaction identifier
+ *
+ * This function is used on a descriptor which has been executed by
the DMA
+ * controller. It will run any callbacks, submit any dependencies.
+ */
+static dma_cookie_t fsldma_run_tx_complete_actions(struct
fsldma_chan *chan,
+struct fsl_desc_sw *desc, dma_cookie_t cookie)
Maybe you could use cookie as local variable?
Yes.. it doesn't seem good to set a value to input parameter.
+{
+struct dma_async_tx_descriptor *txd
Re: [PATCH v3 6/8] DMA: Freescale: change descriptor release process for supporting async_tx
On 04/10/2014 07:56 PM, Andy Shevchenko wrote:
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
From: Hongbo Zhang
Fix the potential risk when enable config NET_DMA and ASYNC_TX. Async_tx is
lack of support in current release process of dma descriptor, all descriptors
will be released whatever is acked or no-acked by async_tx, so there is a
potential race condition when dma engine is uesd by others clients (e.g. when
enable NET_DMA to offload TCP).
In our case, a race condition which is raised when use both of talitos and
dmaengine to offload xor is because napi scheduler will sync all pending
requests in dma channels, it affects the process of raid operations due to
ack_tx is not checked in fsl dma. The no-acked descriptor is freed which is
submitted just now, as a dependent tx, this freed descriptor trigger
BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf4 CPU: 0
GPR00: 0001 ecf41ca0 ee44/921a94a0 003f 0001 c00593e4
0001
GPR08: a7a7a7a7 0001 045/92002 42028042 100a38d4 ed576d98
GPR16: ed5a11b0 2b162000 0200 046/92000 2d555000 ed3015e8
c15a7aa0
GPR24: c155fc40 ecb63220 ecf41d28 e47/92f640bb0 ef640c30
ecf41ca0
NIP [c02b048c] async_tx_submit+0x6c/0x2b4
LR [c02b068c] async_tx_submit+0x26c/0x2b4
Call Trace:
[ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
[ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c
[ecf41d20] [c0421064] async_copy_data+0xa0/0x17c
[ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10
[ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8
[ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
[ecf41f40] [c04329b8] md_thread+0x138/0x16c
[ecf41f90] [c008277c] kthread+0x8c/0x90
[ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
Another modification in this patch is the change of completed descriptors,
there is a potential risk which caused by exception interrupt, all descriptors
in ld_running list are seemed completed when an interrupt raised, it works fine
under normal condition, but if there is an exception occured, it cannot work as
our excepted. Hardware should not be depend on s/w list, the right way is to
read current descriptor address register to find the last completed descriptor.
If an interrupt is raised by an error, all descriptors in ld_running should not
be seemed finished, or these unfinished descriptors in ld_running will be
released wrongly.
A simple way to reproduce:
Enable dmatest first, then insert some bad descriptors which can trigger
Programming Error interrupts before the good descriptors. Last, the good
descriptors will be freed before they are processsed because of the exception
intrerrupt.
Note: the bad descriptors are only for simulating an exception interrupt. This
case can illustrate the potential risk in current fsl-dma very well.
Signed-off-by: Hongbo Zhang
Signed-off-by: Qiang Liu
Signed-off-by: Ira W. Snyder
---
drivers/dma/fsldma.c | 195 --
drivers/dma/fsldma.h | 17 -
2 files changed, 158 insertions(+), 54 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 968877f..f8eee60 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -459,6 +459,87 @@ static struct fsl_desc_sw *fsl_dma_alloc_descriptor(struct
fsldma_chan *chan)
}
/**
+ * fsldma_clean_completed_descriptor - free all descriptors which
+ * has been completed and acked
IIRC the summary should be oneliner.
Check the rest of the code as well.
I don't think so.
See this Documentation/kernel-doc-nano-HOWTO.txt, and you can find this
sentence "The short description following the subject can span multiple
lines"
+ * @chan: Freescale DMA channel
+ *
+ * This function is used on all completed and acked descriptors.
+ * All descriptors should only be freed in this function.
+ */
+static void fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
+{
+ struct fsl_desc_sw *desc, *_desc;
+
+ /* Run the callback for each descriptor, in order */
+ list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node)
+ if (async_tx_test_ack(&desc->async_tx))
+ fsl_dma_free_descriptor(chan, desc);
+}
+
+/**
+ * fsldma_run_tx_complete_actions - cleanup a single link descriptor
+ * @chan: Freescale DMA channel
+ * @desc: descriptor to cleanup and free
+ * @cookie: Freescale DMA transaction identifier
+ *
+ * This function is used on a descriptor which has been executed by the DMA
+ * controller. It will run any callbacks, submit any dependencies.
+ */
+static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
+ struct fsl_desc_sw *desc, dma_cookie_t cookie)
Maybe you could use cookie as local variable?
Yes.. it doesn't seem good to set a value to input parameter.
+{
+ struct dma_async_tx_descriptor *txd = &desc->async_tx;
+
+ BUG_ON(txd->
Re: [PATCH v3 6/8] DMA: Freescale: change descriptor release process for supporting async_tx
On Thu, 2014-04-10 at 15:10 +0800, hongbo.zh...@freescale.com wrote:
> From: Hongbo Zhang
>
> Fix the potential risk when enable config NET_DMA and ASYNC_TX. Async_tx is
> lack of support in current release process of dma descriptor, all descriptors
> will be released whatever is acked or no-acked by async_tx, so there is a
> potential race condition when dma engine is uesd by others clients (e.g. when
> enable NET_DMA to offload TCP).
>
> In our case, a race condition which is raised when use both of talitos and
> dmaengine to offload xor is because napi scheduler will sync all pending
> requests in dma channels, it affects the process of raid operations due to
> ack_tx is not checked in fsl dma. The no-acked descriptor is freed which is
> submitted just now, as a dependent tx, this freed descriptor trigger
> BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
>
> TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf4 CPU: 0
> GPR00: 0001 ecf41ca0 ee44/921a94a0 003f 0001 c00593e4
> 0001
> GPR08: a7a7a7a7 0001 045/92002 42028042 100a38d4 ed576d98
>
> GPR16: ed5a11b0 2b162000 0200 046/92000 2d555000 ed3015e8
> c15a7aa0
> GPR24: c155fc40 ecb63220 ecf41d28 e47/92f640bb0 ef640c30
> ecf41ca0
> NIP [c02b048c] async_tx_submit+0x6c/0x2b4
> LR [c02b068c] async_tx_submit+0x26c/0x2b4
> Call Trace:
> [ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
> [ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c
> [ecf41d20] [c0421064] async_copy_data+0xa0/0x17c
> [ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10
> [ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8
> [ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
> [ecf41f40] [c04329b8] md_thread+0x138/0x16c
> [ecf41f90] [c008277c] kthread+0x8c/0x90
> [ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
>
> Another modification in this patch is the change of completed descriptors,
> there is a potential risk which caused by exception interrupt, all descriptors
> in ld_running list are seemed completed when an interrupt raised, it works
> fine
> under normal condition, but if there is an exception occured, it cannot work
> as
> our excepted. Hardware should not be depend on s/w list, the right way is to
> read current descriptor address register to find the last completed
> descriptor.
> If an interrupt is raised by an error, all descriptors in ld_running should
> not
> be seemed finished, or these unfinished descriptors in ld_running will be
> released wrongly.
>
> A simple way to reproduce:
> Enable dmatest first, then insert some bad descriptors which can trigger
> Programming Error interrupts before the good descriptors. Last, the good
> descriptors will be freed before they are processsed because of the exception
> intrerrupt.
>
> Note: the bad descriptors are only for simulating an exception interrupt.
> This
> case can illustrate the potential risk in current fsl-dma very well.
>
> Signed-off-by: Hongbo Zhang
> Signed-off-by: Qiang Liu
> Signed-off-by: Ira W. Snyder
> ---
> drivers/dma/fsldma.c | 195
> --
> drivers/dma/fsldma.h | 17 -
> 2 files changed, 158 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 968877f..f8eee60 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -459,6 +459,87 @@ static struct fsl_desc_sw
> *fsl_dma_alloc_descriptor(struct fsldma_chan *chan)
> }
>
> /**
> + * fsldma_clean_completed_descriptor - free all descriptors which
> + * has been completed and acked
IIRC the summary should be oneliner.
Check the rest of the code as well.
> + * @chan: Freescale DMA channel
> + *
> + * This function is used on all completed and acked descriptors.
> + * All descriptors should only be freed in this function.
> + */
> +static void fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
> +{
> + struct fsl_desc_sw *desc, *_desc;
> +
> + /* Run the callback for each descriptor, in order */
> + list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node)
> + if (async_tx_test_ack(&desc->async_tx))
> + fsl_dma_free_descriptor(chan, desc);
> +}
> +
> +/**
> + * fsldma_run_tx_complete_actions - cleanup a single link descriptor
> + * @chan: Freescale DMA channel
> + * @desc: descriptor to cleanup and free
> + * @cookie: Freescale DMA transaction identifier
> + *
> + * This function is used on a descriptor which has been executed by the DMA
> + * controller. It will run any callbacks, submit any dependencies.
> + */
> +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
> + struct fsl_desc_sw *desc, dma_cookie_t cookie)
Maybe you could use cookie as local variable?
> +{
> + struct dma_async_tx_descriptor *txd = &desc->async_tx;
> +
> + BUG_ON(txd->cookie < 0);
> +
> + if (txd->cookie > 0) {
> + cookie = txd->cookie;
[PATCH v3 6/8] DMA: Freescale: change descriptor release process for supporting async_tx
From: Hongbo Zhang
Fix the potential risk when enable config NET_DMA and ASYNC_TX. Async_tx is
lack of support in current release process of dma descriptor, all descriptors
will be released whatever is acked or no-acked by async_tx, so there is a
potential race condition when dma engine is uesd by others clients (e.g. when
enable NET_DMA to offload TCP).
In our case, a race condition which is raised when use both of talitos and
dmaengine to offload xor is because napi scheduler will sync all pending
requests in dma channels, it affects the process of raid operations due to
ack_tx is not checked in fsl dma. The no-acked descriptor is freed which is
submitted just now, as a dependent tx, this freed descriptor trigger
BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf4 CPU: 0
GPR00: 0001 ecf41ca0 ee44/921a94a0 003f 0001 c00593e4
0001
GPR08: a7a7a7a7 0001 045/92002 42028042 100a38d4 ed576d98
GPR16: ed5a11b0 2b162000 0200 046/92000 2d555000 ed3015e8
c15a7aa0
GPR24: c155fc40 ecb63220 ecf41d28 e47/92f640bb0 ef640c30
ecf41ca0
NIP [c02b048c] async_tx_submit+0x6c/0x2b4
LR [c02b068c] async_tx_submit+0x26c/0x2b4
Call Trace:
[ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
[ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c
[ecf41d20] [c0421064] async_copy_data+0xa0/0x17c
[ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10
[ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8
[ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
[ecf41f40] [c04329b8] md_thread+0x138/0x16c
[ecf41f90] [c008277c] kthread+0x8c/0x90
[ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
Another modification in this patch is the change of completed descriptors,
there is a potential risk which caused by exception interrupt, all descriptors
in ld_running list are seemed completed when an interrupt raised, it works fine
under normal condition, but if there is an exception occured, it cannot work as
our excepted. Hardware should not be depend on s/w list, the right way is to
read current descriptor address register to find the last completed descriptor.
If an interrupt is raised by an error, all descriptors in ld_running should not
be seemed finished, or these unfinished descriptors in ld_running will be
released wrongly.
A simple way to reproduce:
Enable dmatest first, then insert some bad descriptors which can trigger
Programming Error interrupts before the good descriptors. Last, the good
descriptors will be freed before they are processsed because of the exception
intrerrupt.
Note: the bad descriptors are only for simulating an exception interrupt. This
case can illustrate the potential risk in current fsl-dma very well.
Signed-off-by: Hongbo Zhang
Signed-off-by: Qiang Liu
Signed-off-by: Ira W. Snyder
---
drivers/dma/fsldma.c | 195 --
drivers/dma/fsldma.h | 17 -
2 files changed, 158 insertions(+), 54 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 968877f..f8eee60 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -459,6 +459,87 @@ static struct fsl_desc_sw *fsl_dma_alloc_descriptor(struct
fsldma_chan *chan)
}
/**
+ * fsldma_clean_completed_descriptor - free all descriptors which
+ * has been completed and acked
+ * @chan: Freescale DMA channel
+ *
+ * This function is used on all completed and acked descriptors.
+ * All descriptors should only be freed in this function.
+ */
+static void fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
+{
+ struct fsl_desc_sw *desc, *_desc;
+
+ /* Run the callback for each descriptor, in order */
+ list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node)
+ if (async_tx_test_ack(&desc->async_tx))
+ fsl_dma_free_descriptor(chan, desc);
+}
+
+/**
+ * fsldma_run_tx_complete_actions - cleanup a single link descriptor
+ * @chan: Freescale DMA channel
+ * @desc: descriptor to cleanup and free
+ * @cookie: Freescale DMA transaction identifier
+ *
+ * This function is used on a descriptor which has been executed by the DMA
+ * controller. It will run any callbacks, submit any dependencies.
+ */
+static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
+ struct fsl_desc_sw *desc, dma_cookie_t cookie)
+{
+ struct dma_async_tx_descriptor *txd = &desc->async_tx;
+
+ BUG_ON(txd->cookie < 0);
+
+ if (txd->cookie > 0) {
+ cookie = txd->cookie;
+
+ /* Run the link descriptor callback function */
+ if (txd->callback) {
+ chan_dbg(chan, "LD %p callback\n", desc);
+ txd->callback(txd->callback_param);
+ }
+ }
+
+ /* Run any dependencies */
+ dma_run_dependencies(txd);
+
+ return cookie;
+}
+
+/**
+ * fsldma_clean_running_descriptor - move
