[PATCH] cxl: sparse: add __iomem annotations in vphb.c
Andrew Donnellan
andrew.donnellan at au1.ibm.com
Mon Nov 2 15:57:11 AEDT 2015
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 at au1.ibm.com Australia Development Lab, Canberra
+61 2 6201 8874 (work) IBM Australia Limited
More information about the Linuxppc-dev
mailing list