Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1160704 > unrolled thread

Re: [PATCH 1/2] kernel/resource: Add new flag IORESOURCE_SHARED

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] kernel/resource: Add new flag IORESOURCE_SHARED Grant Likely <grant.likely@linaro.org> - 2015-06-08 20:30 +0200
    Re: [PATCH 1/2] kernel/resource: Add new flag IORESOURCE_SHARED Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2015-06-08 22:10 +0200

#1160704 — Re: [PATCH 1/2] kernel/resource: Add new flag IORESOURCE_SHARED

On Fri,  5 Jun 2015 12:51:17 +0200
, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
 wrote:
> Some device tree platforms have not defined correctly their memory
> resources (i.e. Overlapping or duplication of resources).
> To avoid this issue we have historically avoided to add their resources to
> the resource tree. This leads to code duplication and oops when trying to
> unload dynamically a device tree (feature introduced recently).
> 
> This new flag tells the resource system that a resource can be shared by
> multiple owners, so we can support device trees with problems at the
> same time that we do not duplicate code or crash when unloading the
> device tree.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

I'm really not comfortable with this change. The resource tree code is
complicated enough as is. Adding this exception case quite probably adds
corner cases that aren't property dealt with. If two regions overlay,
and then request_region is called? Which region does it become a child
of? And that's just off the top of my head. I don't want to hack in
changes to the resource code for what is a corner case.

g.

>  include/linux/ioport.h | 1 +
>  kernel/resource.c      | 6 ++++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 388e3ae94f7a..f4d992381529 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -49,6 +49,7 @@ struct resource {
>  #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
>  #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
>  
> +#define IORESOURCE_SHARED	0x04000000	/* Resource can be shared */
>  #define IORESOURCE_EXCLUSIVE	0x08000000	/* Userland may not map this resource */
>  #define IORESOURCE_DISABLED	0x10000000
>  #define IORESOURCE_UNSET	0x20000000	/* No address assigned yet */
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 90552aab5f2d..4a3626489b62 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -210,6 +210,7 @@ static struct resource * __request_resource(struct resource *root, struct resour
>  	resource_size_t start = new->start;
>  	resource_size_t end = new->end;
>  	struct resource *tmp, **p;
> +	bool root_shared = root && root->flags & IORESOURCE_SHARED;
>  
>  	if (end < start)
>  		return root;
> @@ -220,14 +221,15 @@ static struct resource * __request_resource(struct resource *root, struct resour
>  	p = &root->child;
>  	for (;;) {
>  		tmp = *p;
> -		if (!tmp || tmp->start > end) {
> +		if (!tmp || tmp->start > end ||
> +			(root_shared && tmp->start > start)) {
>  			new->sibling = tmp;
>  			*p = new;
>  			new->parent = root;
>  			return NULL;
>  		}
>  		p = &tmp->sibling;
> -		if (tmp->end < start)
> +		if (tmp->end < start || root_shared)
>  			continue;
>  		return tmp;
>  	}
> -- 
> 2.1.4
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1160752

Hello Grant

On Mon, Jun 8, 2015 at 8:23 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Fri,  5 Jun 2015 12:51:17 +0200
> , Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>  wrote:
>> Some device tree platforms have not defined correctly their memory
>> resources (i.e. Overlapping or duplication of resources).
>> To avoid this issue we have historically avoided to add their resources to
>> the resource tree. This leads to code duplication and oops when trying to
>> unload dynamically a device tree (feature introduced recently).
>>
>> This new flag tells the resource system that a resource can be shared by
>> multiple owners, so we can support device trees with problems at the
>> same time that we do not duplicate code or crash when unloading the
>> device tree.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>
> I'm really not comfortable with this change. The resource tree code is
> complicated enough as is. Adding this exception case quite probably adds
> corner cases that aren't property dealt with. If two regions overlay,
> and then request_region is called? Which region does it become a child
> of? And that's just off the top of my head. I don't want to hack in
> changes to the resource code for what is a corner case.

I see your concern, perhaps you could provide a testcase and we can
find out if it fails or not. So far I have tested a device tree with
two devices on the same memory region, each device managed by a
driver.

I can load and unload the device tree perfectly.

>
> g.
>
>>  include/linux/ioport.h | 1 +
>>  kernel/resource.c      | 6 ++++--
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index 388e3ae94f7a..f4d992381529 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -49,6 +49,7 @@ struct resource {
>>  #define IORESOURCE_WINDOW    0x00200000      /* forwarded by bridge */
>>  #define IORESOURCE_MUXED     0x00400000      /* Resource is software muxed */
>>
>> +#define IORESOURCE_SHARED    0x04000000      /* Resource can be shared */
>>  #define IORESOURCE_EXCLUSIVE 0x08000000      /* Userland may not map this resource */
>>  #define IORESOURCE_DISABLED  0x10000000
>>  #define IORESOURCE_UNSET     0x20000000      /* No address assigned yet */
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 90552aab5f2d..4a3626489b62 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -210,6 +210,7 @@ static struct resource * __request_resource(struct resource *root, struct resour
>>       resource_size_t start = new->start;
>>       resource_size_t end = new->end;
>>       struct resource *tmp, **p;
>> +     bool root_shared = root && root->flags & IORESOURCE_SHARED;
>>
>>       if (end < start)
>>               return root;
>> @@ -220,14 +221,15 @@ static struct resource * __request_resource(struct resource *root, struct resour
>>       p = &root->child;
>>       for (;;) {
>>               tmp = *p;
>> -             if (!tmp || tmp->start > end) {
>> +             if (!tmp || tmp->start > end ||
>> +                     (root_shared && tmp->start > start)) {
>>                       new->sibling = tmp;
>>                       *p = new;
>>                       new->parent = root;
>>                       return NULL;
>>               }
>>               p = &tmp->sibling;
>> -             if (tmp->end < start)
>> +             if (tmp->end < start || root_shared)
>>                       continue;
>>               return tmp;
>>       }
>> --
>> 2.1.4
>>
>



-- 
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web