| Message ID | 20201129230743.3006978-5-kw@linux.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: Unify ECAM constants in native PCI Express drivers | expand |
From: Krzysztof Wilczynski > Sent: 29 November 2020 23:08 > > Use "void __iomem" instead "char __iomem" pointer type when working with > the accessor functions (with names like readb() or writel(), etc.) to > better match a given accessor function signature where commonly the > address pointing to an I/O memory region would be a "void __iomem" > pointer. ISTM that is heading in the wrong direction. I think (form the variable names etc) that these are pointers to specific registers. So what you ought to have is a type for that register block. Typically this is actually a structure - to give some type checking that the offsets are being used with the correct base address. If the code is using numeric offsets (hardware engineers like numeric offsets) then you can get some type protection by using a structure that only contains a single field (char in this case). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Nov 30, 2020 at 09:06:56AM +0000, David Laight wrote: > From: Krzysztof Wilczynski > > Sent: 29 November 2020 23:08 > > > > Use "void __iomem" instead "char __iomem" pointer type when working with > > the accessor functions (with names like readb() or writel(), etc.) to > > better match a given accessor function signature where commonly the > > address pointing to an I/O memory region would be a "void __iomem" > > pointer. > > ISTM that is heading in the wrong direction. > > I think (form the variable names etc) that these are pointers > to specific registers. > > So what you ought to have is a type for that register block. > Typically this is actually a structure - to give some type > checking that the offsets are being used with the correct > base address. In this case, "cfgbar" is not really a pointer to a register; it's the address of memory-mapped config space. The VMD hardware turns accesses to that space into PCI config transactions on its secondary side. xgene_pcie_get_cfg_base() and brcm_pcie_map_conf() are similar situations and use "void *". Bjorn
On Mon, 2020-11-30 at 11:20 -0600, Bjorn Helgaas wrote: > On Mon, Nov 30, 2020 at 09:06:56AM +0000, David Laight wrote: > > From: Krzysztof Wilczynski > > > Sent: 29 November 2020 23:08 > > > > > > Use "void __iomem" instead "char __iomem" pointer type when working with > > > the accessor functions (with names like readb() or writel(), etc.) to > > > better match a given accessor function signature where commonly the > > > address pointing to an I/O memory region would be a "void __iomem" > > > pointer. > > > > ISTM that is heading in the wrong direction. > > > > I think (form the variable names etc) that these are pointers > > to specific registers. > > > > So what you ought to have is a type for that register block. > > Typically this is actually a structure - to give some type > > checking that the offsets are being used with the correct > > base address. > > In this case, "cfgbar" is not really a pointer to a register; it's the > address of memory-mapped config space. The VMD hardware turns > accesses to that space into PCI config transactions on its secondary > side. xgene_pcie_get_cfg_base() and brcm_pcie_map_conf() are similar > situations and use "void *". > > Bjorn Yes it's just the passthrough window for PCI config bus ops. Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index 1361a79bd1e7..59fa9a94860f 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -95,7 +95,7 @@ struct vmd_dev { struct pci_dev *dev; spinlock_t cfg_lock; - char __iomem *cfgbar; + void __iomem *cfgbar; int msix_count; struct vmd_irq_list *irqs; @@ -326,7 +326,7 @@ static void vmd_remove_irq_domain(struct vmd_dev *vmd) } } -static char __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus, +static void __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus, unsigned int devfn, int reg, int len) { unsigned int busnr_ecam = bus->number - vmd->busn_start; @@ -346,7 +346,7 @@ static int vmd_pci_read(struct pci_bus *bus, unsigned int devfn, int reg, int len, u32 *value) { struct vmd_dev *vmd = vmd_from_bus(bus); - char __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len); + void __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len); unsigned long flags; int ret = 0; @@ -381,7 +381,7 @@ static int vmd_pci_write(struct pci_bus *bus, unsigned int devfn, int reg, int len, u32 value) { struct vmd_dev *vmd = vmd_from_bus(bus); - char __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len); + void __iomem *addr = vmd_cfg_addr(vmd, bus, devfn, reg, len); unsigned long flags; int ret = 0;
Use "void __iomem" instead "char __iomem" pointer type when working with the accessor functions (with names like readb() or writel(), etc.) to better match a given accessor function signature where commonly the address pointing to an I/O memory region would be a "void __iomem" pointer. Related: https://lwn.net/Articles/102232/ Suggested-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Krzysztof Wilczyński <kw@linux.com> --- drivers/pci/controller/vmd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)