* [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
@ 2016-02-09 11:04 Robin Murphy
2016-02-09 12:06 ` Marc Zyngier
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Robin Murphy @ 2016-02-09 11:04 UTC (permalink / raw)
To: robh+dt, frowand.list, grant.likely, devicetree
Cc: marc.zyngier, mark.rutland, david.daney, stuart.yoder,
linux-arm-kernel, linux-kernel, stable
The existing msi-map code is fine for shifting the entire RID space
upwards, but attempting finer-grained remapping reveals a bug. It turns
out that we are mistakenly treating the msi-base part as an offset, not
as a new base to remap onto, so things get squiffy when rid-base is
nonzero. Fix this, and at the same time add a sanity check against
having msi-map-mask clash with a nonzero rid-base, as that's another
thing one can easily get wrong.
CC: <stable@vger.kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/of/irq.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 7ee21ae..e7bfc17 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
msi_base = be32_to_cpup(msi_map + 2);
rid_len = be32_to_cpup(msi_map + 3);
+ if (rid_base & ~map_mask) {
+ dev_err(parent_dev,
+ "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-base (0x%x)\n",
+ map_mask, rid_base);
+ return rid_out;
+ }
+
msi_controller_node = of_find_node_by_phandle(phandle);
matched = (masked_rid >= rid_base &&
@@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
if (!matched)
return rid_out;
- rid_out = masked_rid + msi_base;
+ rid_out = masked_rid - rid_base + msi_base;
dev_dbg(dev,
"msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
dev_name(parent_dev), map_mask, rid_base, msi_base,
--
2.7.0.25.gfc10eb5.dirty
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 11:04 [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base Robin Murphy
@ 2016-02-09 12:06 ` Marc Zyngier
2016-02-09 15:56 ` Stuart Yoder
2016-02-09 17:03 ` David Daney
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2016-02-09 12:06 UTC (permalink / raw)
To: Robin Murphy, robh+dt, frowand.list, grant.likely, devicetree
Cc: mark.rutland, david.daney, stuart.yoder, linux-arm-kernel,
linux-kernel, stable
Hi Robin,
On 09/02/16 11:04, Robin Murphy wrote:
> The existing msi-map code is fine for shifting the entire RID space
> upwards, but attempting finer-grained remapping reveals a bug. It turns
> out that we are mistakenly treating the msi-base part as an offset, not
> as a new base to remap onto, so things get squiffy when rid-base is
> nonzero. Fix this, and at the same time add a sanity check against
> having msi-map-mask clash with a nonzero rid-base, as that's another
> thing one can easily get wrong.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Looks like Stuart and you both found the same bug at the same time:
https://lkml.org/lkml/2016/2/8/1066
but yours seem more correct to me (the rid_base masking in Stuart's
version seems odd).
> ---
> drivers/of/irq.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 7ee21ae..e7bfc17 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> msi_base = be32_to_cpup(msi_map + 2);
> rid_len = be32_to_cpup(msi_map + 3);
>
> + if (rid_base & ~map_mask) {
> + dev_err(parent_dev,
> + "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-base (0x%x)\n",
> + map_mask, rid_base);
> + return rid_out;
> + }
> +
> msi_controller_node = of_find_node_by_phandle(phandle);
>
> matched = (masked_rid >= rid_base &&
> @@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> if (!matched)
> return rid_out;
>
> - rid_out = masked_rid + msi_base;
> + rid_out = masked_rid - rid_base + msi_base;
> dev_dbg(dev,
> "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
> dev_name(parent_dev), map_mask, rid_base, msi_base,
>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 12:06 ` Marc Zyngier
@ 2016-02-09 15:56 ` Stuart Yoder
2016-02-09 16:08 ` Mark Rutland
0 siblings, 1 reply; 13+ messages in thread
From: Stuart Yoder @ 2016-02-09 15:56 UTC (permalink / raw)
To: Marc Zyngier, Robin Murphy, robh+dt@kernel.org,
frowand.list@gmail.com, grant.likely@linaro.org,
devicetree@vger.kernel.org
Cc: mark.rutland@arm.com, stable@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, david.daney@cavium.com
> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Tuesday, February 09, 2016 6:06 AM
> To: Robin Murphy <robin.murphy@arm.com>; robh+dt@kernel.org; frowand.list@gmail.com;
> grant.likely@linaro.org; devicetree@vger.kernel.org
> Cc: mark.rutland@arm.com; david.daney@cavium.com; Stuart Yoder <stuart.yoder@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org
> Subject: Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
>
> Hi Robin,
>
> On 09/02/16 11:04, Robin Murphy wrote:
> > The existing msi-map code is fine for shifting the entire RID space
> > upwards, but attempting finer-grained remapping reveals a bug. It turns
> > out that we are mistakenly treating the msi-base part as an offset, not
> > as a new base to remap onto, so things get squiffy when rid-base is
> > nonzero. Fix this, and at the same time add a sanity check against
> > having msi-map-mask clash with a nonzero rid-base, as that's another
> > thing one can easily get wrong.
> >
> > CC: <stable@vger.kernel.org>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>
> Looks like Stuart and you both found the same bug at the same time:
> https://lkml.org/lkml/2016/2/8/1066
>
> but yours seem more correct to me (the rid_base masking in Stuart's
> version seems odd).
>
> > ---
> > drivers/of/irq.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 7ee21ae..e7bfc17 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct
> device_node **np,
> > msi_base = be32_to_cpup(msi_map + 2);
> > rid_len = be32_to_cpup(msi_map + 3);
> >
> > + if (rid_base & ~map_mask) {
> > + dev_err(parent_dev,
> > + "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-
> base (0x%x)\n",
> > + map_mask, rid_base);
> > + return rid_out;
> > + }
> > +
> > msi_controller_node = of_find_node_by_phandle(phandle);
> >
> > matched = (masked_rid >= rid_base &&
> > @@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node
> **np,
> > if (!matched)
> > return rid_out;
> >
> > - rid_out = masked_rid + msi_base;
> > + rid_out = masked_rid - rid_base + msi_base;
> > dev_dbg(dev,
> > "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length:
> %08x, rid: %08x -> %08x\n",
> > dev_name(parent_dev), map_mask, rid_base, msi_base,
> >
This computation: masked_rid - rid_base
...doesn't seem right to me. We are taking a rid that
has been already masked and subtracting a rid base that has
not been masked. I don't see how you can combine masked
and unmasked values in the same calculation.
Say I have this msi mapping:
msi-map = <0x0100 &its 0x11 0x1>;
msi-map-mask = <0xff>;
masked_rid = 0x0
rid_base = 0x0100
msi_base = 0x11
masked_rid - rid_base is 0x0 - 0x0100...which does not
give the msi index/offset we want.
Correct final answer should be 0x11.
In my patch I masked the rid_base so it can be subtracted
from the masked_rid.
masked_rid_base = 0x00
msi_base + (masked_rid - masked_rid_base) = 0x11
Stuart
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 15:56 ` Stuart Yoder
@ 2016-02-09 16:08 ` Mark Rutland
2016-02-09 16:17 ` Robin Murphy
2016-02-09 16:53 ` Stuart Yoder
0 siblings, 2 replies; 13+ messages in thread
From: Mark Rutland @ 2016-02-09 16:08 UTC (permalink / raw)
To: Stuart Yoder
Cc: Marc Zyngier, Robin Murphy, robh+dt@kernel.org,
frowand.list@gmail.com, grant.likely@linaro.org,
devicetree@vger.kernel.org, david.daney@cavium.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
On Tue, Feb 09, 2016 at 03:56:55PM +0000, Stuart Yoder wrote:
>
> > -----Original Message-----
> > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > Sent: Tuesday, February 09, 2016 6:06 AM
> > To: Robin Murphy <robin.murphy@arm.com>; robh+dt@kernel.org; frowand.list@gmail.com;
> > grant.likely@linaro.org; devicetree@vger.kernel.org
> > Cc: mark.rutland@arm.com; david.daney@cavium.com; Stuart Yoder <stuart.yoder@nxp.com>;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > stable@vger.kernel.org
> > Subject: Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
> >
> > Hi Robin,
> >
> > On 09/02/16 11:04, Robin Murphy wrote:
> > > The existing msi-map code is fine for shifting the entire RID space
> > > upwards, but attempting finer-grained remapping reveals a bug. It turns
> > > out that we are mistakenly treating the msi-base part as an offset, not
> > > as a new base to remap onto, so things get squiffy when rid-base is
> > > nonzero. Fix this, and at the same time add a sanity check against
> > > having msi-map-mask clash with a nonzero rid-base, as that's another
> > > thing one can easily get wrong.
> > >
> > > CC: <stable@vger.kernel.org>
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >
> > Looks like Stuart and you both found the same bug at the same time:
> > https://lkml.org/lkml/2016/2/8/1066
> >
> > but yours seem more correct to me (the rid_base masking in Stuart's
> > version seems odd).
> >
> > > ---
> > > drivers/of/irq.c | 9 ++++++++-
> > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > > index 7ee21ae..e7bfc17 100644
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct
> > device_node **np,
> > > msi_base = be32_to_cpup(msi_map + 2);
> > > rid_len = be32_to_cpup(msi_map + 3);
> > >
> > > + if (rid_base & ~map_mask) {
> > > + dev_err(parent_dev,
> > > + "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-
> > base (0x%x)\n",
> > > + map_mask, rid_base);
> > > + return rid_out;
> > > + }
> > > +
> > > msi_controller_node = of_find_node_by_phandle(phandle);
> > >
> > > matched = (masked_rid >= rid_base &&
> > > @@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node
> > **np,
> > > if (!matched)
> > > return rid_out;
> > >
> > > - rid_out = masked_rid + msi_base;
> > > + rid_out = masked_rid - rid_base + msi_base;
> > > dev_dbg(dev,
> > > "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length:
> > %08x, rid: %08x -> %08x\n",
> > > dev_name(parent_dev), map_mask, rid_base, msi_base,
> > >
>
> This computation: masked_rid - rid_base
>
> ...doesn't seem right to me. We are taking a rid that
> has been already masked and subtracting a rid base that has
> not been masked.
The binding only mentions that the input RID is masked, not the base, so
that seems correct to me.
> I don't see how you can combine masked and unmasked values in the same
> calculation.
>
> Say I have this msi mapping:
>
> msi-map = <0x0100 &its 0x11 0x1>;
> msi-map-mask = <0xff>;
>
I'd say that this is an inconsistent set of properties, and it's
probably worth warning if we encounter this. There is no possible way
that rid-base can be encountered.
> masked_rid = 0x0
> rid_base = 0x0100
> msi_base = 0x11
>
> masked_rid - rid_base is 0x0 - 0x0100...which does not
> give the msi index/offset we want.
>
> Correct final answer should be 0x11.
You can unambiguously describe this with:
msi-map = <0x00 &its 0x11 0x1>;
msi-map-mask = <0xff>;
This is exactly the pattern we follow in example 2 in the binding
document.
> In my patch I masked the rid_base so it can be subtracted
> from the masked_rid.
>
> masked_rid_base = 0x00
>
> msi_base + (masked_rid - masked_rid_base) = 0x11
As above, I think that this is an inconsistent DT, and we should
warn/fail in that case.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 16:08 ` Mark Rutland
@ 2016-02-09 16:17 ` Robin Murphy
2016-02-09 18:19 ` Mark Rutland
2016-02-09 16:53 ` Stuart Yoder
1 sibling, 1 reply; 13+ messages in thread
From: Robin Murphy @ 2016-02-09 16:17 UTC (permalink / raw)
To: Mark Rutland, Stuart Yoder
Cc: Marc Zyngier, robh+dt@kernel.org, frowand.list@gmail.com,
grant.likely@linaro.org, devicetree@vger.kernel.org,
david.daney@cavium.com, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
On 09/02/16 16:08, Mark Rutland wrote:
[...]
>>>> having msi-map-mask clash with a nonzero rid-base, as that's another
>>>> thing one can easily get wrong.
[...]
>>>> + if (rid_base & ~map_mask) {
>>>> + dev_err(parent_dev,
>>>> + "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-
>>> base (0x%x)\n",
>>>> + map_mask, rid_base);
>>>> + return rid_out;
>>>> + }
[...]
>> msi-map = <0x0100 &its 0x11 0x1>;
>> msi-map-mask = <0xff>;
>>
>
> I'd say that this is an inconsistent set of properties, and it's
> probably worth warning if we encounter this. There is no possible way
> that rid-base can be encountered.
Indeed ;)
Robin.
> Thanks,
> Mark.
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 16:08 ` Mark Rutland
2016-02-09 16:17 ` Robin Murphy
@ 2016-02-09 16:53 ` Stuart Yoder
1 sibling, 0 replies; 13+ messages in thread
From: Stuart Yoder @ 2016-02-09 16:53 UTC (permalink / raw)
To: Mark Rutland
Cc: Marc Zyngier, Robin Murphy, robh+dt@kernel.org,
frowand.list@gmail.com, grant.likely@linaro.org,
devicetree@vger.kernel.org, david.daney@cavium.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Tuesday, February 09, 2016 10:08 AM
> To: Stuart Yoder <stuart.yoder@nxp.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>; Robin Murphy <robin.murphy@arm.com>;
> robh+dt@kernel.org; frowand.list@gmail.com; grant.likely@linaro.org;
> devicetree@vger.kernel.org; david.daney@cavium.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
>
> On Tue, Feb 09, 2016 at 03:56:55PM +0000, Stuart Yoder wrote:
> >
> > > -----Original Message-----
> > > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > > Sent: Tuesday, February 09, 2016 6:06 AM
> > > To: Robin Murphy <robin.murphy@arm.com>; robh+dt@kernel.org; frowand.list@gmail.com;
> > > grant.likely@linaro.org; devicetree@vger.kernel.org
> > > Cc: mark.rutland@arm.com; david.daney@cavium.com; Stuart Yoder
> <stuart.yoder@nxp.com>;
> > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > stable@vger.kernel.org
> > > Subject: Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
> > >
> > > Hi Robin,
> > >
> > > On 09/02/16 11:04, Robin Murphy wrote:
> > > > The existing msi-map code is fine for shifting the entire RID space
> > > > upwards, but attempting finer-grained remapping reveals a bug. It turns
> > > > out that we are mistakenly treating the msi-base part as an offset, not
> > > > as a new base to remap onto, so things get squiffy when rid-base is
> > > > nonzero. Fix this, and at the same time add a sanity check against
> > > > having msi-map-mask clash with a nonzero rid-base, as that's another
> > > > thing one can easily get wrong.
> > > >
> > > > CC: <stable@vger.kernel.org>
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > >
> > > Looks like Stuart and you both found the same bug at the same time:
> > > https://lkml.org/lkml/2016/2/8/1066
> > >
> > > but yours seem more correct to me (the rid_base masking in Stuart's
> > > version seems odd).
> > >
> > > > ---
> > > > drivers/of/irq.c | 9 ++++++++-
> > > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > > > index 7ee21ae..e7bfc17 100644
> > > > --- a/drivers/of/irq.c
> > > > +++ b/drivers/of/irq.c
> > > > @@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct
> > > device_node **np,
> > > > msi_base = be32_to_cpup(msi_map + 2);
> > > > rid_len = be32_to_cpup(msi_map + 3);
> > > >
> > > > + if (rid_base & ~map_mask) {
> > > > + dev_err(parent_dev,
> > > > + "Invalid msi-map translation - msi-map-mask (0x%x) ignores
> rid-
> > > base (0x%x)\n",
> > > > + map_mask, rid_base);
> > > > + return rid_out;
> > > > + }
> > > > +
> > > > msi_controller_node = of_find_node_by_phandle(phandle);
> > > >
> > > > matched = (masked_rid >= rid_base &&
> > > > @@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct
> device_node
> > > **np,
> > > > if (!matched)
> > > > return rid_out;
> > > >
> > > > - rid_out = masked_rid + msi_base;
> > > > + rid_out = masked_rid - rid_base + msi_base;
> > > > dev_dbg(dev,
> > > > "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x,
> length:
> > > %08x, rid: %08x -> %08x\n",
> > > > dev_name(parent_dev), map_mask, rid_base, msi_base,
> > > >
> >
> > This computation: masked_rid - rid_base
> >
> > ...doesn't seem right to me. We are taking a rid that
> > has been already masked and subtracting a rid base that has
> > not been masked.
>
> The binding only mentions that the input RID is masked, not the base, so
> that seems correct to me.
>
> > I don't see how you can combine masked and unmasked values in the same
> > calculation.
> >
> > Say I have this msi mapping:
> >
> > msi-map = <0x0100 &its 0x11 0x1>;
> > msi-map-mask = <0xff>;
> >
>
> I'd say that this is an inconsistent set of properties, and it's
> probably worth warning if we encounter this. There is no possible way
> that rid-base can be encountered.
>
> > masked_rid = 0x0
> > rid_base = 0x0100
> > msi_base = 0x11
> >
> > masked_rid - rid_base is 0x0 - 0x0100...which does not
> > give the msi index/offset we want.
> >
> > Correct final answer should be 0x11.
>
> You can unambiguously describe this with:
>
> msi-map = <0x00 &its 0x11 0x1>;
> msi-map-mask = <0xff>;
>
> This is exactly the pattern we follow in example 2 in the binding
> document.
>
> > In my patch I masked the rid_base so it can be subtracted
> > from the masked_rid.
> >
> > masked_rid_base = 0x00
> >
> > msi_base + (masked_rid - masked_rid_base) = 0x11
>
> As above, I think that this is an inconsistent DT, and we should
> warn/fail in that case.
Thanks...understand now. I'll test Robin's patch and confirm
that it works as is for me.
Thanks,
Stuart
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 11:04 [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base Robin Murphy
2016-02-09 12:06 ` Marc Zyngier
@ 2016-02-09 17:03 ` David Daney
[not found] ` <9f6845195d03b0e0b0d187bb510fbf7bd497e836.1455015344.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
2016-02-11 11:04 ` Marc Zyngier
3 siblings, 0 replies; 13+ messages in thread
From: David Daney @ 2016-02-09 17:03 UTC (permalink / raw)
To: Robin Murphy
Cc: robh+dt, frowand.list, grant.likely, devicetree, marc.zyngier,
mark.rutland, david.daney, stuart.yoder, linux-arm-kernel,
linux-kernel, stable
On 02/09/2016 03:04 AM, Robin Murphy wrote:
> The existing msi-map code is fine for shifting the entire RID space
> upwards, but attempting finer-grained remapping reveals a bug. It turns
> out that we are mistakenly treating the msi-base part as an offset, not
> as a new base to remap onto, so things get squiffy when rid-base is
> nonzero. Fix this, and at the same time add a sanity check against
> having msi-map-mask clash with a nonzero rid-base, as that's another
> thing one can easily get wrong.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
This is equivalent to what I tested yesterday. Thanks for fixing it...
Acked-by: David Daney <david.daney@cavium.com>
> ---
> drivers/of/irq.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 7ee21ae..e7bfc17 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> msi_base = be32_to_cpup(msi_map + 2);
> rid_len = be32_to_cpup(msi_map + 3);
>
> + if (rid_base & ~map_mask) {
> + dev_err(parent_dev,
> + "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-base (0x%x)\n",
> + map_mask, rid_base);
> + return rid_out;
> + }
> +
> msi_controller_node = of_find_node_by_phandle(phandle);
>
> matched = (masked_rid >= rid_base &&
> @@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> if (!matched)
> return rid_out;
>
> - rid_out = masked_rid + msi_base;
> + rid_out = masked_rid - rid_base + msi_base;
> dev_dbg(dev,
> "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
> dev_name(parent_dev), map_mask, rid_base, msi_base,
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
[not found] ` <9f6845195d03b0e0b0d187bb510fbf7bd497e836.1455015344.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
@ 2016-02-09 18:12 ` Stuart Yoder
0 siblings, 0 replies; 13+ messages in thread
From: Stuart Yoder @ 2016-02-09 18:12 UTC (permalink / raw)
To: Robin Murphy, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org]
> Sent: Tuesday, February 09, 2016 5:05 AM
> To: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
> devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: marc.zyngier-5wv7dgnIgG8@public.gmane.org; mark.rutland-5wv7dgnIgG8@public.gmane.org; david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org; Stuart Yoder
> <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
>
> The existing msi-map code is fine for shifting the entire RID space
> upwards, but attempting finer-grained remapping reveals a bug. It turns
> out that we are mistakenly treating the msi-base part as an offset, not
> as a new base to remap onto, so things get squiffy when rid-base is
> nonzero. Fix this, and at the same time add a sanity check against
> having msi-map-mask clash with a nonzero rid-base, as that's another
> thing one can easily get wrong.
>
> CC: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> drivers/of/irq.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 7ee21ae..e7bfc17 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -635,6 +635,13 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node
> **np,
> msi_base = be32_to_cpup(msi_map + 2);
> rid_len = be32_to_cpup(msi_map + 3);
>
> + if (rid_base & ~map_mask) {
> + dev_err(parent_dev,
> + "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-
> base (0x%x)\n",
> + map_mask, rid_base);
> + return rid_out;
> + }
> +
> msi_controller_node = of_find_node_by_phandle(phandle);
>
> matched = (masked_rid >= rid_base &&
> @@ -654,7 +661,7 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node
> **np,
> if (!matched)
> return rid_out;
>
> - rid_out = masked_rid + msi_base;
> + rid_out = masked_rid - rid_base + msi_base;
> dev_dbg(dev,
> "msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length:
> %08x, rid: %08x -> %08x\n",
> dev_name(parent_dev), map_mask, rid_base, msi_base,
> --
> 2.7.0.25.gfc10eb5.dirty
Tested-by: Stuart Yoder <stuart.yoder-3arQi8VN3Tc@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 16:17 ` Robin Murphy
@ 2016-02-09 18:19 ` Mark Rutland
0 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2016-02-09 18:19 UTC (permalink / raw)
To: Robin Murphy
Cc: Stuart Yoder, Marc Zyngier, robh+dt@kernel.org,
frowand.list@gmail.com, grant.likely@linaro.org,
devicetree@vger.kernel.org, david.daney@cavium.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
On Tue, Feb 09, 2016 at 04:17:33PM +0000, Robin Murphy wrote:
> On 09/02/16 16:08, Mark Rutland wrote:
> [...]
>
> >>>>having msi-map-mask clash with a nonzero rid-base, as that's another
> >>>>thing one can easily get wrong.
>
> [...]
>
> >>>>+ if (rid_base & ~map_mask) {
> >>>>+ dev_err(parent_dev,
> >>>>+ "Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-
> >>>base (0x%x)\n",
> >>>>+ map_mask, rid_base);
> >>>>+ return rid_out;
> >>>>+ }
>
> [...]
>
> >> msi-map = <0x0100 &its 0x11 0x1>;
> >> msi-map-mask = <0xff>;
> >>
> >
> >I'd say that this is an inconsistent set of properties, and it's
> >probably worth warning if we encounter this. There is no possible way
> >that rid-base can be encountered.
>
> Indeed ;)
Ah!
FWIW:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Though it would be nice if we could fail the translation entirely rather
than just logging an error and idmapping the rid.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-09 11:04 [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base Robin Murphy
` (2 preceding siblings ...)
[not found] ` <9f6845195d03b0e0b0d187bb510fbf7bd497e836.1455015344.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
@ 2016-02-11 11:04 ` Marc Zyngier
2016-02-11 18:10 ` Frank Rowand
[not found] ` <56BC6ABA.5020605-5wv7dgnIgG8@public.gmane.org>
3 siblings, 2 replies; 13+ messages in thread
From: Marc Zyngier @ 2016-02-11 11:04 UTC (permalink / raw)
To: Robin Murphy, robh+dt, frowand.list, grant.likely, devicetree
Cc: mark.rutland, david.daney, stuart.yoder, linux-arm-kernel,
linux-kernel, stable
On 09/02/16 11:04, Robin Murphy wrote:
> The existing msi-map code is fine for shifting the entire RID space
> upwards, but attempting finer-grained remapping reveals a bug. It turns
> out that we are mistakenly treating the msi-base part as an offset, not
> as a new base to remap onto, so things get squiffy when rid-base is
> nonzero. Fix this, and at the same time add a sanity check against
> having msi-map-mask clash with a nonzero rid-base, as that's another
> thing one can easily get wrong.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Rob, Frank,
Are you willing to take this one through the OF tree? Or should we route
it through the IRQ tree? It'd be good if it make it into 4.5.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
2016-02-11 11:04 ` Marc Zyngier
@ 2016-02-11 18:10 ` Frank Rowand
[not found] ` <56BC6ABA.5020605-5wv7dgnIgG8@public.gmane.org>
1 sibling, 0 replies; 13+ messages in thread
From: Frank Rowand @ 2016-02-11 18:10 UTC (permalink / raw)
To: Marc Zyngier, robh+dt
Cc: Robin Murphy, grant.likely, devicetree, mark.rutland, david.daney,
stuart.yoder, linux-arm-kernel, linux-kernel, stable
On 2/11/2016 3:04 AM, Marc Zyngier wrote:
> On 09/02/16 11:04, Robin Murphy wrote:
>> The existing msi-map code is fine for shifting the entire RID space
>> upwards, but attempting finer-grained remapping reveals a bug. It turns
>> out that we are mistakenly treating the msi-base part as an offset, not
>> as a new base to remap onto, so things get squiffy when rid-base is
>> nonzero. Fix this, and at the same time add a sanity check against
>> having msi-map-mask clash with a nonzero rid-base, as that's another
>> thing one can easily get wrong.
>>
>> CC: <stable@vger.kernel.org>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>
> Rob, Frank,
>
> Are you willing to take this one through the OF tree? Or should we route
> it through the IRQ tree? It'd be good if it make it into 4.5.
>
> Thanks,
>
> M.
>
Just to be picky, I would like the patch to be split in two for easier
bisecting, but if Rob is happy with a single patch I'm ok with that.
Rob, will you pick this up?
Acked-by: Frank Rowand <frank.rowand@sonymobile.com>
-Frank
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
[not found] ` <56BC6ABA.5020605-5wv7dgnIgG8@public.gmane.org>
@ 2016-02-11 23:15 ` Rob Herring
[not found] ` <CAL_JsqLScMdfUAtMPDGWphc_PZp4K-c0MfjRZh9G7=A9pLZu9Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2016-02-11 23:15 UTC (permalink / raw)
To: Marc Zyngier
Cc: Robin Murphy, Frank Rowand, Grant Likely,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Rutland,
David Daney, stuart.yoder-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, stable
On Thu, Feb 11, 2016 at 5:04 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 09/02/16 11:04, Robin Murphy wrote:
>> The existing msi-map code is fine for shifting the entire RID space
>> upwards, but attempting finer-grained remapping reveals a bug. It turns
>> out that we are mistakenly treating the msi-base part as an offset, not
>> as a new base to remap onto, so things get squiffy when rid-base is
>> nonzero. Fix this, and at the same time add a sanity check against
>> having msi-map-mask clash with a nonzero rid-base, as that's another
>> thing one can easily get wrong.
>>
>> CC: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>
> Rob, Frank,
>
> Are you willing to take this one through the OF tree? Or should we route
> it through the IRQ tree? It'd be good if it make it into 4.5.
Applied for 4.5. ATM, I don't have anything else to send to Linus.
I'll give it a week or so if this is not urgent. Or send me a bunch of
DT binding fixes and you can get it in sooner. ;)
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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] 13+ messages in thread
* Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base
[not found] ` <CAL_JsqLScMdfUAtMPDGWphc_PZp4K-c0MfjRZh9G7=A9pLZu9Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-12 8:32 ` Marc Zyngier
0 siblings, 0 replies; 13+ messages in thread
From: Marc Zyngier @ 2016-02-12 8:32 UTC (permalink / raw)
To: Rob Herring
Cc: Robin Murphy, Frank Rowand, Grant Likely,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Rutland,
David Daney, stuart.yoder-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, stable
On 11/02/16 23:15, Rob Herring wrote:
> On Thu, Feb 11, 2016 at 5:04 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 09/02/16 11:04, Robin Murphy wrote:
>>> The existing msi-map code is fine for shifting the entire RID space
>>> upwards, but attempting finer-grained remapping reveals a bug. It turns
>>> out that we are mistakenly treating the msi-base part as an offset, not
>>> as a new base to remap onto, so things get squiffy when rid-base is
>>> nonzero. Fix this, and at the same time add a sanity check against
>>> having msi-map-mask clash with a nonzero rid-base, as that's another
>>> thing one can easily get wrong.
>>>
>>> CC: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>>
>> Rob, Frank,
>>
>> Are you willing to take this one through the OF tree? Or should we route
>> it through the IRQ tree? It'd be good if it make it into 4.5.
>
> Applied for 4.5. ATM, I don't have anything else to send to Linus.
> I'll give it a week or so if this is not urgent. Or send me a bunch of
> DT binding fixes and you can get it in sooner. ;)
Doesn't really qualify as "a bunch", but how about this one:
https://patchwork.ozlabs.org/patch/578268/
I had it in mind for 4.6, but the sooner the better!
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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] 13+ messages in thread
end of thread, other threads:[~2016-02-12 8:32 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-09 11:04 [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base Robin Murphy
2016-02-09 12:06 ` Marc Zyngier
2016-02-09 15:56 ` Stuart Yoder
2016-02-09 16:08 ` Mark Rutland
2016-02-09 16:17 ` Robin Murphy
2016-02-09 18:19 ` Mark Rutland
2016-02-09 16:53 ` Stuart Yoder
2016-02-09 17:03 ` David Daney
[not found] ` <9f6845195d03b0e0b0d187bb510fbf7bd497e836.1455015344.git.robin.murphy-5wv7dgnIgG8@public.gmane.org>
2016-02-09 18:12 ` Stuart Yoder
2016-02-11 11:04 ` Marc Zyngier
2016-02-11 18:10 ` Frank Rowand
[not found] ` <56BC6ABA.5020605-5wv7dgnIgG8@public.gmane.org>
2016-02-11 23:15 ` Rob Herring
[not found] ` <CAL_JsqLScMdfUAtMPDGWphc_PZp4K-c0MfjRZh9G7=A9pLZu9Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-12 8:32 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).