[PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Robin Murphy]

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

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Marc Zyngier]

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...

RE: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Stuart Yoder]

> -----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

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Mark Rutland]

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.

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Robin Murphy]

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.
>

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Mark Rutland]

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.

RE: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Stuart Yoder]

> -----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

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[David Daney]

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,
>

RE: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Stuart Yoder]

> -----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

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Marc Zyngier]

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...

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Frank Rowand]

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

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Rob Herring]

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

Re: [PATCH] of/irq: Fix msi-map calculation for nonzero rid-base

[Marc Zyngier]

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