[PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[Lubomir Rintel]

Hi,

please consider applying these two patches. Thery are not strictly
necessary, but improve diagnostics in case the DT is faulty.

Thanks,
Lubo

[PATCH 1/2] irqchip/mmp: Safeguard against multiple root intc initialization

[Lubomir Rintel]

We can only handle one root ICU. Let's warn if someone messes up and adds
multiple in the devicetree.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/irqchip/irq-mmp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
index 4a74ac7b7c42e..f597d96409a1f 100644
--- a/drivers/irqchip/irq-mmp.c
+++ b/drivers/irqchip/irq-mmp.c
@@ -250,6 +250,11 @@ void __init icu_init_irq(void)
 	int irq;
 
 	max_icu_nr = 1;
+	if (mmp_icu_base) {
+		pr_err("ICU already initialized\n");
+		return;
+	}
+
 	mmp_icu_base = ioremap(0xd4282000, 0x1000);
 	icu_data[0].conf_enable = mmp_conf.conf_enable;
 	icu_data[0].conf_disable = mmp_conf.conf_disable;
@@ -273,6 +278,11 @@ void __init mmp2_init_icu(void)
 	int irq, end;
 
 	max_icu_nr = 8;
+	if (mmp_icu_base) {
+		pr_err("ICU already initialized\n");
+		return;
+	}
+
 	mmp_icu_base = ioremap(0xd4282000, 0x1000);
 	icu_data[0].conf_enable = mmp2_conf.conf_enable;
 	icu_data[0].conf_disable = mmp2_conf.conf_disable;
@@ -380,6 +390,11 @@ static int __init mmp_init_bases(struct device_node *node)
 		return ret;
 	}
 
+	if (mmp_icu_base) {
+		pr_err("ICU already initialized\n");
+		return -EEXIST;
+	}
+
 	mmp_icu_base = of_iomap(node, 0);
 	if (!mmp_icu_base) {
 		pr_err("Failed to get interrupt controller register\n");
-- 
2.24.1

[PATCH 2/2] irqchip/mmp: Avoid overflowing icu_data[]

[Lubomir Rintel]

In case someone messes up and adds too many interrupt muxes in the device
tree, icu_data would overflow into things that follow it in bss, likely
mmp_icu2_base and mmp_icu_base.

Let's add a safeguard, so that we fail gracefully instead of panicking
at some later point for reasons that are then not immediately clear.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/irqchip/irq-mmp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
index f597d96409a1f..c7ef2cea1c020 100644
--- a/drivers/irqchip/irq-mmp.c
+++ b/drivers/irqchip/irq-mmp.c
@@ -505,6 +505,11 @@ static int __init mmp2_mux_of_init(struct device_node *node,
 		return -ENODEV;
 
 	i = max_icu_nr;
+	if (i >= MAX_ICU_NR) {
+		pr_err("Too many interrupt muxes\n");
+		return -EINVAL;
+	}
+
 	ret = of_property_read_u32(node, "mrvl,intc-nr-irqs",
 				   &nr_irqs);
 	if (ret) {
-- 
2.24.1

Re: [PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[Marc Zyngier]

On Wed, 19 Feb 2020 09:00:22 +0100
Lubomir Rintel <lkundrak@v3.sk> wrote:

[+RobH]

Lubomir,

> Hi,
> 
> please consider applying these two patches. Thery are not strictly
> necessary, but improve diagnostics in case the DT is faulty.

Can't we instead make sure our DT infrastructure checks for these? I'm
very reluctant to add more "DT validation" to the kernel, as it feels
like the wrong place to do this.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[Lubomir Rintel]

On Sun, Mar 08, 2020 at 02:04:34PM +0000, Marc Zyngier wrote:
> On Wed, 19 Feb 2020 09:00:22 +0100
> Lubomir Rintel <lkundrak@v3.sk> wrote:
> 
> [+RobH]
> 
> Lubomir,
> 
> > Hi,
> > 
> > please consider applying these two patches. Thery are not strictly
> > necessary, but improve diagnostics in case the DT is faulty.
> 
> Can't we instead make sure our DT infrastructure checks for these? I'm
> very reluctant to add more "DT validation" to the kernel, as it feels
> like the wrong place to do this.

These are not really problems of the DT infrastructure. It's that the
driver has some constrains resulting from use of global data ([PATCH 1])
and statically sized arrays ([PATCH 2]) without enforcing them.

It's probably easier to mess up DT than to mess up board files, but
regardless of that, being a little defensive and checking the bounds of
arrays is probably a good programming practice anyways.

Thank you
Lubo

Re: [PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[Marc Zyngier]

On Sun, 08 Mar 2020 14:46:04 +0000,
Lubomir Rintel <lkundrak@v3.sk> wrote:
> 
> On Sun, Mar 08, 2020 at 02:04:34PM +0000, Marc Zyngier wrote:
> > On Wed, 19 Feb 2020 09:00:22 +0100
> > Lubomir Rintel <lkundrak@v3.sk> wrote:
> > 
> > [+RobH]
> > 
> > Lubomir,
> > 
> > > Hi,
> > > 
> > > please consider applying these two patches. Thery are not strictly
> > > necessary, but improve diagnostics in case the DT is faulty.
> > 
> > Can't we instead make sure our DT infrastructure checks for these? I'm
> > very reluctant to add more "DT validation" to the kernel, as it feels
> > like the wrong place to do this.
> 
> These are not really problems of the DT infrastructure.

They are. The DT bindings describes the constraints (or at least
should), and the DT infrastructure could, at least in theory, check
them at compile time. Adding the checks to the kernel defeats the
single benefit of DT, which is independence from the kernel.

> It's that the driver has some constrains resulting from use of
> global data ([PATCH 1]) and statically sized arrays ([PATCH 2])
> without enforcing them.
> 
> It's probably easier to mess up DT than to mess up board files,

No, both models can be just as easily broken if people write them
without thinking twice.

> but regardless of that, being a little defensive and checking the
> bounds of arrays is probably a good programming practice anyways.

Is there even any example of such broken DT in the tree?

	M.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[Lubomir Rintel]

On Sun, Mar 08, 2020 at 05:26:35PM +0000, Marc Zyngier wrote:
> On Sun, 08 Mar 2020 14:46:04 +0000,
> Lubomir Rintel <lkundrak@v3.sk> wrote:
> > 
> > On Sun, Mar 08, 2020 at 02:04:34PM +0000, Marc Zyngier wrote:
> > > On Wed, 19 Feb 2020 09:00:22 +0100
> > > Lubomir Rintel <lkundrak@v3.sk> wrote:
> > > 
> > > [+RobH]
> > > 
> > > Lubomir,
> > > 
> > > > Hi,
> > > > 
> > > > please consider applying these two patches. Thery are not strictly
> > > > necessary, but improve diagnostics in case the DT is faulty.
> > > 
> > > Can't we instead make sure our DT infrastructure checks for these? I'm
> > > very reluctant to add more "DT validation" to the kernel, as it feels
> > > like the wrong place to do this.
> > 
> > These are not really problems of the DT infrastructure.
> 
> They are. The DT bindings describes the constraints (or at least
> should), and the DT infrastructure could, at least in theory, check
> them at compile time. Adding the checks to the kernel defeats the
> single benefit of DT, which is independence from the kernel.
> 
> > It's that the driver has some constrains resulting from use of
> > global data ([PATCH 1]) and statically sized arrays ([PATCH 2])
> > without enforcing them.
> > 
> > It's probably easier to mess up DT than to mess up board files,
> 
> No, both models can be just as easily broken if people write them
> without thinking twice.
> 
> > but regardless of that, being a little defensive and checking the
> > bounds of arrays is probably a good programming practice anyways.
> 
> Is there even any example of such broken DT in the tree?

No, this didn't occur with a FDT build from the kernel tree.

The device tree from Open Firmware that is used on the OLPC XO-4
machine is broken in this way (but it also needs many more fixes in
order to be able to run mainline kernels).

Lubo

> 
> 	M.
> 
> -- 
> Jazz is not dead, it just smells funny.

Re: [PATCH 0/2] irqchip/mmp: A pair of robustness fixed

[Rob Herring]

On Sun, Mar 8, 2020 at 9:04 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 19 Feb 2020 09:00:22 +0100
> Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> [+RobH]
>
> Lubomir,
>
> > Hi,
> >
> > please consider applying these two patches. Thery are not strictly
> > necessary, but improve diagnostics in case the DT is faulty.
>
> Can't we instead make sure our DT infrastructure checks for these? I'm
> very reluctant to add more "DT validation" to the kernel, as it feels
> like the wrong place to do this.

We don't really have a way to say a binding can only occur once or N
times in a DT. We'd have to have an SoC schema that listed out all the
nodes in the DT and forbid any additional nodes. I don't think that's
too useful as if there's only 1 instance for a given schema, then the
schema is not too useful as the schema has a equal chance of being
wrong.

Is there something inherent about the h/w that prevents more than one
instance? If support of more than one instance is a kernel limitation
(because no current SoC needs more than 1), then shouldn't the kernel
protect against this?

Rob