If the same resource is inserted to the resource tree
(maybe not on purpose), a dead loop will be created. In this situation,
The kernel does not report any warning or error :(
The command below will show a endless print.
#cat /proc/iomem
So, adding the check for the same resource is needed for the stability
and reliability of the kernel.
Signed-off-by: Huang Shijie <[email protected]>
---
kernel/resource.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 7b36976..60daab4 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -451,7 +451,7 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
if (!first)
return first;
- if (first == parent)
+ if (first == parent || first == new)
return first;
if ((first->start > new->start) || (first->end < new->end))
--
1.6.6.1
On Mon, 16 Aug 2010 23:55:38 +0800
Huang Shijie <[email protected]> wrote:
> If the same resource is inserted to the resource tree
> (maybe not on purpose), a dead loop will be created. In this situation,
> The kernel does not report any warning or error :(
>
> The command below will show a endless print.
> #cat /proc/iomem
OK, we shouldn't do that.
> So, adding the check for the same resource is needed for the stability
> and reliability of the kernel.
>
> Signed-off-by: Huang Shijie <[email protected]>
> ---
> kernel/resource.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 7b36976..60daab4 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -451,7 +451,7 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
> if (!first)
> return first;
>
> - if (first == parent)
> + if (first == parent || first == new)
> return first;
However, inserting the same thing twice _is_ a bug, and we shouldn't
silently accept it like this. We should tell the programmer!
But we can recover from the situation so let's not kill the box. How
does this look?
--- a/kernel/resource.c~kernel-resourcec-handle-reinsertion-of-an-already-inserted-resource
+++ a/kernel/resource.c
@@ -453,6 +453,8 @@ static struct resource * __insert_resour
if (first == parent)
return first;
+ if (WARN_ON(first == new)) /* duplicated insertion */
+ return first;
if ((first->start > new->start) || (first->end < new->end))
break;
_
On Wed, Aug 25, 2010 at 7:46 AM, Andrew Morton
<[email protected]> wrote:
> On Mon, 16 Aug 2010 23:55:38 +0800
> Huang Shijie <[email protected]> wrote:
>
>> If the same resource is inserted to the resource tree
>> (maybe not on purpose), a dead loop will be created. In this situation,
>> The kernel does not report any warning or error :(
>>
>> The command below will show a endless print.
>> #cat /proc/iomem
>
> OK, we shouldn't do that.
>
>> So, adding the check for the same resource is needed for the stability
>> and reliability of the kernel.
>>
>> Signed-off-by: Huang Shijie <[email protected]>
>> ---
>> kernel/resource.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 7b36976..60daab4 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -451,7 +451,7 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
>> if (!first)
>> return first;
>>
>> - if (first == parent)
>> + if (first == parent || first == new)
>> return first;
>
> However, inserting the same thing twice _is_ a bug, and we shouldn't
> silently accept it like this. We should tell the programmer!
Indeed, this is a bug caused by the driver programmer.I had spent
nearly two day to find it in my colleague's code.
>
> But we can recover from the situation so let's not kill the box. How
> does this look?
>
> --- a/kernel/resource.c~kernel-resourcec-handle-reinsertion-of-an-already-inserted-resource
> +++ a/kernel/resource.c
> @@ -453,6 +453,8 @@ static struct resource * __insert_resour
>
> if (first == parent)
> return first;
> + if (WARN_ON(first == new)) /* duplicated insertion */
> + return first;
>
Yes. This one is better.
Many drivers do not check the returned value of insert_resouce().
> if ((first->start > new->start) || (first->end < new->end))
> break;
> _
>
>
On Wed, Aug 25, 2010 at 7:46 AM, Andrew Morton
<[email protected]> wrote:
> On Mon, 16 Aug 2010 23:55:38 +0800
> Huang Shijie <[email protected]> wrote:
>
>> If the same resource is inserted to the resource tree
>> (maybe not on purpose), a dead loop will be created. In this situation,
>> The kernel does not report any warning or error :(
>>
>> The command below will show a endless print.
>> #cat /proc/iomem
>
> OK, we shouldn't do that.
>
>> So, adding the check for the same resource is needed for the stability
>> and reliability of the kernel.
>>
>> Signed-off-by: Huang Shijie <[email protected]>
>> ---
>> kernel/resource.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 7b36976..60daab4 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -451,7 +451,7 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
>> if (!first)
>> return first;
>>
>> - if (first == parent)
>> + if (first == parent || first == new)
>> return first;
>
> However, inserting the same thing twice _is_ a bug, and we shouldn't
> silently accept it like this. We should tell the programmer!
>
> But we can recover from the situation so let's not kill the box. How
> does this look?
>
> --- a/kernel/resource.c~kernel-resourcec-handle-reinsertion-of-an-already-inserted-resource
> +++ a/kernel/resource.c
> @@ -453,6 +453,8 @@ static struct resource * __insert_resour
>
> if (first == parent)
> return first;
> + if (WARN_ON(first == new)) /* duplicated insertion */
> + return first;
>
I think WARN_ON() is not power enough. If the CONFIG_BUG is not
defined, the warning message may not be printed. So I think using a
directly printk() here is better.
What about this ?
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -453,6 +453,11 @@ static struct resource * __insert_resource(struct
resource *parent, struct resou
if (first == parent)
return first;
+ if (first == new) {
+ printk(KERN_ERR "You should not insert the"
+ "same resource twice!\n");
+ return first;
+ }
if ((first->start > new->start) || (first->end < new->end))
break;
> if ((first->start > new->start) || (first->end < new->end))
> break;
> _
>
>
On Wed, 25 Aug 2010 13:03:55 +0800 Huang Shijie <[email protected]> wrote:
> On Wed, Aug 25, 2010 at 7:46 AM, Andrew Morton
> <[email protected]> wrote:
>
> I think WARN_ON() is not power enough. If the CONFIG_BUG is not
> defined, the warning message may not be printed. So I think using a
> directly printk() here is better.
No, WARN_ON() is good. It gives us a backtrace and identifies the
buggy code. Only maniacs turn off CONFIG_BUG.
On Wed, Aug 25, 2010 at 2:41 PM, Andrew Morton
<[email protected]> wrote:
> On Wed, 25 Aug 2010 13:03:55 +0800 Huang Shijie <[email protected]> wrote:
>
>> On Wed, Aug 25, 2010 at 7:46 AM, Andrew Morton
>> <[email protected]> wrote:
>>
>> I think WARN_ON() is not power enough. If the CONFIG_BUG is not
>> defined, the warning message may not be printed. So I think using a
>> directly printk() here is better.
>
> No, WARN_ON() is good. It gives us a backtrace and identifies the
> buggy code. Only maniacs turn off CONFIG_BUG.
>
got it. Thanks a lot.
On Tuesday, August 24, 2010 10:31:52 pm Huang Shijie wrote:
> On Wed, Aug 25, 2010 at 7:46 AM, Andrew Morton
> <[email protected]> wrote:
> > On Mon, 16 Aug 2010 23:55:38 +0800
> > Huang Shijie <[email protected]> wrote:
> >
> >> If the same resource is inserted to the resource tree
> >> (maybe not on purpose), a dead loop will be created. In this situation,
> >> The kernel does not report any warning or error :(
> >>
> >> The command below will show a endless print.
> >> #cat /proc/iomem
> >
> > OK, we shouldn't do that.
> >
> >> So, adding the check for the same resource is needed for the stability
> >> and reliability of the kernel.
> >>
> >> Signed-off-by: Huang Shijie <[email protected]>
> >> ---
> >> kernel/resource.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/kernel/resource.c b/kernel/resource.c
> >> index 7b36976..60daab4 100644
> >> --- a/kernel/resource.c
> >> +++ b/kernel/resource.c
> >> @@ -451,7 +451,7 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
> >> if (!first)
> >> return first;
> >>
> >> - if (first == parent)
> >> + if (first == parent || first == new)
> >> return first;
> >
> > However, inserting the same thing twice _is_ a bug, and we shouldn't
> > silently accept it like this. We should tell the programmer!
>
> Indeed, this is a bug caused by the driver programmer.I had spent
> nearly two day to find it in my colleague's code.
insert_resource() is a hack to deal with the fact that we don't
discover resources in the logical order, so sometimes we have to
go back and add a resource after we've already added "children"
of the new resource.
In my opinion, the ugliness of using insert_resource() should be
confined to core architecture code, and drivers shouldn't use it
at all.
That said, I think it's a good idea to add the WARN_ON().
In fact, I wonder if we should also "WARN_ON(first == parent)".
Bjorn