[PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Uwe Kleine-König]

Hello,

this is v6 of the quest to drop the "driver" member from struct pci_dev
which tracks the same data (apart from a constant offset) as dev.driver.

Changes since v5:
 - Some Acks added
 - Some fixes in "PCI: Replace pci_dev::driver usage by
   pci_dev::dev.driver" to properly handle that
   to_pci_driver(X) is wrong if X is NULL.
   This should fix the problem reported by Ido Schimmel.

Full range diff below.

This patch stack survived an allmodconfig build on arm64, m68k, powerpc,
riscv, s390, sparc64 and x86_64 on top of v5.15-rc3.

Best regards
Uwe

Uwe Kleine-König (11):
  PCI: Simplify pci_device_remove()
  PCI: Drop useless check from pci_device_probe()
  xen/pci: Drop some checks that are always true
  bcma: simplify reference to the driver's name
  powerpc/eeh: Don't use driver member of struct pci_dev and further
    cleanups
  ssb: Simplify determination of driver name
  PCI: Replace pci_dev::driver usage that gets the driver name
  scsi: message: fusion: Remove unused parameter of mpt_pci driver's
    probe()
  crypto: qat - simplify adf_enable_aer()
  PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
  PCI: Drop duplicated tracking of a pci_dev's bound driver

 arch/powerpc/include/asm/ppc-pci.h            |  5 -
 arch/powerpc/kernel/eeh.c                     |  8 ++
 arch/powerpc/kernel/eeh_driver.c              | 10 +-
 arch/x86/events/intel/uncore.c                |  2 +-
 arch/x86/kernel/probe_roms.c                  | 10 +-
 drivers/bcma/host_pci.c                       |  6 +-
 drivers/crypto/hisilicon/qm.c                 |  2 +-
 drivers/crypto/qat/qat_4xxx/adf_drv.c         |  7 +-
 drivers/crypto/qat/qat_c3xxx/adf_drv.c        |  7 +-
 drivers/crypto/qat/qat_c62x/adf_drv.c         |  7 +-
 drivers/crypto/qat/qat_common/adf_aer.c       | 10 +-
 .../crypto/qat/qat_common/adf_common_drv.h    |  3 +-
 drivers/crypto/qat/qat_dh895xcc/adf_drv.c     |  7 +-
 drivers/message/fusion/mptbase.c              |  7 +-
 drivers/message/fusion/mptbase.h              |  2 +-
 drivers/message/fusion/mptctl.c               |  4 +-
 drivers/message/fusion/mptlan.c               |  2 +-
 drivers/misc/cxl/guest.c                      | 24 +++--
 drivers/misc/cxl/pci.c                        | 30 +++---
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  2 +-
 .../ethernet/marvell/prestera/prestera_pci.c  |  2 +-
 drivers/net/ethernet/mellanox/mlxsw/pci.c     |  2 +-
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  |  3 +-
 drivers/pci/iov.c                             | 33 +++++--
 drivers/pci/pci-driver.c                      | 96 ++++++++++---------
 drivers/pci/pci.c                             |  4 +-
 drivers/pci/pcie/err.c                        | 36 +++----
 drivers/pci/xen-pcifront.c                    | 63 ++++++------
 drivers/ssb/pcihost_wrapper.c                 |  6 +-
 drivers/usb/host/xhci-pci.c                   |  2 +-
 include/linux/pci.h                           |  1 -
 31 files changed, 208 insertions(+), 195 deletions(-)

Range-diff against v5:
 -:  ------------ >  1:  c2b53ab26a6b PCI: Simplify pci_device_remove()
 -:  ------------ >  2:  2c733e1d5186 PCI: Drop useless check from pci_device_probe()
 -:  ------------ >  3:  547ca5a7aa16 xen/pci: Drop some checks that are always true
 -:  ------------ >  4:  40eb07353844 bcma: simplify reference to the driver's name
 -:  ------------ >  5:  bab59c1dff6d powerpc/eeh: Don't use driver member of struct pci_dev and further cleanups
 1:  abd70de9782d !  6:  92f4d61bbac3 ssb: Simplify determination of driver name
    @@ Commit message
         This has the upside of not requiring the driver member of struct pci_dev
         which is about to be removed and being simpler.
     
    +    Acked-by: Michael Büsch <m@bues.ch>
         Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
     
      ## drivers/ssb/pcihost_wrapper.c ##
 2:  735845bd26b9 !  7:  6303f03ab2aa PCI: Replace pci_dev::driver usage that gets the driver name
    @@ Commit message
         driver name by dev_driver_string() which implicitly makes use of struct
         pci_dev::dev->driver.
     
    +    Acked-by: Simon Horman <simon.horman@corigine.com> (for NFP)
         Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
     
      ## drivers/crypto/hisilicon/qm.c ##
 3:  1e58019165b9 =  8:  658a6c00ec96 scsi: message: fusion: Remove unused parameter of mpt_pci driver's probe()
 4:  dea72a470141 =  9:  aceaf5321603 crypto: qat - simplify adf_enable_aer()
 5:  b4165dda38ea ! 10:  80648d999985 PCI: Replace pci_dev::driver usage by pci_dev::dev.driver
    @@ arch/x86/kernel/probe_roms.c: static struct resource video_rom_resource = {
      static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
      {
     -	struct pci_driver *drv = pdev->driver;
    -+	struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
      	const struct pci_device_id *id;
      
      	if (pdev->vendor == vendor && pdev->device == device)
    + 		return true;
    + 
    +-	for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
    +-		if (id->vendor == vendor && id->device == device)
    +-			break;
    ++	if (pdev->dev.driver) {
    ++		struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
    ++		for (id = drv->id_table; id && id->vendor; id++)
    ++			if (id->vendor == vendor && id->device == device)
    ++				break;
    ++	}
    + 
    + 	return id && id->vendor;
    + }
     
      ## drivers/misc/cxl/guest.c ##
     @@ drivers/misc/cxl/guest.c: static void pci_error_handlers(struct cxl_afu *afu,
    @@ drivers/pci/iov.c: static ssize_t sriov_vf_total_msix_show(struct device *dev,
      
      	device_lock(dev);
     -	if (!pdev->driver || !pdev->driver->sriov_get_vf_total_msix)
    -+	pdrv = to_pci_driver(dev->driver);
    -+	if (!pdrv || !pdrv->sriov_get_vf_total_msix)
    ++	if (!dev->driver)
      		goto unlock;
      
     -	vf_total_msix = pdev->driver->sriov_get_vf_total_msix(pdev);
    ++	pdrv = to_pci_driver(dev->driver);
    ++	if (!pdrv->sriov_get_vf_total_msix)
    ++		goto unlock;
    ++
     +	vf_total_msix = pdrv->sriov_get_vf_total_msix(pdev);
      unlock:
      	device_unlock(dev);
    @@ drivers/pci/iov.c: static ssize_t sriov_vf_msix_count_store(struct device *dev,
      
      	device_lock(&pdev->dev);
     -	if (!pdev->driver || !pdev->driver->sriov_set_msix_vec_count) {
    ++	if (!pdev->dev.driver) {
    ++		ret = -EOPNOTSUPP;
    ++		goto err_pdev;
    ++	}
    ++
     +	pdrv = to_pci_driver(pdev->dev.driver);
    -+	if (!pdrv || !pdrv->sriov_set_msix_vec_count) {
    ++	if (!pdrv->sriov_set_msix_vec_count) {
      		ret = -EOPNOTSUPP;
      		goto err_pdev;
      	}
    @@ drivers/pci/pci-driver.c: static void pci_device_remove(struct device *dev)
      {
      	struct pci_dev *pci_dev = to_pci_dev(dev);
     -	struct pci_driver *drv = pci_dev->driver;
    -+	struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
      
      	pm_runtime_resume(dev);
      
    +-	if (drv && drv->shutdown)
    +-		drv->shutdown(pci_dev);
    ++	if (pci_dev->dev.driver) {
    ++		struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
    ++
    ++		if (drv->shutdown)
    ++			drv->shutdown(pci_dev);
    ++	}
    + 
    + 	/*
    + 	 * If this is a kexec reboot, turn off Bus Master bit on the
     @@ drivers/pci/pci-driver.c: static int pci_pm_reenable_device(struct pci_dev *pci_dev)
      static int pci_legacy_suspend(struct device *dev, pm_message_t state)
      {
      	struct pci_dev *pci_dev = to_pci_dev(dev);
     -	struct pci_driver *drv = pci_dev->driver;
    -+	struct pci_driver *drv = to_pci_driver(dev->driver);
      
    - 	if (drv && drv->suspend) {
    - 		pci_power_t prev = pci_dev->current_state;
    +-	if (drv && drv->suspend) {
    +-		pci_power_t prev = pci_dev->current_state;
    +-		int error;
    ++	if (dev->driver) {
    ++		struct pci_driver *drv = to_pci_driver(dev->driver);
    + 
    +-		error = drv->suspend(pci_dev, state);
    +-		suspend_report_result(drv->suspend, error);
    +-		if (error)
    +-			return error;
    ++		if (drv->suspend) {
    ++			pci_power_t prev = pci_dev->current_state;
    ++			int error;
    + 
    +-		if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
    +-		    && pci_dev->current_state != PCI_UNKNOWN) {
    +-			pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
    +-				      "PCI PM: Device state not saved by %pS\n",
    +-				      drv->suspend);
    ++			error = drv->suspend(pci_dev, state);
    ++			suspend_report_result(drv->suspend, error);
    ++			if (error)
    ++				return error;
    ++
    ++			if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
    ++			    && pci_dev->current_state != PCI_UNKNOWN) {
    ++				pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
    ++					      "PCI PM: Device state not saved by %pS\n",
    ++					      drv->suspend);
    ++			}
    + 		}
    + 	}
    + 
     @@ drivers/pci/pci-driver.c: static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
      static int pci_legacy_resume(struct device *dev)
      {
      	struct pci_dev *pci_dev = to_pci_dev(dev);
     -	struct pci_driver *drv = pci_dev->driver;
    -+	struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
      
      	pci_fixup_device(pci_fixup_resume, pci_dev);
      
    +-	return drv && drv->resume ?
    +-			drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
    ++	if (pci_dev->dev.driver) {
    ++		struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
    ++
    ++		if (drv->resume)
    ++			return drv->resume(pci_dev);
    ++	}
    ++
    ++	return pci_pm_reenable_device(pci_dev);
    + }
    + 
    + /* Auxiliary functions used by the new power management framework */
     @@ drivers/pci/pci-driver.c: static void pci_pm_default_suspend(struct pci_dev *pci_dev)
      
      static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
      {
     -	struct pci_driver *drv = pci_dev->driver;
    -+	struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
    - 	bool ret = drv && (drv->suspend || drv->resume);
    +-	bool ret = drv && (drv->suspend || drv->resume);
    ++	struct pci_driver *drv;
    ++	bool ret;
    ++
    ++	if (!pci_dev->dev.driver)
    ++		return false;
    ++
    ++	drv = to_pci_driver(pci_dev->dev.driver);
    ++	ret = drv && (drv->suspend || drv->resume);
      
      	/*
    + 	 * Legacy PM support is used by default, so warn if the new framework is
     @@ drivers/pci/pci-driver.c: static int pci_pm_runtime_suspend(struct device *dev)
      	int error;
      
 6:  d93a138bd7ab = 11:  2686d69bca17 PCI: Drop duplicated tracking of a pci_dev's bound driver

base-commit: 5816b3e6577eaa676ceb00a848f0fd65fe2adc29
-- 
2.30.2

[PATCH v6 04/11] bcma: simplify reference to the driver's name

[Uwe Kleine-König]

When bcma_host_pci_probe() is called the pci driver core already
assigned the device's driver in local_pci_probe(). So dev->driver is
always true and here it points to bcma_pci_bridge_driver which has .name
set to "bcma-pci-bridge". Simplify accordingly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/bcma/host_pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/bcma/host_pci.c b/drivers/bcma/host_pci.c
index 69c10a7b7c61..960632197b05 100644
--- a/drivers/bcma/host_pci.c
+++ b/drivers/bcma/host_pci.c
@@ -162,7 +162,6 @@ static int bcma_host_pci_probe(struct pci_dev *dev,
 {
 	struct bcma_bus *bus;
 	int err = -ENOMEM;
-	const char *name;
 	u32 val;
 
 	/* Alloc */
@@ -175,10 +174,7 @@ static int bcma_host_pci_probe(struct pci_dev *dev,
 	if (err)
 		goto err_kfree_bus;
 
-	name = dev_name(&dev->dev);
-	if (dev->driver && dev->driver->name)
-		name = dev->driver->name;
-	err = pci_request_regions(dev, name);
+	err = pci_request_regions(dev, "bcma-pci-bridge");
 	if (err)
 		goto err_pci_disable;
 	pci_set_master(dev);
-- 
2.30.2

[PATCH v6 06/11] ssb: Simplify determination of driver name

[Uwe Kleine-König]

For all drivers that make use of ssb_pcihost_probe() (i.e.
b43_pci_bridge_driver and b44_pci_driver) the driver name is set.
As at the time for the function is called __pci_register_driver() already
assigned drv->driver.name to hold the same value, use
dev_driver_string() with the same result.

This has the upside of not requiring the driver member of struct pci_dev
which is about to be removed and being simpler.

Acked-by: Michael Büsch <m@bues.ch>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/ssb/pcihost_wrapper.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/ssb/pcihost_wrapper.c b/drivers/ssb/pcihost_wrapper.c
index 410215c16920..dd70fd41c77d 100644
--- a/drivers/ssb/pcihost_wrapper.c
+++ b/drivers/ssb/pcihost_wrapper.c
@@ -69,7 +69,6 @@ static int ssb_pcihost_probe(struct pci_dev *dev,
 {
 	struct ssb_bus *ssb;
 	int err = -ENOMEM;
-	const char *name;
 	u32 val;
 
 	ssb = kzalloc(sizeof(*ssb), GFP_KERNEL);
@@ -78,10 +77,7 @@ static int ssb_pcihost_probe(struct pci_dev *dev,
 	err = pci_enable_device(dev);
 	if (err)
 		goto err_kfree_ssb;
-	name = dev_name(&dev->dev);
-	if (dev->driver && dev->driver->name)
-		name = dev->driver->name;
-	err = pci_request_regions(dev, name);
+	err = pci_request_regions(dev, dev_driver_string(&dev->dev));
 	if (err)
 		goto err_pci_disable;
 	pci_set_master(dev);
-- 
2.30.2

[PATCH v6 07/11] PCI: Replace pci_dev::driver usage that gets the driver name

[Uwe Kleine-König]

struct pci_dev::driver holds (apart from a constant offset) the same
data as struct pci_dev::dev->driver. With the goal to remove struct
pci_dev::driver to get rid of data duplication replace getting the
driver name by dev_driver_string() which implicitly makes use of struct
pci_dev::dev->driver.

Acked-by: Simon Horman <simon.horman@corigine.com> (for NFP)
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/crypto/hisilicon/qm.c                        | 2 +-
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c   | 2 +-
 drivers/net/ethernet/marvell/prestera/prestera_pci.c | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/pci.c            | 2 +-
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 3 ++-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index 369562d34d66..8f361e54e524 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -3085,7 +3085,7 @@ static int qm_alloc_uacce(struct hisi_qm *qm)
 	};
 	int ret;
 
-	ret = strscpy(interface.name, pdev->driver->name,
+	ret = strscpy(interface.name, dev_driver_string(&pdev->dev),
 		      sizeof(interface.name));
 	if (ret < 0)
 		return -ENAMETOOLONG;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 7ea511d59e91..f279edfce3f1 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -606,7 +606,7 @@ static void hns3_get_drvinfo(struct net_device *netdev,
 		return;
 	}
 
-	strncpy(drvinfo->driver, h->pdev->driver->name,
+	strncpy(drvinfo->driver, dev_driver_string(&h->pdev->dev),
 		sizeof(drvinfo->driver));
 	drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';
 
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
index a250d394da38..a8f007f6dad2 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
@@ -720,7 +720,7 @@ static int prestera_fw_load(struct prestera_fw *fw)
 static int prestera_pci_probe(struct pci_dev *pdev,
 			      const struct pci_device_id *id)
 {
-	const char *driver_name = pdev->driver->name;
+	const char *driver_name = dev_driver_string(&pdev->dev);
 	struct prestera_fw *fw;
 	int err;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
index 13b0259f7ea6..8f306364f7bf 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
@@ -1876,7 +1876,7 @@ static void mlxsw_pci_cmd_fini(struct mlxsw_pci *mlxsw_pci)
 
 static int mlxsw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
-	const char *driver_name = pdev->driver->name;
+	const char *driver_name = dev_driver_string(&pdev->dev);
 	struct mlxsw_pci *mlxsw_pci;
 	int err;
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 0685ece1f155..1de076f55740 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -202,7 +202,8 @@ nfp_get_drvinfo(struct nfp_app *app, struct pci_dev *pdev,
 {
 	char nsp_version[ETHTOOL_FWVERS_LEN] = {};
 
-	strlcpy(drvinfo->driver, pdev->driver->name, sizeof(drvinfo->driver));
+	strlcpy(drvinfo->driver, dev_driver_string(&pdev->dev),
+		sizeof(drvinfo->driver));
 	nfp_net_get_nspinfo(app, nsp_version);
 	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
 		 "%s %s %s %s", vnic_version, nsp_version,
-- 
2.30.2

Re: [PATCH v6 07/11] PCI: Replace pci_dev::driver usage that gets the driver name

[Ido Schimmel]

On Mon, Oct 04, 2021 at 02:59:31PM +0200, Uwe Kleine-König wrote:
> struct pci_dev::driver holds (apart from a constant offset) the same
> data as struct pci_dev::dev->driver. With the goal to remove struct
> pci_dev::driver to get rid of data duplication replace getting the
> driver name by dev_driver_string() which implicitly makes use of struct
> pci_dev::dev->driver.
> 
> Acked-by: Simon Horman <simon.horman@corigine.com> (for NFP)
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

For mlxsw:

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>

Tested with the kexec flow that I mentioned last time. Works fine now.

Thanks

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Bjorn Helgaas]

On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> this is v6 of the quest to drop the "driver" member from struct pci_dev
> which tracks the same data (apart from a constant offset) as dev.driver.

I like this a lot and applied it to pci/driver for v5.16, thanks!

I split some of the bigger patches apart so they only touched one
driver or subsystem at a time.  I also updated to_pci_driver() so it
returns NULL when given NULL, which makes some of the validations
quite a bit simpler, especially in the PM code in pci-driver.c.

Full interdiff from this v6 series:

diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
index deaaef6efe34..36e84d904260 100644
--- a/arch/x86/kernel/probe_roms.c
+++ b/arch/x86/kernel/probe_roms.c
@@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
  */
 static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
 {
+	struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
 	const struct pci_device_id *id;
 
 	if (pdev->vendor == vendor && pdev->device == device)
 		return true;
 
-	if (pdev->dev.driver) {
-		struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
-		for (id = drv->id_table; id && id->vendor; id++)
-			if (id->vendor == vendor && id->device == device)
-				break;
-	}
+	for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
+		if (id->vendor == vendor && id->device == device)
+			break;
 
 	return id && id->vendor;
 }
diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
index d997c9c3ebb5..7eb3706cf42d 100644
--- a/drivers/misc/cxl/guest.c
+++ b/drivers/misc/cxl/guest.c
@@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
 				pci_channel_state_t state)
 {
 	struct pci_dev *afu_dev;
+	struct pci_driver *afu_drv;
+	struct pci_error_handlers *err_handler;
 
 	if (afu->phb == NULL)
 		return;
 
 	list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
-		struct pci_driver *afu_drv;
-
-		if (!afu_dev->dev.driver)
-			continue;
-
 		afu_drv = to_pci_driver(afu_dev->dev.driver);
+		if (!afu_drv)
+			continue;
 
+		err_handler = afu_drv->err_handler;
 		switch (bus_error_event) {
 		case CXL_ERROR_DETECTED_EVENT:
 			afu_dev->error_state = state;
 
-			if (afu_drv->err_handler &&
-			    afu_drv->err_handler->error_detected)
-				afu_drv->err_handler->error_detected(afu_dev, state);
-		break;
+			if (err_handler &&
+			    err_handler->error_detected)
+				err_handler->error_detected(afu_dev, state);
+			break;
 		case CXL_SLOT_RESET_EVENT:
 			afu_dev->error_state = state;
 
-			if (afu_drv->err_handler &&
-			    afu_drv->err_handler->slot_reset)
-				afu_drv->err_handler->slot_reset(afu_dev);
-		break;
+			if (err_handler &&
+			    err_handler->slot_reset)
+				err_handler->slot_reset(afu_dev);
+			break;
 		case CXL_RESUME_EVENT:
-			if (afu_drv->err_handler &&
-			    afu_drv->err_handler->resume)
-				afu_drv->err_handler->resume(afu_dev);
-		break;
+			if (err_handler &&
+			    err_handler->resume)
+				err_handler->resume(afu_dev);
+			break;
 		}
 	}
 }
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 7e7545d01e27..08bd81854101 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1795,6 +1795,8 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
 						pci_channel_state_t state)
 {
 	struct pci_dev *afu_dev;
+	struct pci_driver *afu_drv;
+	struct pci_error_handlers *err_handler;
 	pci_ers_result_t result = PCI_ERS_RESULT_NEED_RESET;
 	pci_ers_result_t afu_result = PCI_ERS_RESULT_NEED_RESET;
 
@@ -1805,16 +1807,16 @@ static pci_ers_result_t cxl_vphb_error_detected(struct cxl_afu *afu,
 		return result;
 
 	list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
-		struct pci_driver *afu_drv;
-		if (!afu_dev->dev.driver)
-			continue;
-
 		afu_drv = to_pci_driver(afu_dev->dev.driver);
+		if (!afu_drv)
+			continue;
 
 		afu_dev->error_state = state;
 
-		if (afu_drv->err_handler)
-			afu_result = afu_drv->err_handler->error_detected(afu_dev, state);
+		err_handler = afu_drv->err_handler;
+		if (err_handler)
+			afu_result = err_handler->error_detected(afu_dev,
+								 state);
 		/* Disconnect trumps all, NONE trumps NEED_RESET */
 		if (afu_result == PCI_ERS_RESULT_DISCONNECT)
 			result = PCI_ERS_RESULT_DISCONNECT;
@@ -1974,6 +1976,8 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
 	struct cxl_afu *afu;
 	struct cxl_context *ctx;
 	struct pci_dev *afu_dev;
+	struct pci_driver *afu_drv;
+	struct pci_error_handlers *err_handler;
 	pci_ers_result_t afu_result = PCI_ERS_RESULT_RECOVERED;
 	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
 	int i;
@@ -2005,8 +2009,6 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
 			continue;
 
 		list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
-			struct pci_driver *afu_drv;
-
 			/* Reset the device context.
 			 * TODO: make this less disruptive
 			 */
@@ -2032,14 +2034,13 @@ static pci_ers_result_t cxl_pci_slot_reset(struct pci_dev *pdev)
 			 * shouldn't start new work until we call
 			 * their resume function.
 			 */
-			if (!afu_dev->dev.driver)
-				continue;
-
 			afu_drv = to_pci_driver(afu_dev->dev.driver);
+			if (!afu_drv)
+				continue;
 
-			if (afu_drv->err_handler &&
-			    afu_drv->err_handler->slot_reset)
-				afu_result = afu_drv->err_handler->slot_reset(afu_dev);
+			err_handler = afu_drv->err_handler;
+			if (err_handler && err_handler->slot_reset)
+				afu_result = err_handler->slot_reset(afu_dev);
 
 			if (afu_result == PCI_ERS_RESULT_DISCONNECT)
 				result = PCI_ERS_RESULT_DISCONNECT;
@@ -2066,6 +2067,8 @@ static void cxl_pci_resume(struct pci_dev *pdev)
 	struct cxl *adapter = pci_get_drvdata(pdev);
 	struct cxl_afu *afu;
 	struct pci_dev *afu_dev;
+	struct pci_driver *afu_drv;
+	struct pci_error_handlers *err_handler;
 	int i;
 
 	/* Everything is back now. Drivers should restart work now.
@@ -2080,11 +2083,13 @@ static void cxl_pci_resume(struct pci_dev *pdev)
 			continue;
 
 		list_for_each_entry(afu_dev, &afu->phb->bus->devices, bus_list) {
-			struct pci_driver *afu_drv;
-			if (afu_dev->dev.driver &&
-			    (afu_drv = to_pci_driver(afu_dev->dev.driver))->err_handler &&
-			    afu_drv->err_handler->resume)
-				afu_drv->err_handler->resume(afu_dev);
+			afu_drv = to_pci_driver(afu_dev->dev.driver);
+			if (!afu_drv)
+				continue;
+
+			err_handler = afu_drv->err_handler;
+			if (err_handler && err_handler->resume)
+				err_handler->resume(afu_dev);
 		}
 	}
 	spin_unlock(&adapter->afu_list_lock);
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 0d0a34347868..fa4b52bb1e05 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -168,11 +168,8 @@ static ssize_t sriov_vf_total_msix_show(struct device *dev,
 	u32 vf_total_msix = 0;
 
 	device_lock(dev);
-	if (!dev->driver)
-		goto unlock;
-
 	pdrv = to_pci_driver(dev->driver);
-	if (!pdrv->sriov_get_vf_total_msix)
+	if (!pdrv || !pdrv->sriov_get_vf_total_msix)
 		goto unlock;
 
 	vf_total_msix = pdrv->sriov_get_vf_total_msix(pdev);
@@ -199,19 +196,14 @@ static ssize_t sriov_vf_msix_count_store(struct device *dev,
 		return -EINVAL;
 
 	device_lock(&pdev->dev);
-	if (!pdev->dev.driver) {
-		ret = -EOPNOTSUPP;
-		goto err_pdev;
-	}
-
-	pdrv = to_pci_driver(pdev->dev.driver);
-	if (!pdrv->sriov_set_msix_vec_count) {
+	pdrv = to_pci_driver(dev->driver);
+	if (!pdrv || !pdrv->sriov_set_msix_vec_count) {
 		ret = -EOPNOTSUPP;
 		goto err_pdev;
 	}
 
 	device_lock(&vf_dev->dev);
-	if (vf_dev->dev.driver) {
+	if (to_pci_driver(vf_dev->dev.driver)) {
 		/*
 		 * A driver is already attached to this VF and has configured
 		 * itself based on the current MSI-X vector count. Changing
@@ -405,14 +397,13 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 		goto exit;
 
 	/* is PF driver loaded */
-	if (!pdev->dev.driver) {
+	pdrv = to_pci_driver(dev->driver);
+	if (!pdrv) {
 		pci_info(pdev, "no driver bound to device; cannot configure SR-IOV\n");
 		ret = -ENOENT;
 		goto exit;
 	}
 
-	pdrv = to_pci_driver(pdev->dev.driver);
-
 	/* is PF driver loaded w/callback */
 	if (!pdrv->sriov_configure) {
 		pci_info(pdev, "driver does not support SR-IOV configuration via sysfs\n");
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index e94aa338bab4..3884a1542e86 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -454,7 +454,7 @@ static int pci_device_probe(struct device *dev)
 static void pci_device_remove(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
+	struct pci_driver *drv = to_pci_driver(dev->driver);
 
 	if (drv->remove) {
 		pm_runtime_get_sync(dev);
@@ -489,15 +489,12 @@ static void pci_device_remove(struct device *dev)
 static void pci_device_shutdown(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct pci_driver *drv = to_pci_driver(dev->driver);
 
 	pm_runtime_resume(dev);
 
-	if (pci_dev->dev.driver) {
-		struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
-
-		if (drv->shutdown)
-			drv->shutdown(pci_dev);
-	}
+	if (drv && drv->shutdown)
+		drv->shutdown(pci_dev);
 
 	/*
 	 * If this is a kexec reboot, turn off Bus Master bit on the
@@ -588,25 +585,22 @@ static int pci_pm_reenable_device(struct pci_dev *pci_dev)
 static int pci_legacy_suspend(struct device *dev, pm_message_t state)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct pci_driver *drv = to_pci_driver(dev->driver);
 
-	if (dev->driver) {
-		struct pci_driver *drv = to_pci_driver(dev->driver);
+	if (drv && drv->suspend) {
+		pci_power_t prev = pci_dev->current_state;
+		int error;
 
-		if (drv->suspend) {
-			pci_power_t prev = pci_dev->current_state;
-			int error;
+		error = drv->suspend(pci_dev, state);
+		suspend_report_result(drv->suspend, error);
+		if (error)
+			return error;
 
-			error = drv->suspend(pci_dev, state);
-			suspend_report_result(drv->suspend, error);
-			if (error)
-				return error;
-
-			if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
-			    && pci_dev->current_state != PCI_UNKNOWN) {
-				pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
-					      "PCI PM: Device state not saved by %pS\n",
-					      drv->suspend);
-			}
+		if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
+		    && pci_dev->current_state != PCI_UNKNOWN) {
+			pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
+				      "PCI PM: Device state not saved by %pS\n",
+				      drv->suspend);
 		}
 	}
 
@@ -632,17 +626,12 @@ static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
 static int pci_legacy_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct pci_driver *drv = to_pci_driver(dev->driver);
 
 	pci_fixup_device(pci_fixup_resume, pci_dev);
 
-	if (pci_dev->dev.driver) {
-		struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
-
-		if (drv->resume)
-			return drv->resume(pci_dev);
-	}
-
-	return pci_pm_reenable_device(pci_dev);
+	return drv && drv->resume ?
+			drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
 }
 
 /* Auxiliary functions used by the new power management framework */
@@ -656,14 +645,8 @@ static void pci_pm_default_suspend(struct pci_dev *pci_dev)
 
 static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
 {
-	struct pci_driver *drv;
-	bool ret;
-
-	if (!pci_dev->dev.driver)
-		return false;
-
-	drv = to_pci_driver(pci_dev->dev.driver);
-	ret = drv && (drv->suspend || drv->resume);
+	struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
+	bool ret = drv && (drv->suspend || drv->resume);
 
 	/*
 	 * Legacy PM support is used by default, so warn if the new framework is
@@ -1255,11 +1238,11 @@ static int pci_pm_runtime_suspend(struct device *dev)
 	int error;
 
 	/*
-	 * If pci_dev->dev.driver is not set (unbound), we leave the device in D0,
-	 * but it may go to D3cold when the bridge above it runtime suspends.
-	 * Save its config space in case that happens.
+	 * If the device has no driver, we leave it in D0, but it may go to
+	 * D3cold when the bridge above it runtime suspends.  Save its
+	 * config space in case that happens.
 	 */
-	if (!pci_dev->dev.driver) {
+	if (!to_pci_driver(dev->driver)) {
 		pci_save_state(pci_dev);
 		return 0;
 	}
@@ -1316,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev)
 	 */
 	pci_restore_standard_config(pci_dev);
 
-	if (!dev->driver)
+	if (!to_pci_driver(dev->driver))
 		return 0;
 
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
@@ -1335,13 +1318,14 @@ static int pci_pm_runtime_resume(struct device *dev)
 
 static int pci_pm_runtime_idle(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
 	/*
-	 * If dev->driver is not set (unbound), the device should
-	 * always remain in D0 regardless of the runtime PM status
+	 * If the device has no driver, it should always remain in D0
+	 * regardless of the runtime PM status
 	 */
-	if (!dev->driver)
+	if (!to_pci_driver(dev->driver))
 		return 0;
 
 	if (!pm)
@@ -1448,8 +1432,10 @@ static struct pci_driver pci_compat_driver = {
  */
 struct pci_driver *pci_dev_driver(const struct pci_dev *dev)
 {
-	if (dev->dev.driver)
-		return to_pci_driver(dev->dev.driver);
+	struct pci_driver *drv = to_pci_driver(dev->dev.driver);
+
+	if (drv)
+		return drv;
 	else {
 		int i;
 		for (i = 0; i <= PCI_ROM_RESOURCE; i++)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ccecf740de59..5298ce131f8c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5088,13 +5088,14 @@ EXPORT_SYMBOL_GPL(pci_dev_unlock);
 
 static void pci_dev_save_and_disable(struct pci_dev *dev)
 {
+	struct pci_driver *drv = to_pci_driver(dev->dev.driver);
 	const struct pci_error_handlers *err_handler =
-			dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
+			drv ? drv->err_handler : NULL;
 
 	/*
-	 * dev->driver->err_handler->reset_prepare() is protected against
-	 * races with ->remove() by the device lock, which must be held by
-	 * the caller.
+	 * drv->err_handler->reset_prepare() is protected against races
+	 * with ->remove() by the device lock, which must be held by the
+	 * caller.
 	 */
 	if (err_handler && err_handler->reset_prepare)
 		err_handler->reset_prepare(dev);
@@ -5119,15 +5120,15 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
 
 static void pci_dev_restore(struct pci_dev *dev)
 {
+	struct pci_driver *drv = to_pci_driver(dev->dev.driver);
 	const struct pci_error_handlers *err_handler =
-			dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
+			drv ? drv->err_handler : NULL;
 
 	pci_restore_state(dev);
 
 	/*
-	 * dev->driver->err_handler->reset_done() is protected against
-	 * races with ->remove() by the device lock, which must be held by
-	 * the caller.
+	 * drv->err_handler->reset_done() is protected against races with
+	 * ->remove() by the device lock, which must be held by the caller.
 	 */
 	if (err_handler && err_handler->reset_done)
 		err_handler->reset_done(dev);
diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index b314b54f7821..42385fe6b7fa 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -54,9 +54,10 @@ static int report_error_detected(struct pci_dev *dev,
 	const struct pci_error_handlers *err_handler;
 
 	device_lock(&dev->dev);
+	pdrv = to_pci_driver(dev->dev.driver);
 	if (!pci_dev_set_io_state(dev, state) ||
-		!dev->dev.driver ||
-		!(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
+		!pdrv ||
+		!pdrv->err_handler ||
 		!pdrv->err_handler->error_detected) {
 		/*
 		 * If any device in the subtree does not have an error_detected
@@ -92,13 +93,14 @@ static int report_normal_detected(struct pci_dev *dev, void *data)
 
 static int report_mmio_enabled(struct pci_dev *dev, void *data)
 {
-	pci_ers_result_t vote, *result = data;
 	struct pci_driver *pdrv;
+	pci_ers_result_t vote, *result = data;
 	const struct pci_error_handlers *err_handler;
 
 	device_lock(&dev->dev);
-	if (!dev->dev.driver ||
-		!(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
+	pdrv = to_pci_driver(dev->dev.driver);
+	if (!pdrv ||
+		!pdrv->err_handler ||
 		!pdrv->err_handler->mmio_enabled)
 		goto out;
 
@@ -112,13 +114,14 @@ static int report_mmio_enabled(struct pci_dev *dev, void *data)
 
 static int report_slot_reset(struct pci_dev *dev, void *data)
 {
+	struct pci_driver *pdrv;
 	pci_ers_result_t vote, *result = data;
 	const struct pci_error_handlers *err_handler;
-	struct pci_driver *pdrv;
 
 	device_lock(&dev->dev);
-	if (!dev->dev.driver ||
-		!(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
+	pdrv = to_pci_driver(dev->dev.driver);
+	if (!pdrv ||
+		!pdrv->err_handler ||
 		!pdrv->err_handler->slot_reset)
 		goto out;
 
@@ -132,13 +135,14 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
 
 static int report_resume(struct pci_dev *dev, void *data)
 {
-	const struct pci_error_handlers *err_handler;
 	struct pci_driver *pdrv;
+	const struct pci_error_handlers *err_handler;
 
 	device_lock(&dev->dev);
+	pdrv = dev->driver;
 	if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
-		!dev->dev.driver ||
-		!(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
+		!pdrv ||
+		!pdrv->err_handler ||
 		!pdrv->err_handler->resume)
 		goto out;
 
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index 73831fb87a1e..0ec76b4af16f 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -588,7 +588,6 @@ static pci_ers_result_t pcifront_common_process(int cmd,
 						struct pcifront_device *pdev,
 						pci_channel_state_t state)
 {
-	pci_ers_result_t result;
 	struct pci_driver *pdrv;
 	int bus = pdev->sh_info->aer_op.bus;
 	int devfn = pdev->sh_info->aer_op.devfn;
@@ -598,13 +597,12 @@ static pci_ers_result_t pcifront_common_process(int cmd,
 	dev_dbg(&pdev->xdev->dev,
 		"pcifront AER process: cmd %x (bus:%x, devfn%x)",
 		cmd, bus, devfn);
-	result = PCI_ERS_RESULT_NONE;
 
 	pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
 	if (!pcidev || !pcidev->dev.driver) {
 		dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n");
 		pci_dev_put(pcidev);
-		return result;
+		return PCI_ERS_RESULT_NONE;
 	}
 	pdrv = to_pci_driver(pcidev->dev.driver);
 
@@ -612,27 +610,18 @@ static pci_ers_result_t pcifront_common_process(int cmd,
 		pci_dbg(pcidev, "trying to call AER service\n");
 		switch (cmd) {
 		case XEN_PCI_OP_aer_detected:
-			result = pdrv->err_handler->
-				 error_detected(pcidev, state);
-			break;
+			return pdrv->err_handler->error_detected(pcidev, state);
 		case XEN_PCI_OP_aer_mmio:
-			result = pdrv->err_handler->
-				 mmio_enabled(pcidev);
-			break;
+			return pdrv->err_handler->mmio_enabled(pcidev);
 		case XEN_PCI_OP_aer_slotreset:
-			result = pdrv->err_handler->
-				 slot_reset(pcidev);
-			break;
+			return pdrv->err_handler->slot_reset(pcidev);
 		case XEN_PCI_OP_aer_resume:
 			pdrv->err_handler->resume(pcidev);
-			break;
+			return PCI_ERS_RESULT_NONE;
 		default:
 			dev_err(&pdev->xdev->dev,
-				"bad request in aer recovery "
-				"operation!\n");
+				"bad request in AER recovery operation!\n");
 		}
-
-		return result;
 	}
 
 	return PCI_ERS_RESULT_NONE;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7c1ceb39035c..03bfdb25a55c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -899,7 +899,10 @@ struct pci_driver {
 	struct pci_dynids	dynids;
 };
 
-#define	to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
+static inline struct pci_driver *to_pci_driver(struct device_driver *drv)
+{
+    return drv ? container_of(drv, struct pci_driver, driver) : NULL;
+}
 
 /**
  * PCI_DEVICE - macro used to describe a specific PCI device

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]

On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > this is v6 of the quest to drop the "driver" member from struct pci_dev
> > which tracks the same data (apart from a constant offset) as dev.driver.
> 
> I like this a lot and applied it to pci/driver for v5.16, thanks!
> 
> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time.  I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.

OK.

> Full interdiff from this v6 series:
> 
> diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> index deaaef6efe34..36e84d904260 100644
> --- a/arch/x86/kernel/probe_roms.c
> +++ b/arch/x86/kernel/probe_roms.c
> @@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
>   */
>  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
>  {
> +	struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
>  	const struct pci_device_id *id;
>  
>  	if (pdev->vendor == vendor && pdev->device == device)
>  		return true;
>  
> -	if (pdev->dev.driver) {
> -		struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> -		for (id = drv->id_table; id && id->vendor; id++)
> -			if (id->vendor == vendor && id->device == device)
> -				break;
> -	}
> +	for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> +		if (id->vendor == vendor && id->device == device)
> +			break;
>  
>  	return id && id->vendor;
>  }
> diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> index d997c9c3ebb5..7eb3706cf42d 100644
> --- a/drivers/misc/cxl/guest.c
> +++ b/drivers/misc/cxl/guest.c
> @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
>  				pci_channel_state_t state)
>  {
>  	struct pci_dev *afu_dev;
> +	struct pci_driver *afu_drv;
> +	struct pci_error_handlers *err_handler;

These two could be moved into the for loop (where afu_drv was with my
patch already). This is also possible in a few other drivers.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time.  I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.

It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.

Below are some comments as well.

...

>  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
>  {
> +       struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
>         const struct pci_device_id *id;
>
>         if (pdev->vendor == vendor && pdev->device == device)
>                 return true;

> +       for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> +               if (id->vendor == vendor && id->device == device)

> +                       break;

return true;

>         return id && id->vendor;

return false;

>  }

...

> +                       afu_result = err_handler->error_detected(afu_dev,
> +                                                                state);

One line?

...

>         device_lock(&vf_dev->dev);
> -       if (vf_dev->dev.driver) {
> +       if (to_pci_driver(vf_dev->dev.driver)) {

Hmm...

...

> +               if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0

> +                   && pci_dev->current_state != PCI_UNKNOWN) {

Can we keep && on the previous line?

> +                       pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> +                                     "PCI PM: Device state not saved by %pS\n",
> +                                     drv->suspend);
>                 }

...

> +       return drv && drv->resume ?
> +                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);

One line?

...

> +       struct pci_driver *drv = to_pci_driver(dev->dev.driver);
>         const struct pci_error_handlers *err_handler =
> -                       dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
> +                       drv ? drv->err_handler : NULL;

Isn't dev->driver == to_pci_driver(dev->dev.driver)?

...

> +       struct pci_driver *drv = to_pci_driver(dev->dev.driver);
>         const struct pci_error_handlers *err_handler =
> -                       dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
> +                       drv ? drv->err_handler : NULL;

Ditto.

...

>         device_lock(&dev->dev);
> +       pdrv = to_pci_driver(dev->dev.driver);
>         if (!pci_dev_set_io_state(dev, state) ||
> -               !dev->dev.driver ||
> -               !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||

> +               !pdrv ||
> +               !pdrv->err_handler ||

One line now?

>                 !pdrv->err_handler->error_detected) {

Or this and the previous line?

...

> +       pdrv = to_pci_driver(dev->dev.driver);
> +       if (!pdrv ||
> +               !pdrv->err_handler ||
>                 !pdrv->err_handler->mmio_enabled)
>                 goto out;

Ditto.

...

> +       pdrv = to_pci_driver(dev->dev.driver);
> +       if (!pdrv ||
> +               !pdrv->err_handler ||
>                 !pdrv->err_handler->slot_reset)
>                 goto out;

Ditto.

...

>         if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
> -               !dev->dev.driver ||
> -               !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> +               !pdrv ||
> +               !pdrv->err_handler ||
>                 !pdrv->err_handler->resume)
>                 goto out;

Ditto.

> -       result = PCI_ERS_RESULT_NONE;
>
>         pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>         if (!pcidev || !pcidev->dev.driver) {
>                 dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n");
>                 pci_dev_put(pcidev);
> -               return result;
> +               return PCI_ERS_RESULT_NONE;
>         }
>         pdrv = to_pci_driver(pcidev->dev.driver);

What about splitting the conditional to two with clear error message
in each and use pci_err() in the second one?

...

>                 default:
>                         dev_err(&pdev->xdev->dev,
> -                               "bad request in aer recovery "
> -                               "operation!\n");
> +                               "bad request in AER recovery operation!\n");

Stray change? Or is it in a separate patch in your tree?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Bjorn Helgaas]

On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote:
> On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > this is v6 of the quest to drop the "driver" member from struct pci_dev
> > > which tracks the same data (apart from a constant offset) as dev.driver.
> > 
> > I like this a lot and applied it to pci/driver for v5.16, thanks!
> > 
> > I split some of the bigger patches apart so they only touched one
> > driver or subsystem at a time.  I also updated to_pci_driver() so it
> > returns NULL when given NULL, which makes some of the validations
> > quite a bit simpler, especially in the PM code in pci-driver.c.
> 
> OK.
> 
> > Full interdiff from this v6 series:
> > 
> > diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> > index deaaef6efe34..36e84d904260 100644
> > --- a/arch/x86/kernel/probe_roms.c
> > +++ b/arch/x86/kernel/probe_roms.c
> > @@ -80,17 +80,15 @@ static struct resource video_rom_resource = {
> >   */
> >  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
> >  {
> > +	struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> >  	const struct pci_device_id *id;
> >  
> >  	if (pdev->vendor == vendor && pdev->device == device)
> >  		return true;
> >  
> > -	if (pdev->dev.driver) {
> > -		struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> > -		for (id = drv->id_table; id && id->vendor; id++)
> > -			if (id->vendor == vendor && id->device == device)
> > -				break;
> > -	}
> > +	for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > +		if (id->vendor == vendor && id->device == device)
> > +			break;
> >  
> >  	return id && id->vendor;
> >  }
> > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > index d997c9c3ebb5..7eb3706cf42d 100644
> > --- a/drivers/misc/cxl/guest.c
> > +++ b/drivers/misc/cxl/guest.c
> > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> >  				pci_channel_state_t state)
> >  {
> >  	struct pci_dev *afu_dev;
> > +	struct pci_driver *afu_drv;
> > +	struct pci_error_handlers *err_handler;
> 
> These two could be moved into the for loop (where afu_drv was with my
> patch already). This is also possible in a few other drivers.

That's true, they could.  I tried to follow the prevailing style in
the file.  At least in cxl, I didn't see any other cases of
declarations being in the minimal scope like that.

Bjorn

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

Hello,

On Wed, Oct 13, 2021 at 05:54:28AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 10:51:31AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 12, 2021 at 06:32:12PM -0500, Bjorn Helgaas wrote:
> > > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > > index d997c9c3ebb5..7eb3706cf42d 100644
> > > --- a/drivers/misc/cxl/guest.c
> > > +++ b/drivers/misc/cxl/guest.c
> > > @@ -20,38 +20,38 @@ static void pci_error_handlers(struct cxl_afu *afu,
> > >  				pci_channel_state_t state)
> > >  {
> > >  	struct pci_dev *afu_dev;
> > > +	struct pci_driver *afu_drv;
> > > +	struct pci_error_handlers *err_handler;
> > 
> > These two could be moved into the for loop (where afu_drv was with my
> > patch already). This is also possible in a few other drivers.
> 
> That's true, they could.  I tried to follow the prevailing style in
> the file.  At least in cxl, I didn't see any other cases of
> declarations being in the minimal scope like that.

I don't care much, do whatever you consider nice. I'm happy you liked
the cleanup and that you took it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Bjorn Helgaas]

On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> 
> > I split some of the bigger patches apart so they only touched one
> > driver or subsystem at a time.  I also updated to_pci_driver() so it
> > returns NULL when given NULL, which makes some of the validations
> > quite a bit simpler, especially in the PM code in pci-driver.c.
> 
> It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.

It is a little unusual.  I only found three of 77 that are NULL-aware:

  to_moxtet_driver()
  to_siox_driver()
  to_spi_driver()

It seems worthwhile to me because it makes the patch and the resulting
code significantly cleaner.  Here's one example without the NULL
check:

  @@ -493,12 +493,15 @@ static void pci_device_remove(struct device *dev)
   static void pci_device_shutdown(struct device *dev)
   {
          struct pci_dev *pci_dev = to_pci_dev(dev);
  -       struct pci_driver *drv = pci_dev->driver;

          pm_runtime_resume(dev);

  -       if (drv && drv->shutdown)
  -               drv->shutdown(pci_dev);
  +       if (pci_dev->dev.driver) {
  +               struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
  +
  +               if (drv->shutdown)
  +                       drv->shutdown(pci_dev);
  +       }

  static void pci_device_shutdown(struct device *dev)
  {
    struct pci_dev *pci_dev = to_pci_dev(dev);

    pm_runtime_resume(dev);

    if (pci_dev->dev.driver) {
      struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);

      if (drv->shutdown)
        drv->shutdown(pci_dev);
    }

and here's the same thing with the NULL check:

  @@ -493,7 +493,7 @@ static void pci_device_remove(struct device *dev)
   static void pci_device_shutdown(struct device *dev)
   {
          struct pci_dev *pci_dev = to_pci_dev(dev);
  -       struct pci_driver *drv = pci_dev->driver;
  +       struct pci_driver *drv = to_pci_driver(dev->driver);

  static void pci_device_shutdown(struct device *dev)
  {
    struct pci_dev *pci_dev = to_pci_dev(dev);
    struct pci_driver *drv = to_pci_driver(dev->driver);

    pm_runtime_resume(dev);

    if (drv && drv->shutdown)
      drv->shutdown(pci_dev);

> >  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
> >  {
> > +       struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> >         const struct pci_device_id *id;
> >
> >         if (pdev->vendor == vendor && pdev->device == device)
> >                 return true;
> 
> > +       for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > +               if (id->vendor == vendor && id->device == device)
> 
> > +                       break;
> 
> return true;
> 
> >         return id && id->vendor;
> 
> return false;

Good cleanup for a follow-up patch, but doesn't seem directly related
to the objective here.  The current patch is:

  @@ -80,7 +80,7 @@ static struct resource video_rom_resource = {
    */
   static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned short device)
   {
  -       struct pci_driver *drv = pdev->driver;
  +       struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
          const struct pci_device_id *id;

          if (pdev->vendor == vendor && pdev->device == device)

> >         device_lock(&vf_dev->dev);
> > -       if (vf_dev->dev.driver) {
> > +       if (to_pci_driver(vf_dev->dev.driver)) {
> 
> Hmm...

Yeah, it could be either of:

  if (to_pci_driver(vf_dev->dev.driver))
  if (vf_dev->dev.driver)

I went back and forth on that and went with to_pci_driver() on the
theory that we were testing the pci_driver * before and the patch is
more of a mechanical change and easier to review if we test the
pci_driver * after.

> > +               if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
> 
> > +                   && pci_dev->current_state != PCI_UNKNOWN) {
> 
> Can we keep && on the previous line?

I think this is in pci_legacy_suspend(), and I didn't touch that line.
It shows up in the interdiff because without the NULL check in
to_pci_driver(), we had to indent this code another level.  With the
NULL check, we don't need that extra indentation.

> > +                       pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> > +                                     "PCI PM: Device state not saved by %pS\n",
> > +                                     drv->suspend);
> >                 }
> 
> ...
> 
> > +       return drv && drv->resume ?
> > +                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
> 
> One line?

I don't think I touched that line.

> > +       struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> >         const struct pci_error_handlers *err_handler =
> > -                       dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
> > +                       drv ? drv->err_handler : NULL;
> 
> Isn't dev->driver == to_pci_driver(dev->dev.driver)?

Yes, I think so, but not sure what you're getting at here, can you
elaborate?

> >         device_lock(&dev->dev);
> > +       pdrv = to_pci_driver(dev->dev.driver);
> >         if (!pci_dev_set_io_state(dev, state) ||
> > -               !dev->dev.driver ||
> > -               !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> 
> > +               !pdrv ||
> > +               !pdrv->err_handler ||
> 
> One line now?
> 
> >                 !pdrv->err_handler->error_detected) {
> 
> Or this and the previous line?

Could, but the "dev->driver" to "to_pci_driver(dev->dev.driver)"
changes are the heart of this patch, and I don't like to clutter it
with unrelated changes.

> > -       result = PCI_ERS_RESULT_NONE;
> >
> >         pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> >         if (!pcidev || !pcidev->dev.driver) {
> >                 dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n");
> >                 pci_dev_put(pcidev);
> > -               return result;
> > +               return PCI_ERS_RESULT_NONE;
> >         }
> >         pdrv = to_pci_driver(pcidev->dev.driver);
> 
> What about splitting the conditional to two with clear error message
> in each and use pci_err() in the second one?

Could possibly be cleaned up.  Felt like feature creep so I didn't.

> >                 default:
> >                         dev_err(&pdev->xdev->dev,
> > -                               "bad request in aer recovery "
> > -                               "operation!\n");
> > +                               "bad request in AER recovery operation!\n");
> 
> Stray change? Or is it in a separate patch in your tree?

Could be skipped.  The string now fits on one line so I combined it to
make it more greppable.

Bjorn

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

...

> > It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.
> 
> It is a little unusual.  I only found three of 77 that are NULL-aware:
> 
>   to_moxtet_driver()
>   to_siox_driver()
>   to_spi_driver()
> 
> It seems worthwhile to me because it makes the patch and the resulting
> code significantly cleaner.

I'm not objecting the change, just a remark.

...

> > > +       for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > > +               if (id->vendor == vendor && id->device == device)
> > 
> > > +                       break;
> > 
> > return true;
> > 
> > >         return id && id->vendor;
> > 
> > return false;
> 
> Good cleanup for a follow-up patch, but doesn't seem directly related
> to the objective here.

True. Maybe you can bake one while not forgotten?

...

> > > +       return drv && drv->resume ?
> > > +                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
> > 
> > One line?
> 
> I don't think I touched that line.

Then why they are both in + section?

...

> > > +       struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> > >         const struct pci_error_handlers *err_handler =
> > > -                       dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
> > > +                       drv ? drv->err_handler : NULL;
> > 
> > Isn't dev->driver == to_pci_driver(dev->dev.driver)?
> 
> Yes, I think so, but not sure what you're getting at here, can you
> elaborate?

Getting pointer from another pointer seems waste of resources, why we
can't simply

	struct pci_driver *drv = dev->driver;

?

...

> > Stray change? Or is it in a separate patch in your tree?
> 
> Could be skipped.  The string now fits on one line so I combined it to
> make it more greppable.

This is inconsistency in your changes, in one case you are objecting of
doing something close to the changed lines, in the other you are doing
unrelated change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Bjorn Helgaas]

On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> > > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

> > > > +       return drv && drv->resume ?
> > > > +                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
> > > 
> > > One line?
> > 
> > I don't think I touched that line.
> 
> Then why they are both in + section?

They're both in the + section of the interdiff because Uwe's v6 patch
looks like this:

  static int pci_legacy_resume(struct device *dev)
  {
          struct pci_dev *pci_dev = to_pci_dev(dev);
  -       return drv && drv->resume ?
  -                       drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
  +       if (pci_dev->dev.driver) {
  +               struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
  +
  +               if (drv->resume)
  +                       return drv->resume(pci_dev);
  +       }
  +
  +       return pci_pm_reenable_device(pci_dev);

and my revision looks like this:

   static int pci_legacy_resume(struct device *dev)
   {
	  struct pci_dev *pci_dev = to_pci_dev(dev);
  -       struct pci_driver *drv = pci_dev->driver;
  +       struct pci_driver *drv = to_pci_driver(dev->driver);

so compared to Uwe's v6, I restored that section to the original code.
My goal here was to make the patch as simple and easy to review as
possible.

> > > > +       struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> > > >         const struct pci_error_handlers *err_handler =
> > > > -                       dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
> > > > +                       drv ? drv->err_handler : NULL;
> > > 
> > > Isn't dev->driver == to_pci_driver(dev->dev.driver)?
> > 
> > Yes, I think so, but not sure what you're getting at here, can you
> > elaborate?
> 
> Getting pointer from another pointer seems waste of resources, why we
> can't simply
> 
> 	struct pci_driver *drv = dev->driver;

I think this is in pci_dev_save_and_disable(), and "dev" here is a
struct pci_dev *.  We're removing the dev->driver member.  Let me know
if I'm still missing something.

> > > > -                               "bad request in aer recovery "
> > > > -                               "operation!\n");
> > > > +                               "bad request in AER recovery operation!\n");

> > > Stray change? Or is it in a separate patch in your tree?
> > 
> > Could be skipped.  The string now fits on one line so I combined it to
> > make it more greppable.
> 
> This is inconsistency in your changes, in one case you are objecting of
> doing something close to the changed lines, in the other you are doing
> unrelated change.

You're right, this didn't make much sense in that patch.  I moved the
line join to the previous patch, which unindented this section and
made space for this to fit on one line.  Here's the revised commit:

  https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Fri, Oct 15, 2021 at 7:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:

...

> so compared to Uwe's v6, I restored that section to the original code.
> My goal here was to make the patch as simple and easy to review as
> possible.

Thanks for elaboration. I have got it.

...

> You're right, this didn't make much sense in that patch.  I moved the
> line join to the previous patch, which unindented this section and
> made space for this to fit on one line.  Here's the revised commit:
>
>   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287

Side remark: default without break or return is error prone (okay, to
some extent). Perhaps adding the return statement there will make
things robust and clean.

-- 
With Best Regards,
Andy Shevchenko