diff mbox series

[v6,4/5] PCI: vmd: Update type of the __iomem pointers

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

Commit Message

Krzysztof Wilczyński Nov. 29, 2020, 11:07 p.m. UTC
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(-)

Comments

David Laight Nov. 30, 2020, 9:06 a.m. UTC | #1
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)
Bjorn Helgaas Nov. 30, 2020, 5:20 p.m. UTC | #2
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
Jon Derrick Nov. 30, 2020, 6:19 p.m. UTC | #3
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 mbox series

Patch

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;