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

[Andrew Donnellan <andrew.donnellan@au1.ibm.com>]

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