Skip to contentSkip to conversation

PCI: access.c: Piggyback user config access on pci_read/write_*()

PATCH In review Alexandru Gagniuc 3 participants 3 messages Jun 4, 2018 PCI

lore
Share & tools
lore

Alexandru Gagniuc

author
#0 lore raw

PATCH 1/1 PCI: access.c: Piggyback user config access on pci_read/write_*()

pci_read/write_config*() functions have several safeguards to prevent stallling accesses when a device is removed. However, their "_user_" counterparts use a different code path.

To make sure that safeguards are used for userspace PCI config accesses, piggyback the "_user_" functions on the in-kernel pci_read/write_config*().

Signed-off-by Alexandru Gagniuc <[email protected]>
---
 drivers/pci/access.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)
drivers/pci/access.c +2 -14
index a3ad2fe185b9..6db2a8713c85 100644
@@ -223,16 +223,9 @@ int pci_user_read_config_##size						\
 	(struct pci_dev *dev, int pos, type *val)			\
 {									\
 	int ret = PCIBIOS_SUCCESSFUL;					\
-	u32 data = -1;							\
 	if (PCI_##size##_BAD)						\
 		return -EINVAL;						\
-	raw_spin_lock_irq(&pci_lock);				\
-	if (unlikely(dev->block_cfg_access))				\
-		pci_wait_cfg(dev);					\
-	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
-					pos, sizeof(type), &data);	\
-	raw_spin_unlock_irq(&pci_lock);				\
-	*val = (type)data;						\
+	ret = pci_read_config_##size(dev, pos, val);			\
 	return pcibios_err_to_errno(ret);				\
 }									\
 EXPORT_SYMBOL_GPL(pci_user_read_config_##size);

@@ -245,12 +238,7 @@ int pci_user_write_config_##size					\
 	int ret = PCIBIOS_SUCCESSFUL;					\
 	if (PCI_##size##_BAD)						\
 		return -EINVAL;						\
-	raw_spin_lock_irq(&pci_lock);				\
-	if (unlikely(dev->block_cfg_access))				\
-		pci_wait_cfg(dev);					\
-	ret = dev->bus->ops->write(dev->bus, dev->devfn,		\
-					pos, sizeof(type), val);	\
-	raw_spin_unlock_irq(&pci_lock);				\
+	ret = pci_write_config_##size(dev, pos, val);			\
 	return pcibios_err_to_errno(ret);				\
 }									\
 EXPORT_SYMBOL_GPL(pci_user_write_config_##size);
Signature · 1 line
2.14.4
#1 lore raw
Quoted 8 lines · Alexandru Gagniuc

+++ b/drivers/pci/access.c

drivers/pci/access.c +0 -1
@@ -223,16 +223,9 @@ int pci_user_read_config_##size						\
 	(struct pci_dev *dev, int pos, type *val)			\
 {									\
 	int ret = PCIBIOS_SUCCESSFUL;					\
-	u32 data = -1;							\
 	if (PCI_##size##_BAD)						\
 		return -EINVAL;						\

drivers/pci/access.c int pci_user_read_config_##size \ · line 230

Quoted 10 lines
drivers/pci/access.c +1 -7
-	raw_spin_lock_irq(&pci_lock);				\
-	if (unlikely(dev->block_cfg_access))				\
-		pci_wait_cfg(dev);					\
-	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
-					pos, sizeof(type), &data);	\
-	raw_spin_unlock_irq(&pci_lock);				\
-	*val = (type)data;						\
+	ret = pci_read_config_##size(dev, pos, val);			\
 	return pcibios_err_to_errno(ret);				\
 }									\

If it wasn't for the block_cfg_access check for user access, this would have been a nice code reuse cleanup.

#2 lore raw

On 6/4/2018 11:09 AM, Keith Busch wrote:=0A=

Quoted 13 lines

On Mon, Jun 04, 2018 at 10:48:02AM -0500, Alexandru Gagniuc wrote:=0A=

Quoted 12 lines

+++ b/drivers/pci/access.c=0A=

(quoted diff) +0 -5
@@ -223,16 +223,9 @@ int pci_user_read_config_##size						\=0A=
  	(struct pci_dev *dev, int pos, type *val)			\=0A=
  {									\=0A=
  	int ret =3D PCIBIOS_SUCCESSFUL;					\=0A=
-	u32 data =3D -1;							\=0A=
  	if (PCI_##size##_BAD)						\=0A=
  		return -EINVAL;						\=0A=
-	raw_spin_lock_irq(&pci_lock);				\=0A=
-	if (unlikely(dev->block_cfg_access))				\=0A=
-		pci_wait_cfg(dev);					\=0A=
-	ret =3D dev->bus->ops->read(dev->bus, dev->devfn,			\=0A=
Quoted 8 lines

Review int pci_user_read_config_##size \=0A= · line 230

Quoted 6 lines
(quoted diff) +1 -3
-					pos, sizeof(type), &data);	\=0A=
-	raw_spin_unlock_irq(&pci_lock);				\=0A=
-	*val =3D (type)data;						\=0A=
+	ret =3D pci_read_config_##size(dev, pos, val);			\=0A=
  	return pcibios_err_to_errno(ret);				\=0A=
  }									\=0A=

=0A=
If it wasn't for the block_cfg_access check for user access, this would=

=0A=

Quoted 1 line

have been a nice code reuse cleanup.=0A=

=0A=
Great fresh catch! I'll get that fixed in v2.=0A=
=0A=
Alex=0A=
=0A=