* [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
@ 2011-02-23 17:29 ` Stephen Warren
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Warren @ 2011-02-23 17:29 UTC (permalink / raw)
To: ccross-z5hGa2qSFaRBDgjK7y7TUQ
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
konkers-z5hGa2qSFaRBDgjK7y7TUQ, Stephen Warren
tegra_dma_init currently simply bails out early if any initialization fails.
This skips various data-structure initialization. In turn, this means that
tegra_dma_allocate_channel can still hand out channels. In this case, when
tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
on ch->list will OOPS since the list's next/prev pointers may still be
NULL.
To solve this:
* Mark all possible channels as in-use before doing anything else in init.
* Only mark a channel as free once all channel-related initialization has
completed.
This prevents allocate_channel from handing out uninitialized channels.
There is still one small hole; allocate_channel can't check the usage array
for the shared channel, since this channel is permanently marked in-use.
This could be solved using an explicit "init OK" flag that allocate_channel
could check.
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
arch/arm/mach-tegra/dma.c | 20 ++++++++------------
1 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c
index 2d720f2..79765ca 100644
--- a/arch/arm/mach-tegra/dma.c
+++ b/arch/arm/mach-tegra/dma.c
@@ -678,6 +678,12 @@ int __init tegra_dma_init(void)
void __iomem *addr;
struct clk *c;
+ memset(channel_usage, 0, sizeof(channel_usage));
+ memset(dma_channels, 0, sizeof(dma_channels));
+
+ for (i = 0; i < NV_DMA_MAX_CHANNELS; i++)
+ __set_bit(i, channel_usage);
+
c = clk_get_sys("tegra-dma", NULL);
if (IS_ERR(c)) {
pr_err("Unable to get clock for APB DMA\n");
@@ -696,18 +702,9 @@ int __init tegra_dma_init(void)
writel(0xFFFFFFFFul >> (31 - TEGRA_SYSTEM_DMA_CH_MAX),
addr + APB_DMA_IRQ_MASK_SET);
- memset(channel_usage, 0, sizeof(channel_usage));
- memset(dma_channels, 0, sizeof(dma_channels));
-
- /* Reserve all the channels we are not supposed to touch */
- for (i = 0; i < TEGRA_SYSTEM_DMA_CH_MIN; i++)
- __set_bit(i, channel_usage);
-
for (i = TEGRA_SYSTEM_DMA_CH_MIN; i <= TEGRA_SYSTEM_DMA_CH_MAX; i++) {
struct tegra_dma_channel *ch = &dma_channels[i];
- __clear_bit(i, channel_usage);
-
ch->id = i;
snprintf(ch->name, TEGRA_DMA_NAME_SIZE, "dma_channel_%d", i);
@@ -726,13 +723,12 @@ int __init tegra_dma_init(void)
goto fail;
}
ch->irq = irq;
+
+ __clear_bit(i, channel_usage);
}
/* mark the shared channel allocated */
__set_bit(TEGRA_SYSTEM_DMA_CH_MIN, channel_usage);
- for (i = TEGRA_SYSTEM_DMA_CH_MAX+1; i < NV_DMA_MAX_CHANNELS; i++)
- __set_bit(i, channel_usage);
-
return ret;
fail:
writel(0, addr + APB_DMA_GEN);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
@ 2011-02-23 17:29 ` Stephen Warren
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Warren @ 2011-02-23 17:29 UTC (permalink / raw)
To: linux-arm-kernel
tegra_dma_init currently simply bails out early if any initialization fails.
This skips various data-structure initialization. In turn, this means that
tegra_dma_allocate_channel can still hand out channels. In this case, when
tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
on ch->list will OOPS since the list's next/prev pointers may still be
NULL.
To solve this:
* Mark all possible channels as in-use before doing anything else in init.
* Only mark a channel as free once all channel-related initialization has
completed.
This prevents allocate_channel from handing out uninitialized channels.
There is still one small hole; allocate_channel can't check the usage array
for the shared channel, since this channel is permanently marked in-use.
This could be solved using an explicit "init OK" flag that allocate_channel
could check.
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
arch/arm/mach-tegra/dma.c | 20 ++++++++------------
1 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c
index 2d720f2..79765ca 100644
--- a/arch/arm/mach-tegra/dma.c
+++ b/arch/arm/mach-tegra/dma.c
@@ -678,6 +678,12 @@ int __init tegra_dma_init(void)
void __iomem *addr;
struct clk *c;
+ memset(channel_usage, 0, sizeof(channel_usage));
+ memset(dma_channels, 0, sizeof(dma_channels));
+
+ for (i = 0; i < NV_DMA_MAX_CHANNELS; i++)
+ __set_bit(i, channel_usage);
+
c = clk_get_sys("tegra-dma", NULL);
if (IS_ERR(c)) {
pr_err("Unable to get clock for APB DMA\n");
@@ -696,18 +702,9 @@ int __init tegra_dma_init(void)
writel(0xFFFFFFFFul >> (31 - TEGRA_SYSTEM_DMA_CH_MAX),
addr + APB_DMA_IRQ_MASK_SET);
- memset(channel_usage, 0, sizeof(channel_usage));
- memset(dma_channels, 0, sizeof(dma_channels));
-
- /* Reserve all the channels we are not supposed to touch */
- for (i = 0; i < TEGRA_SYSTEM_DMA_CH_MIN; i++)
- __set_bit(i, channel_usage);
-
for (i = TEGRA_SYSTEM_DMA_CH_MIN; i <= TEGRA_SYSTEM_DMA_CH_MAX; i++) {
struct tegra_dma_channel *ch = &dma_channels[i];
- __clear_bit(i, channel_usage);
-
ch->id = i;
snprintf(ch->name, TEGRA_DMA_NAME_SIZE, "dma_channel_%d", i);
@@ -726,13 +723,12 @@ int __init tegra_dma_init(void)
goto fail;
}
ch->irq = irq;
+
+ __clear_bit(i, channel_usage);
}
/* mark the shared channel allocated */
__set_bit(TEGRA_SYSTEM_DMA_CH_MIN, channel_usage);
- for (i = TEGRA_SYSTEM_DMA_CH_MAX+1; i < NV_DMA_MAX_CHANNELS; i++)
- __set_bit(i, channel_usage);
-
return ret;
fail:
writel(0, addr + APB_DMA_GEN);
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
2011-02-23 17:29 ` Stephen Warren
@ 2011-02-23 17:38 ` Colin Cross
-1 siblings, 0 replies; 8+ messages in thread
From: Colin Cross @ 2011-02-23 17:38 UTC (permalink / raw)
To: Stephen Warren
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
konkers-z5hGa2qSFaRBDgjK7y7TUQ
On Wed, Feb 23, 2011 at 9:29 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> tegra_dma_init currently simply bails out early if any initialization fails.
> This skips various data-structure initialization. In turn, this means that
> tegra_dma_allocate_channel can still hand out channels. In this case, when
> tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
> on ch->list will OOPS since the list's next/prev pointers may still be
> NULL.
>
> To solve this:
> * Mark all possible channels as in-use before doing anything else in init.
> * Only mark a channel as free once all channel-related initialization has
> completed.
>
> This prevents allocate_channel from handing out uninitialized channels.
>
> There is still one small hole; allocate_channel can't check the usage array
> for the shared channel, since this channel is permanently marked in-use.
> This could be solved using an explicit "init OK" flag that allocate_channel
> could check.
If we still need an init complete flag, why not skip this patch and
just add that?
>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> arch/arm/mach-tegra/dma.c | 20 ++++++++------------
> 1 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c
> index 2d720f2..79765ca 100644
> --- a/arch/arm/mach-tegra/dma.c
> +++ b/arch/arm/mach-tegra/dma.c
> @@ -678,6 +678,12 @@ int __init tegra_dma_init(void)
> void __iomem *addr;
> struct clk *c;
>
> + memset(channel_usage, 0, sizeof(channel_usage));
> + memset(dma_channels, 0, sizeof(dma_channels));
I noticed yesterday these memsets were unnecessary, channel_usage and
dma_channels will be allocated as 0.
> +
> + for (i = 0; i < NV_DMA_MAX_CHANNELS; i++)
> + __set_bit(i, channel_usage);
> +
> c = clk_get_sys("tegra-dma", NULL);
> if (IS_ERR(c)) {
> pr_err("Unable to get clock for APB DMA\n");
> @@ -696,18 +702,9 @@ int __init tegra_dma_init(void)
> writel(0xFFFFFFFFul >> (31 - TEGRA_SYSTEM_DMA_CH_MAX),
> addr + APB_DMA_IRQ_MASK_SET);
>
> - memset(channel_usage, 0, sizeof(channel_usage));
> - memset(dma_channels, 0, sizeof(dma_channels));
> -
> - /* Reserve all the channels we are not supposed to touch */
> - for (i = 0; i < TEGRA_SYSTEM_DMA_CH_MIN; i++)
> - __set_bit(i, channel_usage);
> -
> for (i = TEGRA_SYSTEM_DMA_CH_MIN; i <= TEGRA_SYSTEM_DMA_CH_MAX; i++) {
> struct tegra_dma_channel *ch = &dma_channels[i];
>
> - __clear_bit(i, channel_usage);
> -
> ch->id = i;
> snprintf(ch->name, TEGRA_DMA_NAME_SIZE, "dma_channel_%d", i);
>
> @@ -726,13 +723,12 @@ int __init tegra_dma_init(void)
> goto fail;
> }
> ch->irq = irq;
> +
> + __clear_bit(i, channel_usage);
> }
> /* mark the shared channel allocated */
> __set_bit(TEGRA_SYSTEM_DMA_CH_MIN, channel_usage);
>
> - for (i = TEGRA_SYSTEM_DMA_CH_MAX+1; i < NV_DMA_MAX_CHANNELS; i++)
> - __set_bit(i, channel_usage);
> -
> return ret;
> fail:
> writel(0, addr + APB_DMA_GEN);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
@ 2011-02-23 17:38 ` Colin Cross
0 siblings, 0 replies; 8+ messages in thread
From: Colin Cross @ 2011-02-23 17:38 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Feb 23, 2011 at 9:29 AM, Stephen Warren <swarren@nvidia.com> wrote:
> tegra_dma_init currently simply bails out early if any initialization fails.
> This skips various data-structure initialization. In turn, this means that
> tegra_dma_allocate_channel can still hand out channels. In this case, when
> tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
> on ch->list will OOPS since the list's next/prev pointers may still be
> NULL.
>
> To solve this:
> * Mark all possible channels as in-use before doing anything else in init.
> * Only mark a channel as free once all channel-related initialization has
> ?completed.
>
> This prevents allocate_channel from handing out uninitialized channels.
>
> There is still one small hole; allocate_channel can't check the usage array
> for the shared channel, since this channel is permanently marked in-use.
> This could be solved using an explicit "init OK" flag that allocate_channel
> could check.
If we still need an init complete flag, why not skip this patch and
just add that?
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> ?arch/arm/mach-tegra/dma.c | ? 20 ++++++++------------
> ?1 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/dma.c b/arch/arm/mach-tegra/dma.c
> index 2d720f2..79765ca 100644
> --- a/arch/arm/mach-tegra/dma.c
> +++ b/arch/arm/mach-tegra/dma.c
> @@ -678,6 +678,12 @@ int __init tegra_dma_init(void)
> ? ? ? ?void __iomem *addr;
> ? ? ? ?struct clk *c;
>
> + ? ? ? memset(channel_usage, 0, sizeof(channel_usage));
> + ? ? ? memset(dma_channels, 0, sizeof(dma_channels));
I noticed yesterday these memsets were unnecessary, channel_usage and
dma_channels will be allocated as 0.
> +
> + ? ? ? for (i = 0; i < NV_DMA_MAX_CHANNELS; i++)
> + ? ? ? ? ? ? ? __set_bit(i, channel_usage);
> +
> ? ? ? ?c = clk_get_sys("tegra-dma", NULL);
> ? ? ? ?if (IS_ERR(c)) {
> ? ? ? ? ? ? ? ?pr_err("Unable to get clock for APB DMA\n");
> @@ -696,18 +702,9 @@ int __init tegra_dma_init(void)
> ? ? ? ?writel(0xFFFFFFFFul >> (31 - TEGRA_SYSTEM_DMA_CH_MAX),
> ? ? ? ? ? ? ? addr + APB_DMA_IRQ_MASK_SET);
>
> - ? ? ? memset(channel_usage, 0, sizeof(channel_usage));
> - ? ? ? memset(dma_channels, 0, sizeof(dma_channels));
> -
> - ? ? ? /* Reserve all the channels we are not supposed to touch */
> - ? ? ? for (i = 0; i < TEGRA_SYSTEM_DMA_CH_MIN; i++)
> - ? ? ? ? ? ? ? __set_bit(i, channel_usage);
> -
> ? ? ? ?for (i = TEGRA_SYSTEM_DMA_CH_MIN; i <= TEGRA_SYSTEM_DMA_CH_MAX; i++) {
> ? ? ? ? ? ? ? ?struct tegra_dma_channel *ch = &dma_channels[i];
>
> - ? ? ? ? ? ? ? __clear_bit(i, channel_usage);
> -
> ? ? ? ? ? ? ? ?ch->id = i;
> ? ? ? ? ? ? ? ?snprintf(ch->name, TEGRA_DMA_NAME_SIZE, "dma_channel_%d", i);
>
> @@ -726,13 +723,12 @@ int __init tegra_dma_init(void)
> ? ? ? ? ? ? ? ? ? ? ? ?goto fail;
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?ch->irq = irq;
> +
> + ? ? ? ? ? ? ? __clear_bit(i, channel_usage);
> ? ? ? ?}
> ? ? ? ?/* mark the shared channel allocated */
> ? ? ? ?__set_bit(TEGRA_SYSTEM_DMA_CH_MIN, channel_usage);
>
> - ? ? ? for (i = TEGRA_SYSTEM_DMA_CH_MAX+1; i < NV_DMA_MAX_CHANNELS; i++)
> - ? ? ? ? ? ? ? __set_bit(i, channel_usage);
> -
> ? ? ? ?return ret;
> ?fail:
> ? ? ? ?writel(0, addr + APB_DMA_GEN);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
2011-02-23 17:38 ` Colin Cross
@ 2011-02-23 17:54 ` Stephen Warren
-1 siblings, 0 replies; 8+ messages in thread
From: Stephen Warren @ 2011-02-23 17:54 UTC (permalink / raw)
To: Colin Cross
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
konkers-z5hGa2qSFaRBDgjK7y7TUQ
Colin Cross wrote at Wednesday, February 23, 2011 10:38 AM:
>
> On Wed, Feb 23, 2011 at 9:29 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > tegra_dma_init currently simply bails out early if any initialization fails.
> > This skips various data-structure initialization. In turn, this means that
> > tegra_dma_allocate_channel can still hand out channels. In this case, when
> > tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
> > on ch->list will OOPS since the list's next/prev pointers may still be
> > NULL.
> >
> > To solve this:
> > * Mark all possible channels as in-use before doing anything else in init.
> > * Only mark a channel as free once all channel-related initialization has
> > completed.
> >
> > This prevents allocate_channel from handing out uninitialized channels.
> >
> > There is still one small hole; allocate_channel can't check the usage array
> > for the shared channel, since this channel is permanently marked in-use.
> > This could be solved using an explicit "init OK" flag that allocate_channel
> > could check.
>
> If we still need an init complete flag, why not skip this patch and
> just add that?
I tend to like the cleanup in this patch; it simplifies the channel_usage
initialization. I'll let you make the call on whether to take this or not.
I can certainly throw together a second patch, or modify this patch, to also
use an init_ok flag to completely solve the shared channel case too though.
--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
@ 2011-02-23 17:54 ` Stephen Warren
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Warren @ 2011-02-23 17:54 UTC (permalink / raw)
To: linux-arm-kernel
Colin Cross wrote at Wednesday, February 23, 2011 10:38 AM:
>
> On Wed, Feb 23, 2011 at 9:29 AM, Stephen Warren <swarren@nvidia.com> wrote:
> > tegra_dma_init currently simply bails out early if any initialization fails.
> > This skips various data-structure initialization. In turn, this means that
> > tegra_dma_allocate_channel can still hand out channels. In this case, when
> > tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
> > on ch->list will OOPS since the list's next/prev pointers may still be
> > NULL.
> >
> > To solve this:
> > * Mark all possible channels as in-use before doing anything else in init.
> > * Only mark a channel as free once all channel-related initialization has
> > ?completed.
> >
> > This prevents allocate_channel from handing out uninitialized channels.
> >
> > There is still one small hole; allocate_channel can't check the usage array
> > for the shared channel, since this channel is permanently marked in-use.
> > This could be solved using an explicit "init OK" flag that allocate_channel
> > could check.
>
> If we still need an init complete flag, why not skip this patch and
> just add that?
I tend to like the cleanup in this patch; it simplifies the channel_usage
initialization. I'll let you make the call on whether to take this or not.
I can certainly throw together a second patch, or modify this patch, to also
use an init_ok flag to completely solve the shared channel case too though.
--
nvpublic
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
2011-02-23 17:54 ` Stephen Warren
@ 2011-02-23 18:39 ` Colin Cross
-1 siblings, 0 replies; 8+ messages in thread
From: Colin Cross @ 2011-02-23 18:39 UTC (permalink / raw)
To: Stephen Warren
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
konkers-z5hGa2qSFaRBDgjK7y7TUQ
On Wed, Feb 23, 2011 at 9:54 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Colin Cross wrote at Wednesday, February 23, 2011 10:38 AM:
>>
>> On Wed, Feb 23, 2011 at 9:29 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > tegra_dma_init currently simply bails out early if any initialization fails.
>> > This skips various data-structure initialization. In turn, this means that
>> > tegra_dma_allocate_channel can still hand out channels. In this case, when
>> > tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
>> > on ch->list will OOPS since the list's next/prev pointers may still be
>> > NULL.
>> >
>> > To solve this:
>> > * Mark all possible channels as in-use before doing anything else in init.
>> > * Only mark a channel as free once all channel-related initialization has
>> > completed.
>> >
>> > This prevents allocate_channel from handing out uninitialized channels.
>> >
>> > There is still one small hole; allocate_channel can't check the usage array
>> > for the shared channel, since this channel is permanently marked in-use.
>> > This could be solved using an explicit "init OK" flag that allocate_channel
>> > could check.
>>
>> If we still need an init complete flag, why not skip this patch and
>> just add that?
>
> I tend to like the cleanup in this patch; it simplifies the channel_usage
> initialization. I'll let you make the call on whether to take this or not.
Ok, it seems like a reasonable cleanup. Can I request a few changes?
Get rid of the redundant memsets
Replace the initial for loop to set all the bits with bitmap_set
Might as well add the init complete flag to this patch as well. You
could also do it by checking the channel_usage bit for the shared
channel, and never setting it in tegra_dma_allocate_channel.
> I can certainly throw together a second patch, or modify this patch, to also
> use an init_ok flag to completely solve the shared channel case too though.
>
> --
> nvpublic
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails
@ 2011-02-23 18:39 ` Colin Cross
0 siblings, 0 replies; 8+ messages in thread
From: Colin Cross @ 2011-02-23 18:39 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Feb 23, 2011 at 9:54 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Colin Cross wrote at Wednesday, February 23, 2011 10:38 AM:
>>
>> On Wed, Feb 23, 2011 at 9:29 AM, Stephen Warren <swarren@nvidia.com> wrote:
>> > tegra_dma_init currently simply bails out early if any initialization fails.
>> > This skips various data-structure initialization. In turn, this means that
>> > tegra_dma_allocate_channel can still hand out channels. In this case, when
>> > tegra_dma_free_channel is called, which calls tegra_dma_cancel, the walking
>> > on ch->list will OOPS since the list's next/prev pointers may still be
>> > NULL.
>> >
>> > To solve this:
>> > * Mark all possible channels as in-use before doing anything else in init.
>> > * Only mark a channel as free once all channel-related initialization has
>> > ?completed.
>> >
>> > This prevents allocate_channel from handing out uninitialized channels.
>> >
>> > There is still one small hole; allocate_channel can't check the usage array
>> > for the shared channel, since this channel is permanently marked in-use.
>> > This could be solved using an explicit "init OK" flag that allocate_channel
>> > could check.
>>
>> If we still need an init complete flag, why not skip this patch and
>> just add that?
>
> I tend to like the cleanup in this patch; it simplifies the channel_usage
> initialization. I'll let you make the call on whether to take this or not.
Ok, it seems like a reasonable cleanup. Can I request a few changes?
Get rid of the redundant memsets
Replace the initial for loop to set all the bits with bitmap_set
Might as well add the init complete flag to this patch as well. You
could also do it by checking the channel_usage bit for the shared
channel, and never setting it in tegra_dma_allocate_channel.
> I can certainly throw together a second patch, or modify this patch, to also
> use an init_ok flag to completely solve the shared channel case too though.
>
> --
> nvpublic
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-02-23 18:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-23 17:29 [PATCH] ARM: Tegra: DMA: Fail safe if initialization fails Stephen Warren
2011-02-23 17:29 ` Stephen Warren
[not found] ` <1298482170-3678-1-git-send-email-swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2011-02-23 17:38 ` Colin Cross
2011-02-23 17:38 ` Colin Cross
2011-02-23 17:54 ` Stephen Warren
2011-02-23 17:54 ` Stephen Warren
[not found] ` <74CDBE0F657A3D45AFBB94109FB122FF03112A5CF5-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org>
2011-02-23 18:39 ` Colin Cross
2011-02-23 18:39 ` Colin Cross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.