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 <[email protected]>
---
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
Some device tree platform do not define their resources properly. i.e.
overlapping or repeated resources.
This patch mark all device tree resources as shareable.
In the future this should only be set for the platforms that have
problems.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/of/platform.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ddf8e42c9367..89cb502f8812 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -136,6 +136,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
pr_debug("not all legacy IRQ resources mapped for %s\n",
np->name);
+ for (i = 0; i < num_reg + num_irq; i++, res++)
+ dev->resource[i].flags |= IORESOURCE_SHARED;
}
dev->dev.of_node = of_node_get(np);
--
2.1.4
On Fri, Jun 5, 2015 at 5:51 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> Some device tree platform do not define their resources properly. i.e.
> overlapping or repeated resources.
>
> This patch mark all device tree resources as shareable.
>
> In the future this should only be set for the platforms that have
> problems.
I don't think we want to do this globally. This should be very rare
and we want to discourage any new cases.
Rob
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/of/platform.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ddf8e42c9367..89cb502f8812 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -136,6 +136,8 @@ struct platform_device *of_device_alloc(struct device_node *np,
> if (of_irq_to_resource_table(np, res, num_irq) != num_irq)
> pr_debug("not all legacy IRQ resources mapped for %s\n",
> np->name);
> + for (i = 0; i < num_reg + num_irq; i++, res++)
> + dev->resource[i].flags |= IORESOURCE_SHARED;
> }
>
> dev->dev.of_node = of_node_get(np);
> --
> 2.1.4
>
Hello Rob,
Thanks for your feedback!
On Fri, Jun 5, 2015 at 6:45 PM, Rob Herring <[email protected]> wrote:
> On Fri, Jun 5, 2015 at 5:51 AM, Ricardo Ribalda Delgado
> <[email protected]> wrote:
>> Some device tree platform do not define their resources properly. i.e.
>> overlapping or repeated resources.
>>
>> This patch mark all device tree resources as shareable.
>>
>> In the future this should only be set for the platforms that have
>> problems.
>
> I don't think we want to do this globally. This should be very rare
> and we want to discourage any new cases.
I just wanted to mimic the original behaviour. Unfortunately I have no
idea of what platform is broken. Grant needs to help us here :)
What do you think about the new flag? Does it make any sense for you?
Thanks!
On Fri, 5 Jun 2015 18:51:36 +0200
, Ricardo Ribalda Delgado <[email protected]>
wrote:
> Hello Rob,
>
> Thanks for your feedback!
>
> On Fri, Jun 5, 2015 at 6:45 PM, Rob Herring <[email protected]> wrote:
> > On Fri, Jun 5, 2015 at 5:51 AM, Ricardo Ribalda Delgado
> > <[email protected]> wrote:
> >> Some device tree platform do not define their resources properly. i.e.
> >> overlapping or repeated resources.
> >>
> >> This patch mark all device tree resources as shareable.
> >>
> >> In the future this should only be set for the platforms that have
> >> problems.
> >
> > I don't think we want to do this globally. This should be very rare
> > and we want to discourage any new cases.
>
> I just wanted to mimic the original behaviour. Unfortunately I have no
> idea of what platform is broken. Grant needs to help us here :)
I know of powerpc platforms that split the ethernet and mdio controllers
into separate nodes, even though they share a register block. Those ones
are broken (ie. mpc5200). I know there are ARM platforms that exhibited
the same behaviour, but I can't remember specifics at the moment.
> What do you think about the new flag? Does it make any sense for you?
I think I have another solution to the whole problem. IIUC, the problem
is the kernel crashes on unregistering resources. I've got a patch that
doesn't try to unregister resources that weren't registered in the first
place. It doesn't fix the problem of DT not registering the resources in
the first place, but it makes the unregister path safe.
I'll cc: you on the patch series.
g.
On Fri, 5 Jun 2015 12:51:17 +0200
, Ricardo Ribalda Delgado <[email protected]>
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 <[email protected]>
> ---
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
>
Hello Grant
On Mon, Jun 8, 2015 at 8:23 PM, Grant Likely <[email protected]> wrote:
> On Fri, 5 Jun 2015 12:51:17 +0200
> , Ricardo Ribalda Delgado <[email protected]>
> 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 <[email protected]>
>> ---
>
> 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
On Mon, 8 Jun 2015 22:02:06 +0200
, Ricardo Ribalda Delgado <[email protected]>
wrote:
> Hello Grant
>
> On Mon, Jun 8, 2015 at 8:23 PM, Grant Likely <[email protected]> wrote:
> > On Fri, 5 Jun 2015 12:51:17 +0200
> > , Ricardo Ribalda Delgado <[email protected]>
> > 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 <[email protected]>
> >> ---
> >
> > 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.
Actually, you need to provide the test case. You need to show that
you've thought through all the implications and corner cases on the
resource code. This is a non-trivial change to the how the resource code
works, and you need to demonstrate that your really understand the
implications of what you are doing.
Start with the example I pointed out. When a driver does a
request_mem_region(), which resource does it end up being a parent of if
the regions overlap? Can you write a unittest that demonstrates the code
has the correct behaviour? Will a driver end up getting the wrong
device's resource structure as the parent? (hint: yes it will)
> I can load and unload the device tree perfectly.
Merely making it work for your use-case isn't the issue. It's whether or
not making this change will break the core behavour of the resource
code.
g.
Hello Grant
On Tue, Jun 9, 2015 at 1:13 PM, Grant Likely <[email protected]> wrote:
>
> On Mon, 8 Jun 2015 22:02:06 +0200
> , Ricardo Ribalda Delgado <[email protected]>
> wrote:
> > Hello Grant
> >
> > On Mon, Jun 8, 2015 at 8:23 PM, Grant Likely <[email protected]> wrote:
> > > On Fri, 5 Jun 2015 12:51:17 +0200
> > > , Ricardo Ribalda Delgado <[email protected]>
> > > 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 <[email protected]>
> > >> ---
> > >
> > > 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.
>
> Actually, you need to provide the test case. You need to show that
> you've thought through all the implications and corner cases on the
> resource code. This is a non-trivial change to the how the resource code
> works, and you need to demonstrate that your really understand the
> implications of what you are doing.
On non broken platforms:
it will work exactly as it works today.
On broken platforms:
I have tried with duplicated devices: Both requesting the region via
devm_ioremap_resource
a0000000-dfffffff : PCI Bus 0000:00
b0000000-cfffffff : PCI Bus 0000:01
b0030000-b003ffff : /axi1/pcie_bridge_1
b0030000-b003ffff : /axi1/pcie_bridge
b0030000-b003ffff : /axi1/pcie_bridge
b0030000-b003ffff : /axi1/pcie_bridge_1
pcie_bridge_0: pcie_bridge {
#address-cells = <1>;
#size-cells = <1>;
compatible = "xlnx,axi-pcie-1.00.a";
reg = < 0x30030000 0x10000 >;
interrupt-parent = <&xps_intc_0>;
interrupts = < 0 2 >;
qtec,apertures =<
0x80000000 0x20000000
0xa0000000 0x20000000
0xc0000000 0x20000000
0xe0000000 0x20000000
>;
};
pcie_bridge_1: pcie_bridge_1 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "xlnx,axi-pcie-1.00.a";
reg = < 0x30030000 0x10000 >;
interrupt-parent = <&xps_intc_0>;
interrupts = < 31 2 >;
qtec,apertures =<
0x80000000 0x20000000
0xa0000000 0x20000000
0xc0000000 0x20000000
0xe0000000 0x20000000
>;
};
And for two devices requesting the same region via devm_ioremap_resource()
a0000000-dfffffff : PCI Bus 0000:00
b0000000-cfffffff : PCI Bus 0000:01
b0030000-b003ffff : /axi1/pcie_bridge
b0030000-b003ffff : /axi1/pcie_bridge
b0030000-b003ffff : /axi1/pcie_bridge
pcie_bridge_0: pcie_bridge {
#address-cells = <1>;
#size-cells = <1>;
compatible = "xlnx,axi-pcie-1.00.a";
reg = < 0x30030000 0x10000 >;
interrupt-parent = <&xps_intc_0>;
interrupts = < 0 2 >;
qtec,apertures =<
0x80000000 0x20000000
0xa0000000 0x20000000
0xc0000000 0x20000000
0xe0000000 0x20000000
>;
};
packer_0: packer_0{
#address-cells = <1>;
#size-cells = <1>;
compatible = "qtec,axi_matrix_packer-1.00.a";
reg = < 0x30060000 0x10000 >;
qtec,pcie_bridge= <&pcie_bridge_0>;
};
If you can think of any other corner case, please let me know, and I
will try it, now I have a setup for this.
>
>
> Start with the example I pointed out. When a driver does a
> request_mem_region(), which resource does it end up being a parent of if
> the regions overlap? Can you write a unittest that demonstrates the code
> has the correct behaviour? Will a driver end up getting the wrong
> device's resource structure as the parent? (hint: yes it will)
>
> > I can load and unload the device tree perfectly.
>
> Merely making it work for your use-case isn't the issue. It's whether or
> not making this change will break the core behavour of the resource
> code.
>
> g.
--
Ricardo Ribalda