[PATCH] cxl: use correct operator when writing pcie config space values

[PATCH] cxl: use correct operator when writing pcie config space values

[Andrew Donnellan]

When writing a value to config space, cxl_pcie_write_config() calls
cxl_pcie_config_info() to obtain a mask and shift value, shifts the new
value accordingly, then uses the mask to combine the shifted value with the
existing value at the address as part of a read-modify-write pattern.

Currently, we use a logical OR operator rather than a bitwise OR operator,
which means any use of this function results in an incorrect value being
written. Replace the logical OR operator with a bitwise OR operator so the
value is written correctly.

Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: stable@vger.kernel.org
Fixes: 6f7f0b3df6d4 ("cxl: Add AFU virtual PHB and kernel API")
Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
 drivers/misc/cxl/vphb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
index 94b5208..9be09bb 100644
--- a/drivers/misc/cxl/vphb.c
+++ b/drivers/misc/cxl/vphb.c
@@ -203,7 +203,7 @@ static int cxl_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
 	mask <<= shift;
 	val <<= shift;
 
-	v = (in_le32(ioaddr) & ~mask) || (val & mask);
+	v = (in_le32(ioaddr) & ~mask) | (val & mask);
 
 	out_le32(ioaddr, v);
 	return PCIBIOS_SUCCESSFUL;
-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Ian Munsie]

Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

> When writing a value to config space, cxl_pcie_write_config() calls
> cxl_pcie_config_info() to obtain a mask and shift value, shifts the new
> value accordingly, then uses the mask to combine the shifted value with the
> existing value at the address as part of a read-modify-write pattern.
>
> Currently, we use a logical OR operator rather than a bitwise OR operator,
> which means any use of this function results in an incorrect value being
> written. Replace the logical OR operator with a bitwise OR operator so the
> value is written correctly.
>
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: stable@vger.kernel.org

Given that there are no current users of this function, does this need
to go to stable? Does it actually fix a real (as opposed to theoretical)
bug?

Regards,
Daniel

> Fixes: 6f7f0b3df6d4 ("cxl: Add AFU virtual PHB and kernel API")
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> ---
>  drivers/misc/cxl/vphb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
> index 94b5208..9be09bb 100644
> --- a/drivers/misc/cxl/vphb.c
> +++ b/drivers/misc/cxl/vphb.c
> @@ -203,7 +203,7 @@ static int cxl_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
>  	mask <<= shift;
>  	val <<= shift;
>  
> -	v = (in_le32(ioaddr) & ~mask) || (val & mask);
> +	v = (in_le32(ioaddr) & ~mask) | (val & mask);
>  
>  	out_le32(ioaddr, v);
>  	return PCIBIOS_SUCCESSFUL;
> -- 
> Andrew Donnellan              Software Engineer, OzLabs
> andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
> +61 2 6201 8874 (work)        IBM Australia Limited
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 859 bytes --]

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Michael Neuling]

On Fri, 2015-11-06 at 10:05 +1100, Daniel Axtens wrote:
> Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
>=20
> > When writing a value to config space, cxl_pcie_write_config() calls
> > cxl_pcie_config_info() to obtain a mask and shift value, shifts the
> > new
> > value accordingly, then uses the mask to combine the shifted value
> > with the
> > existing value at the address as part of a read-modify-write
> > pattern.
> >=20
> > Currently, we use a logical OR operator rather than a bitwise OR
> > operator,
> > which means any use of this function results in an incorrect value
> > being
> > written. Replace the logical OR operator with a bitwise OR operator
> > so the
> > value is written correctly.
> >=20
> > Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: stable@vger.kernel.org
>=20
> Given that there are no current users of this function, does this
> need
> to go to stable? Does it actually fix a real (as opposed to
> theoretical)
> bug?

Agreed.  Not really needed in stable.

Mikey

>=20
> Regards,
> Daniel
>=20
> > Fixes: 6f7f0b3df6d4 ("cxl: Add AFU virtual PHB and kernel API")
> > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> > ---
> >  drivers/misc/cxl/vphb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
> > index 94b5208..9be09bb 100644
> > --- a/drivers/misc/cxl/vphb.c
> > +++ b/drivers/misc/cxl/vphb.c
> > @@ -203,7 +203,7 @@ static int cxl_pcie_write_config(struct pci_bus
> > *bus, unsigned int devfn,
> >  	mask <<=3D shift;
> >  	val <<=3D shift;
> > =20
> > -	v =3D (in_le32(ioaddr) & ~mask) || (val & mask);
> > +	v =3D (in_le32(ioaddr) & ~mask) | (val & mask);
> > =20
> >  	out_le32(ioaddr, v);
> >  	return PCIBIOS_SUCCESSFUL;

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Andrew Donnellan]

On 06/11/15 10:05, Daniel Axtens wrote:
> Given that there are no current users of this function, does this need
> to go to stable? Does it actually fix a real (as opposed to theoretical)
> bug?

I tagged it for stable on mpe's request - I'm not fussed either way.


Andrew

-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Michael Ellerman]

On Fri, 2015-11-06 at 10:05 +1100, Daniel Axtens wrote:

> Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
> 

> > When writing a value to config space, cxl_pcie_write_config() calls
> > cxl_pcie_config_info() to obtain a mask and shift value, shifts the new
> > value accordingly, then uses the mask to combine the shifted value with the
> > existing value at the address as part of a read-modify-write pattern.
> > 
> > Currently, we use a logical OR operator rather than a bitwise OR operator,
> > which means any use of this function results in an incorrect value being
> > written. Replace the logical OR operator with a bitwise OR operator so the
> > value is written correctly.
> > 
> > Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: stable@vger.kernel.org
> 
> Given that there are no current users of this function, does this need
> to go to stable? Does it actually fix a real (as opposed to theoretical)
> bug?

If it's unused *and* broken then we should just remove it.

cheers

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Andrew Donnellan]

On 06/11/15 10:43, Michael Ellerman wrote:
> If it's unused *and* broken then we should just remove it.

Following some discussion with Ian and Vaibhav, we'd like to keep it at 
this stage - while there are no current AFUs which write to AFU config 
space, it would be reasonable for an AFU developer to assume that AFU 
config space is writable, and I am led to believe there is nothing in 
the publicly available documentation that would suggest otherwise.

Ian is working on obtaining a test AFU with support for this, at which 
point we will add it to our internal test suite.

As such I'd like to continue with the patch as is, I'm happy to drop the 
stable Cc though.


Andrew

-- 
Andrew Donnellan              Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
+61 2 6201 8874 (work)        IBM Australia Limited

Re: [PATCH] cxl: use correct operator when writing pcie config space values

[Daniel Axtens]

Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:

> On 06/11/15 10:43, Michael Ellerman wrote:
>> If it's unused *and* broken then we should just remove it.
>
> Following some discussion with Ian and Vaibhav, we'd like to keep it at 
> this stage - while there are no current AFUs which write to AFU config 
> space, it would be reasonable for an AFU developer to assume that AFU 
> config space is writable, and I am led to believe there is nothing in 
> the publicly available documentation that would suggest otherwise.
>
> Ian is working on obtaining a test AFU with support for this, at which 
> point we will add it to our internal test suite.
>
> As such I'd like to continue with the patch as is, I'm happy to drop the 
> stable Cc though.

Sounds good to me.

Regards,
Daniel

>
>
> Andrew
>
> -- 
> Andrew Donnellan              Software Engineer, OzLabs
> andrew.donnellan@au1.ibm.com  Australia Development Lab, Canberra
> +61 2 6201 8874 (work)        IBM Australia Limited

Re: cxl: use correct operator when writing pcie config space values

[Michael Ellerman]

On Wed, 2015-04-11 at 02:24:09 UTC, Andrew Donnellan wrote:
> When writing a value to config space, cxl_pcie_write_config() calls
> cxl_pcie_config_info() to obtain a mask and shift value, shifts the new
> value accordingly, then uses the mask to combine the shifted value with the
> existing value at the address as part of a read-modify-write pattern.
> 
> Currently, we use a logical OR operator rather than a bitwise OR operator,
> which means any use of this function results in an incorrect value being
> written. Replace the logical OR operator with a bitwise OR operator so the
> value is written correctly.
> 
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: stable@vger.kernel.org
> Fixes: 6f7f0b3df6d4 ("cxl: Add AFU virtual PHB and kernel API")
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Acked-by: Ian Munsie <imunsie@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/48f0f6b717e314a30be121b6

cheers