* [PATCH 1/2] PCI: Change pci_device_is_present() to pci_dev_is_inaccessible()
2019-09-04 4:36 [PATCH 0/2] PCI: Change to using pci_dev_is_inaccessible() Kelsey Skunberg
@ 2019-09-04 4:36 ` Kelsey Skunberg
2019-09-04 4:36 ` [PATCH 2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible() Kelsey Skunberg
1 sibling, 0 replies; 6+ messages in thread
From: Kelsey Skunberg @ 2019-09-04 4:36 UTC (permalink / raw)
To: bhelgaas, linux-pci, linux-kernel, linux-kernel-mentees, skunberg.kelsey
Cc: skhan, rafael.j.wysocki, keith.busch
The pci_device_is_present() name may encourage poor practice of calling
pci_device_is_present() and if true, assuming the device is still present
after the call. This type of practice can be racy when assuming a device
is still connected after checking.
Change pci_device_is_present() to pci_dev_is_inaccessible() to promote
only using to learn whether we should avoid accessing a device that's
inaccessible.
Change pci_device_is_inaccessible() to now return true if PCI device is
inaccessible.
Change the boolean values returned from calling pci_dev_is_inaccessible()
to their opposite value to reflect the change of checking if the device is
present to checking if the device is inaccessible. Example:
Before:
if (!pci_device_is_present(tp->pdev))
return -ENODEV;
After:
if (pci_dev_is_inaccessible(tp->pdev))
return -ENODEV;
Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
drivers/net/ethernet/broadcom/tg3.c | 4 ++--
drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
drivers/nvme/host/pci.c | 2 +-
drivers/pci/hotplug/acpiphp_glue.c | 2 +-
drivers/pci/pci.c | 10 +++++-----
drivers/thunderbolt/nhi.c | 2 +-
include/linux/pci.h | 2 +-
7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 4c404d2213f9..7646a8303d01 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -9067,7 +9067,7 @@ static int tg3_chip_reset(struct tg3 *tp)
void (*write_op)(struct tg3 *, u32, u32);
int i, err;
- if (!pci_device_is_present(tp->pdev))
+ if (pci_dev_is_inaccessible(tp->pdev))
return -ENODEV;
tg3_nvram_lock(tp);
@@ -11782,7 +11782,7 @@ static int tg3_close(struct net_device *dev)
tg3_stop(tp);
- if (pci_device_is_present(tp->pdev)) {
+ if (!pci_dev_is_inaccessible(tp->pdev)) {
tg3_power_down_prepare(tp);
tg3_carrier_off(tp);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b4df3e319467..87bc067c2abc 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8835,7 +8835,7 @@ static int __maybe_unused igb_resume(struct device *dev)
pci_restore_state(pdev);
pci_save_state(pdev);
- if (!pci_device_is_present(pdev))
+ if (pci_dev_is_inaccessible(pdev))
return -ENODEV;
err = pci_enable_device_mem(pdev);
if (err) {
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index bb970ca82517..2a4500325471 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2812,7 +2812,7 @@ static void nvme_remove(struct pci_dev *pdev)
nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
pci_set_drvdata(pdev, NULL);
- if (!pci_device_is_present(pdev)) {
+ if (pci_dev_is_inaccessible(pdev)) {
nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD);
nvme_dev_disable(dev, true);
nvme_dev_remove_admin(dev);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index e4c46637f32f..9cc2d65877bd 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -647,7 +647,7 @@ static void trim_stale_devices(struct pci_dev *dev)
alive = alive || (ACPI_SUCCESS(status) && device_status_valid(sta));
}
if (!alive)
- alive = pci_device_is_present(dev);
+ alive = !pci_dev_is_inaccessible(dev);
if (!alive) {
pci_dev_set_disconnected(dev, NULL);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ed5ec1ac27..7b4e248db5f9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -931,7 +931,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
{
if (platform_pci_get_power_state(dev) == PCI_D3cold ||
- !pci_device_is_present(dev)) {
+ pci_dev_is_inaccessible(dev)) {
dev->current_state = PCI_D3cold;
} else if (dev->pm_cap) {
u16 pmcsr;
@@ -5906,15 +5906,15 @@ bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
test_bit(dev1->devfn, dev2->dma_alias_mask));
}
-bool pci_device_is_present(struct pci_dev *pdev)
+bool pci_dev_is_inaccessible(struct pci_dev *pdev)
{
u32 v;
if (pci_dev_is_disconnected(pdev))
- return false;
- return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
+ return true;
+ return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
}
-EXPORT_SYMBOL_GPL(pci_device_is_present);
+EXPORT_SYMBOL_GPL(pci_dev_is_inaccessible);
void pci_ignore_hotplug(struct pci_dev *dev)
{
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 27fbe62c7ddd..597c7579d882 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -879,7 +879,7 @@ static int nhi_resume_noirq(struct device *dev)
* unplugged last device which causes the host controller to go
* away on PCs.
*/
- if (!pci_device_is_present(pdev))
+ if (pci_dev_is_inaccessible(pdev))
tb->nhi->going_away = true;
else
nhi_enable_int_throttling(tb->nhi);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9e700d9f9f28..e599068537cf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1202,7 +1202,7 @@ int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size
void pci_release_resource(struct pci_dev *dev, int resno);
int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
int pci_select_bars(struct pci_dev *dev, unsigned long flags);
-bool pci_device_is_present(struct pci_dev *pdev);
+bool pci_dev_is_inaccessible(struct pci_dev *pdev);
void pci_ignore_hotplug(struct pci_dev *dev);
int __printf(6, 7) pci_request_irq(struct pci_dev *dev, unsigned int nr,
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()
2019-09-04 4:36 [PATCH 0/2] PCI: Change to using pci_dev_is_inaccessible() Kelsey Skunberg
2019-09-04 4:36 ` [PATCH 1/2] PCI: Change pci_device_is_present() to pci_dev_is_inaccessible() Kelsey Skunberg
@ 2019-09-04 4:36 ` Kelsey Skunberg
2019-09-04 5:35 ` Lukas Wunner
1 sibling, 1 reply; 6+ messages in thread
From: Kelsey Skunberg @ 2019-09-04 4:36 UTC (permalink / raw)
To: bhelgaas, linux-pci, linux-kernel, linux-kernel-mentees, skunberg.kelsey
Cc: skhan, rafael.j.wysocki, keith.busch
Combine pci_dev_is_disconnected() with pci_dev_is_inaccessible() so only
one function is used to learn if we should avoid accessing a device that's
inaccessible due to surprise removal or an error condition.
The use cases for pci_dev_is_disconnected() do not need to distinguish
between a device being inaccessible due to a surprise removal or an error
condition. This provides the opportunity to unify
pci_dev_is_disconnected() and pci_dev_is_inaccessible() to reduce multiple
functions used for the same task.
Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to:
pdev->error_state == pci_channel_io_perm_failure
Change remaining pci_dev_is_disconnected() calls to
pci_dev_is_inaccessible() calls.
Remove pci_dev_is_disconnected() from /pci/pci.h which would now no longer
be used.
Demonstration of changes to pci_dev_is_disconnected() and
pci_dev_is_inaccessible():
Before combining:
static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
{
return dev->error_state == pci_channel_io_perm_failure;
}
bool pci_dev_is_inaccessible(struct pci_dev *pdev)
{
u32 v;
if (pci_dev_is_disconnected(pdev))
return true;
return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
}
After combining:
bool pci_dev_is_inaccessible(struct pci_dev *pdev)
{
u32 v;
if (pdev->error_state == pci_channel_io_perm_failure)
return true;
return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
}
Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
drivers/pci/access.c | 12 ++++++------
drivers/pci/msi.c | 4 ++--
drivers/pci/pci.c | 2 +-
drivers/pci/pci.h | 5 -----
drivers/pci/pcie/portdrv_core.c | 2 +-
5 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 544922f097c0..c096340afb8c 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -535,7 +535,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
{
- if (pci_dev_is_disconnected(dev)) {
+ if (pci_dev_is_inaccessible(dev)) {
*val = ~0;
return PCIBIOS_DEVICE_NOT_FOUND;
}
@@ -545,7 +545,7 @@ EXPORT_SYMBOL(pci_read_config_byte);
int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
{
- if (pci_dev_is_disconnected(dev)) {
+ if (pci_dev_is_inaccessible(dev)) {
*val = ~0;
return PCIBIOS_DEVICE_NOT_FOUND;
}
@@ -556,7 +556,7 @@ EXPORT_SYMBOL(pci_read_config_word);
int pci_read_config_dword(const struct pci_dev *dev, int where,
u32 *val)
{
- if (pci_dev_is_disconnected(dev)) {
+ if (pci_dev_is_inaccessible(dev)) {
*val = ~0;
return PCIBIOS_DEVICE_NOT_FOUND;
}
@@ -566,7 +566,7 @@ EXPORT_SYMBOL(pci_read_config_dword);
int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
{
- if (pci_dev_is_disconnected(dev))
+ if (pci_dev_is_inaccessible(dev))
return PCIBIOS_DEVICE_NOT_FOUND;
return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
}
@@ -574,7 +574,7 @@ EXPORT_SYMBOL(pci_write_config_byte);
int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
{
- if (pci_dev_is_disconnected(dev))
+ if (pci_dev_is_inaccessible(dev))
return PCIBIOS_DEVICE_NOT_FOUND;
return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
}
@@ -583,7 +583,7 @@ EXPORT_SYMBOL(pci_write_config_word);
int pci_write_config_dword(const struct pci_dev *dev, int where,
u32 val)
{
- if (pci_dev_is_disconnected(dev))
+ if (pci_dev_is_inaccessible(dev))
return PCIBIOS_DEVICE_NOT_FOUND;
return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
}
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0884bedcfc7a..4680043aa315 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -311,7 +311,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
{
struct pci_dev *dev = msi_desc_to_pci_dev(entry);
- if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
+ if (dev->current_state != PCI_D0 || pci_dev_is_inaccessible(dev)) {
/* Don't touch the hardware now */
} else if (entry->msi_attrib.is_msix) {
void __iomem *base = pci_msix_desc_addr(entry);
@@ -1008,7 +1008,7 @@ static void pci_msix_shutdown(struct pci_dev *dev)
if (!pci_msi_enable || !dev || !dev->msix_enabled)
return;
- if (pci_dev_is_disconnected(dev)) {
+ if (pci_dev_is_inaccessible(dev)) {
dev->msix_enabled = 0;
return;
}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7b4e248db5f9..e5f46d98dbe1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5910,7 +5910,7 @@ bool pci_dev_is_inaccessible(struct pci_dev *pdev)
{
u32 v;
- if (pci_dev_is_disconnected(pdev))
+ if (pdev->error_state == pci_channel_io_perm_failure)
return true;
return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 1be03a97cb92..f0dc86dc8aab 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -363,11 +363,6 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
return 0;
}
-static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
-{
- return dev->error_state == pci_channel_io_perm_failure;
-}
-
/* pci_dev priv_flags */
#define PCI_DEV_ADDED 0
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 308c3e0c4a34..8bf6b47dd2c6 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -416,7 +416,7 @@ static void wait_for_downstream_link(struct pci_dev *pdev)
pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)
return;
- if (pci_dev_is_disconnected(pdev))
+ if (pci_dev_is_inaccessible(pdev))
return;
if (!pdev->subordinate || list_empty(&pdev->subordinate->devices) ||
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread