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 <d...@axtens.net> > Signed-off-by: Andrew Donnellan <andrew.donnel...@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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev