Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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. Mikey ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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.donnel...@au1.ibm.com Australia Development Lab, Canberra +61 2 6201 8874 (work)IBM Australia Limited ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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.donnel...@au1.ibm.com Australia Development Lab, Canberra +61 2 6201 8874 (work)IBM Australia Limited ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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] > * > drivers/misc/cxl/vphb.c:131:17:got void * > 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] > *cfg_data > drivers/misc/cxl/vphb.c:252:23:got void * > > Add __iomem annotations and remove unnecessary casts to clear up these > warnings. > > Reported-by: Daniel Axtens > Signed-off-by: Andrew Donnellan > > --- > > 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
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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.donnel...@au1.ibm.com Australia Development Lab, Canberra +61 2 6201 8874 (work)IBM Australia Limited ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] cxl: sparse: add __iomem annotations in vphb.c
Acked-by: Ian Munsie ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] cxl: sparse: add __iomem annotations in vphb.c
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] * drivers/misc/cxl/vphb.c:131:17:got void * 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] *cfg_data drivers/misc/cxl/vphb.c:252:23:got void * Add __iomem annotations and remove unnecessary casts to clear up these warnings. Reported-by: Daniel Axtens Signed-off-by: Andrew Donnellan --- 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.donnel...@au1.ibm.com Australia Development Lab, Canberra +61 2 6201 8874 (work)IBM Australia Limited ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev