[PATCH] cxl: sparse: add __iomem annotations in vphb.c

[PATCH] cxl: sparse: add __iomem annotations in vphb.c

[Andrew Donnellan]

sparse identifies the following issues:

  drivers/misc/cxl/vphb.c:131:17: warning: incorrect type in assignment
    (different address spaces)
  drivers/misc/cxl/vphb.c:131:17:    expected void volatile [noderef]
    <asn:2>*<noident>
  drivers/misc/cxl/vphb.c:131:17:    got void *<noident>
  drivers/misc/cxl/vphb.c:252:23: warning: incorrect type in assignment
    (different address spaces)
  drivers/misc/cxl/vphb.c:252:23:    expected void [noderef]
    <asn:2>*cfg_data
  drivers/misc/cxl/vphb.c:252:23:    got void *<noident>

Add __iomem annotations and remove unnecessary casts to clear up these
warnings.

Reported-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

---

This patch is a respin of https://patchwork.ozlabs.org/patch/504976/,
incorporating comments from mpe.

As with the old patch, this patch doesn't make any changes to the return
type of cxl_pcie_cfg_addr() and casts to an __iomem type when we use it in
cxl_pcie_config_info(). cxl_pcie_cfg_addr() returns an unsigned long,
rather than a pointer type, as we use its return value in bitwise
operations. If there's a better way to handle this I'd like to know.
---
 drivers/misc/cxl/vphb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/cxl/vphb.c b/drivers/misc/cxl/vphb.c
index c241e15..588bfc0 100644
--- a/drivers/misc/cxl/vphb.c
+++ b/drivers/misc/cxl/vphb.c
@@ -128,7 +128,7 @@ static int cxl_pcie_config_info(struct pci_bus *bus, unsigned int devfn,
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 	addr = cxl_pcie_cfg_addr(phb, bus->number, devfn, offset);
 
-	*ioaddr = (void *)(addr & ~0x3ULL);
+	*ioaddr = (void __iomem *)(addr & ~0x3ULL);
 	*shift = ((addr & 0x3) * 8);
 	switch (len) {
 	case 1:
@@ -249,7 +249,7 @@ int cxl_pci_vphb_add(struct cxl_afu *afu)
 	/* Setup the PHB using arch provided callback */
 	phb->ops = &cxl_pcie_pci_ops;
 	phb->cfg_addr = afu->afu_desc_mmio + afu->crs_offset;
-	phb->cfg_data = (void *)(u64)afu->crs_len;
+	phb->cfg_data = (void __iomem *)afu->crs_len;
 	phb->private_data = afu;
 	phb->controller_ops = cxl_pci_controller_ops;
 
-- 
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: sparse: add __iomem annotations in vphb.c

[Ian Munsie]

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

Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c

[Arnd Bergmann]

On Wednesday 28 October 2015 14:29:39 Andrew Donnellan wrote:
> --- a/drivers/misc/cxl/vphb.c
> +++ b/drivers/misc/cxl/vphb.c
> @@ -128,7 +128,7 @@ static int cxl_pcie_config_info(struct pci_bus *bus, unsigned int devfn,
>                 return PCIBIOS_BAD_REGISTER_NUMBER;
>         addr = cxl_pcie_cfg_addr(phb, bus->number, devfn, offset);
>  
> -       *ioaddr = (void *)(addr & ~0x3ULL);
> +       *ioaddr = (void __iomem *)(addr & ~0x3ULL);
>         *shift = ((addr & 0x3) * 8);
>         switch (len) {
>         case 1:

It would be nice to change the return value of cxl_pcie_cfg_addr to
'void __iomem *' as well, and only do the cast (back and forth) inside 
the calculation, to make it clear that the input type is the same as the
output. Perhaps use a static inline function to wrap it.

> @@ -249,7 +249,7 @@ int cxl_pci_vphb_add(struct cxl_afu *afu)
>         /* Setup the PHB using arch provided callback */
>         phb->ops = &cxl_pcie_pci_ops;
>         phb->cfg_addr = afu->afu_desc_mmio + afu->crs_offset;
> -       phb->cfg_data = (void *)(u64)afu->crs_len;
> +       phb->cfg_data = (void __iomem *)afu->crs_len;
>         phb->private_data = afu;
>         phb->controller_ops = cxl_pci_controller_ops;


This needs a comment to explain why this is correct. I've tried to find it
out by reading the code and could not find any explanation. Also, you
need to cast to an intermediate (uintptr_t) type, as directly converting
a u64 to a pointer of any sort is nonportable, and it would be good to
at least allow compile-testing this code on other architectures.

I suspect that 'phb->cfg_data' is used in this driver in a way that is
incompatible with the other uses of the same variable. Maybe you can
replace it with something like

	union {
		void __iomem *cfg_data;
		u64 cxl_cfg_offset;
	};

to make it clear that in this driver it is used as an offset rather than
a pointer.

	Arnd

Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c

[Andrew Donnellan]

On 31/10/15 00:07, Arnd Bergmann wrote:
> On Wednesday 28 October 2015 14:29:39 Andrew Donnellan wrote:
>> --- a/drivers/misc/cxl/vphb.c
>> +++ b/drivers/misc/cxl/vphb.c
>> @@ -128,7 +128,7 @@ static int cxl_pcie_config_info(struct pci_bus *bus, unsigned int devfn,
>>                  return PCIBIOS_BAD_REGISTER_NUMBER;
>>          addr = cxl_pcie_cfg_addr(phb, bus->number, devfn, offset);
>>
>> -       *ioaddr = (void *)(addr & ~0x3ULL);
>> +       *ioaddr = (void __iomem *)(addr & ~0x3ULL);
>>          *shift = ((addr & 0x3) * 8);
>>          switch (len) {
>>          case 1:
>
> It would be nice to change the return value of cxl_pcie_cfg_addr to
> 'void __iomem *' as well, and only do the cast (back and forth) inside
> the calculation, to make it clear that the input type is the same as the
> output. Perhaps use a static inline function to wrap it.

That would work, not sure if I'd bother with a wrapper function.

>> @@ -249,7 +249,7 @@ int cxl_pci_vphb_add(struct cxl_afu *afu)
>>          /* Setup the PHB using arch provided callback */
>>          phb->ops = &cxl_pcie_pci_ops;
>>          phb->cfg_addr = afu->afu_desc_mmio + afu->crs_offset;
>> -       phb->cfg_data = (void *)(u64)afu->crs_len;
>> +       phb->cfg_data = (void __iomem *)afu->crs_len;
>>          phb->private_data = afu;
>>          phb->controller_ops = cxl_pci_controller_ops;
>
>
> This needs a comment to explain why this is correct. I've tried to find it
> out by reading the code and could not find any explanation. Also, you
> need to cast to an intermediate (uintptr_t) type, as directly converting
> a u64 to a pointer of any sort is nonportable, and it would be good to
> at least allow compile-testing this code on other architectures.

It's impossible to compile cxl on other architectures given that we 
depend on powerpc- and powernv-specific functions... but in any case, I 
suppose using uintptr_t is more correct.

> I suspect that 'phb->cfg_data' is used in this driver in a way that is
> incompatible with the other uses of the same variable. Maybe you can
> replace it with something like
>
> 	union {
> 		void __iomem *cfg_data;
> 		u64 cxl_cfg_offset;
> 	};
>
> to make it clear that in this driver it is used as an offset rather than
> a pointer.

You're right, I hadn't looked closely at exactly how it was been used.

Ian, thoughts?


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: sparse: add __iomem annotations in vphb.c

[Michael Ellerman]

On Wed, 2015-10-28 at 14:29 +1100, Andrew Donnellan wrote:

> sparse identifies the following issues:
> 
>   drivers/misc/cxl/vphb.c:131:17: warning: incorrect type in assignment
>     (different address spaces)
>   drivers/misc/cxl/vphb.c:131:17:    expected void volatile [noderef]
>     <asn:2>*<noident>
>   drivers/misc/cxl/vphb.c:131:17:    got void *<noident>
>   drivers/misc/cxl/vphb.c:252:23: warning: incorrect type in assignment
>     (different address spaces)
>   drivers/misc/cxl/vphb.c:252:23:    expected void [noderef]
>     <asn:2>*cfg_data
>   drivers/misc/cxl/vphb.c:252:23:    got void *<noident>
> 
> Add __iomem annotations and remove unnecessary casts to clear up these
> warnings.
> 
> Reported-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> 
> ---
> 
> This patch is a respin of https://patchwork.ozlabs.org/patch/504976/,
> incorporating comments from mpe.
> 
> As with the old patch, this patch doesn't make any changes to the return
> type of cxl_pcie_cfg_addr() and casts to an __iomem type when we use it in
> cxl_pcie_config_info(). cxl_pcie_cfg_addr() returns an unsigned long,
> rather than a pointer type, as we use its return value in bitwise
> operations. If there's a better way to handle this I'd like to know.

There's always a better way, but can we agree on what it is :)

Part of your problem is you're storing afu->crs_len which is not __iomem in
cfg_data which is, and so that's leading to some of your casts.

I don't really see why you're using cfg_data like that, you have the afu in
phb->private_data. But maybe cfg_data needs to hold that value for some other
code I'm not seeing.

Regardless, in cxl_pcie_config_info() you have the afu pointer, so you can just
look at afu->crs_len directly can't you?

That means you can drop one cast of cfg_data to unsigned long in there.

Then I see that cxl_pcie_cfg_addr() is only used in cxl_pcie_config_info(), and
doesn't abstract much. So I'd drop it and inline the logic. That leads to the
realisation that we're calling cxl_pcie_cfg_record() twice, so we can save the
value and only call it once.

You're then left with:

	addr = phb->cfg_addr + (afu->crs_len * record) + offset;
	*ioaddr = (void *)(addr & ~0x3ULL);
	*shift = ((addr & 0x3) * 8);

Ideally we'd be able to say that cfg_addr is always aligned, and so it doesn't
need to be part of the calculation. I suspect that is true but you'll have to
check. If it is you can then leave cfg_addr out of the logic and you have:

	addr = (afu->crs_len * record) + offset;

	*ioaddr = phb->cfg_addr + (addr & ~0x3ull);
	*shift = (addr & 0x3) * 8;

Which hopefully still gives you the right result! :)

cheers

Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c

[Andrew Donnellan]

On 03/11/15 20:09, Michael Ellerman wrote:
> Part of your problem is you're storing afu->crs_len which is not __iomem in
> cfg_data which is, and so that's leading to some of your casts.
>
> I don't really see why you're using cfg_data like that, you have the afu in
> phb->private_data. But maybe cfg_data needs to hold that value for some other
> code I'm not seeing.

I can't see any obvious reason why we need to use cfg_data either.

> Regardless, in cxl_pcie_config_info() you have the afu pointer, so you can just
> look at afu->crs_len directly can't you?
>
> That means you can drop one cast of cfg_data to unsigned long in there.
>
> Then I see that cxl_pcie_cfg_addr() is only used in cxl_pcie_config_info(), and
> doesn't abstract much. So I'd drop it and inline the logic. That leads to the
> realisation that we're calling cxl_pcie_cfg_record() twice, so we can save the
> value and only call it once.
> You're then left with:
>
> 	addr = phb->cfg_addr + (afu->crs_len * record) + offset;
> 	*ioaddr = (void *)(addr & ~0x3ULL);
> 	*shift = ((addr & 0x3) * 8);

Will do, that's nicer.

> Ideally we'd be able to say that cfg_addr is always aligned, and so it doesn't
> need to be part of the calculation. I suspect that is true but you'll have to
> check. If it is you can then leave cfg_addr out of the logic and you have:
>
> 	addr = (afu->crs_len * record) + offset;
>
> 	*ioaddr = phb->cfg_addr + (addr & ~0x3ull);
> 	*shift = (addr & 0x3) * 8;
>
> Which hopefully still gives you the right result! :)

Will check.


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: sparse: add __iomem annotations in vphb.c

[Andrew Donnellan]

Finally looking at this patch again for the first time in a couple of 
months...

On 04/11/15 17:17, Andrew Donnellan wrote:
> On 03/11/15 20:09, Michael Ellerman wrote:
>> Part of your problem is you're storing afu->crs_len which is not
>> __iomem in
>> cfg_data which is, and so that's leading to some of your casts.
>>
>> I don't really see why you're using cfg_data like that, you have the
>> afu in
>> phb->private_data. But maybe cfg_data needs to hold that value for
>> some other
>> code I'm not seeing.
>
> I can't see any obvious reason why we need to use cfg_data either.

Ian/Mikey - do you happen to know why we're using cfg_data? I've taken 
another look and I can't see anything obvious.

-- 
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: sparse: add __iomem annotations in vphb.c

[Michael Neuling]

On Tue, 2015-12-08 at 17:30 +1100, Andrew Donnellan wrote:
> Finally looking at this patch again for the first time in a couple of
> months...
>=20
> On 04/11/15 17:17, Andrew Donnellan wrote:
> > On 03/11/15 20:09, Michael Ellerman wrote:
> > > Part of your problem is you're storing afu->crs_len which is not
> > > __iomem in
> > > cfg_data which is, and so that's leading to some of your casts.
> > >=20
> > > I don't really see why you're using cfg_data like that, you have
> > > the
> > > afu in
> > > phb->private_data. But maybe cfg_data needs to hold that value
> > > for
> > > some other
> > > code I'm not seeing.
> >=20
> > I can't see any obvious reason why we need to use cfg_data either.
>=20
> Ian/Mikey - do you happen to know why we're using cfg_data? I've
> taken=20
> another look and I can't see anything obvious.
>=20

IIRC, when I coded this up, benh just said these (cfg_addr/data) are
just private data and we can stick whatever we like in there.

We could store it in the AFU struct but it's (was just) convenient to
just store it here.

Mikey

Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c

[Michael Ellerman]

On Wed, 2015-12-09 at 12:00 +1100, Michael Neuling wrote:
> On Tue, 2015-12-08 at 17:30 +1100, Andrew Donnellan wrote:
> > Finally looking at this patch again for the first time in a couple of
> > months...
> > 
> > On 04/11/15 17:17, Andrew Donnellan wrote:
> > > On 03/11/15 20:09, Michael Ellerman wrote:
> > > > Part of your problem is you're storing afu->crs_len which is not __iomem in
> > > > cfg_data which is, and so that's leading to some of your casts.
> > > > 
> > > > I don't really see why you're using cfg_data like that, you have the
> > > > afu in phb->private_data. But maybe cfg_data needs to hold that value
> > > > for some other code I'm not seeing.
> > > 
> > > I can't see any obvious reason why we need to use cfg_data either.
> > 
> > Ian/Mikey - do you happen to know why we're using cfg_data? I've
> > taken another look and I can't see anything obvious.
> 
> IIRC, when I coded this up, benh just said these (cfg_addr/data) are
> just private data and we can stick whatever we like in there.
> 
> We could store it in the AFU struct but it's (was just) convenient to
> just store it here.

You're storing afu->crs_len in there, so it is in the AFU struct. Unless
there's a different "AFU struct", in which case meh.

Please send me a patch to clean it up Andrew.

cheers