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

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

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