[PATCH 0/3] r8169: extend PCI core and switch to device-managed functions in probe

[PATCH 0/3] r8169: extend PCI core and switch to device-managed functions in probe

[Heiner Kallweit]

Probe error path and remove callback can be significantly simplified
by using device-managed functions. To be able to do this in the r8169
driver we need a device-managed version of pci_set_mwi first.

Heiner Kallweit (3):
  PCI: introduce device-managed version of pci_set_mwi
  r8169: switch to device-managed functions in probe
  r8169: remove netif_napi_del in probe error path

 drivers/net/ethernet/realtek/r8169.c | 87 +++++++++---------------------------
 drivers/pci/pci.c                    | 29 ++++++++++++
 include/linux/pci.h                  |  1 +
 3 files changed, 50 insertions(+), 67 deletions(-)

-- 
2.15.1

[PATCH 1/3] PCI: introduce a device-managed version of pci_set_mwi

[Heiner Kallweit]

Introduce a device-managed version of pci_set_mwi. First user is the
Realtek r8169 driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/pci.c   | 29 +++++++++++++++++++++++++++++
 include/linux/pci.h |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4a7c6864f..fc57c378d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1458,6 +1458,7 @@ struct pci_devres {
 	unsigned int pinned:1;
 	unsigned int orig_intx:1;
 	unsigned int restore_intx:1;
+	unsigned int mwi:1;
 	u32 region_mask;
 };
 
@@ -1476,6 +1477,9 @@ static void pcim_release(struct device *gendev, void *res)
 		if (this->region_mask & (1 << i))
 			pci_release_region(dev, i);
 
+	if (this->mwi)
+		pci_clear_mwi(dev);
+
 	if (this->restore_intx)
 		pci_intx(dev, this->orig_intx);
 
@@ -3760,6 +3764,31 @@ int pci_set_mwi(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_set_mwi);
 
+/**
+ * pcim_set_mwi - Managed pci_set_mwi()
+ * @dev: the PCI device for which MWI is enabled
+ *
+ * Managed pci_set_mwi().
+ *
+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
+ */
+int pcim_set_mwi(struct pci_dev *dev)
+{
+	struct pci_devres *dr;
+	int ret;
+
+	ret = pci_set_mwi(dev);
+	if (ret)
+		return ret;
+
+	dr = find_pci_dr(dev);
+	if (dr)
+		dr->mwi = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL(pcim_set_mwi);
+
 /**
  * pci_try_set_mwi - enables memory-write-invalidate PCI transaction
  * @dev: the PCI device for which MWI is enabled
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 978aad784..0a7ac863a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1064,6 +1064,7 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
 int pci_set_cacheline_size(struct pci_dev *dev);
 #define HAVE_PCI_SET_MWI
 int __must_check pci_set_mwi(struct pci_dev *dev);
+int __must_check pcim_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
-- 
2.15.1

Re: [PATCH 1/3] PCI: introduce a device-managed version of pci_set_mwi

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 10 Dec 2017 00:43:48 +0100

> Introduce a device-managed version of pci_set_mwi. First user is the
> Realtek r8169 driver.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Bjorn, can I get an ACK on this?  And would you be OK with this going
through my tree as the patches later in this series give an example of
usage?

Thanks.

Re: [PATCH 1/3] PCI: introduce a device-managed version of pci_set_mwi

[Bjorn Helgaas]

On Sun, Dec 10, 2017 at 12:43:48AM +0100, Heiner Kallweit wrote:
> Introduce a device-managed version of pci_set_mwi. First user is the
> Realtek r8169 driver.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

With the subject and changelog as follows and the code reordering below,

  PCI: Add pcim_set_mwi(), a device-managed pci_set_mwi()

  Add pcim_set_mwi(), a device-managed version of pci_set_mwi(). First user
  is the Realtek r8169 driver.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

With these changes, feel free to merge with the series via the netdev
tree.

> ---
>  drivers/pci/pci.c   | 29 +++++++++++++++++++++++++++++
>  include/linux/pci.h |  1 +
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4a7c6864f..fc57c378d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1458,6 +1458,7 @@ struct pci_devres {
>  	unsigned int pinned:1;
>  	unsigned int orig_intx:1;
>  	unsigned int restore_intx:1;
> +	unsigned int mwi:1;
>  	u32 region_mask;
>  };
>  
> @@ -1476,6 +1477,9 @@ static void pcim_release(struct device *gendev, void *res)
>  		if (this->region_mask & (1 << i))
>  			pci_release_region(dev, i);
>  
> +	if (this->mwi)
> +		pci_clear_mwi(dev);
> +
>  	if (this->restore_intx)
>  		pci_intx(dev, this->orig_intx);
>  
> @@ -3760,6 +3764,31 @@ int pci_set_mwi(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL(pci_set_mwi);
>  
> +/**
> + * pcim_set_mwi - Managed pci_set_mwi()
> + * @dev: the PCI device for which MWI is enabled
> + *
> + * Managed pci_set_mwi().
> + *
> + * RETURNS: An appropriate -ERRNO error value on error, or zero for success.

> + */
> +int pcim_set_mwi(struct pci_dev *dev)
> +{
> +	struct pci_devres *dr;
> +	int ret;
> +
> +	ret = pci_set_mwi(dev);
> +	if (ret)
> +		return ret;
> +
> +	dr = find_pci_dr(dev);
> +	if (dr)
> +		dr->mwi = 1;
> +
> +	return 0;

I would rather look up the pci_devres first, e.g.,

  dr = find_pci_dr(dev);
  if (!dr)
    return -ENOMEM;

  dr->mwi = 1;
  return pci_set_mwi(dev);

That way we won't enable MWI and be unable to disable it at release-time.

> +}
> +EXPORT_SYMBOL(pcim_set_mwi);
> +
>  /**
>   * pci_try_set_mwi - enables memory-write-invalidate PCI transaction
>   * @dev: the PCI device for which MWI is enabled
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 978aad784..0a7ac863a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1064,6 +1064,7 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
>  int pci_set_cacheline_size(struct pci_dev *dev);
>  #define HAVE_PCI_SET_MWI
>  int __must_check pci_set_mwi(struct pci_dev *dev);
> +int __must_check pcim_set_mwi(struct pci_dev *dev);
>  int pci_try_set_mwi(struct pci_dev *dev);
>  void pci_clear_mwi(struct pci_dev *dev);
>  void pci_intx(struct pci_dev *dev, int enable);
> -- 
> 2.15.1
> 
> 

[PATCH 2/3] r8169: switch to device-managed functions in probe

[Heiner Kallweit]

Simplify probe error path and remove callback by using device-managed
functions.

rtl_disable_msi isn't needed any longer because the release callback
of pcim_enable_device does this implicitely.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 80 +++++++++---------------------------
 1 file changed, 20 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index fc0d5fa65..3c7d90d3a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4643,16 +4643,6 @@ static void rtl8169_phy_timer(struct timer_list *t)
 	rtl_schedule_task(tp, RTL_FLAG_TASK_PHY_PENDING);
 }
 
-static void rtl8169_release_board(struct pci_dev *pdev, struct net_device *dev,
-				  void __iomem *ioaddr)
-{
-	iounmap(ioaddr);
-	pci_release_regions(pdev);
-	pci_clear_mwi(pdev);
-	pci_disable_device(pdev);
-	free_netdev(dev);
-}
-
 DECLARE_RTL_COND(rtl_phy_reset_cond)
 {
 	return tp->phy_reset_pending(tp);
@@ -4784,14 +4774,6 @@ static int rtl_tbi_ioctl(struct rtl8169_private *tp, struct mii_ioctl_data *data
 	return -EOPNOTSUPP;
 }
 
-static void rtl_disable_msi(struct pci_dev *pdev, struct rtl8169_private *tp)
-{
-	if (tp->features & RTL_FEATURE_MSI) {
-		pci_disable_msi(pdev);
-		tp->features &= ~RTL_FEATURE_MSI;
-	}
-}
-
 static void rtl_init_mdio_ops(struct rtl8169_private *tp)
 {
 	struct mdio_ops *ops = &tp->mdio_ops;
@@ -8256,9 +8238,6 @@ static void rtl_remove_one(struct pci_dev *pdev)
 
 	unregister_netdev(dev);
 
-	dma_free_coherent(&tp->pci_dev->dev, sizeof(*tp->counters),
-			  tp->counters, tp->counters_phys_addr);
-
 	rtl_release_firmware(tp);
 
 	if (pci_dev_run_wake(pdev))
@@ -8266,9 +8245,6 @@ static void rtl_remove_one(struct pci_dev *pdev)
 
 	/* restore original MAC address */
 	rtl_rar_set(tp, dev->perm_addr);
-
-	rtl_disable_msi(pdev, tp);
-	rtl8169_release_board(pdev, dev, tp->mmio_addr);
 }
 
 static const struct net_device_ops rtl_netdev_ops = {
@@ -8445,11 +8421,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		       MODULENAME, RTL8169_VERSION);
 	}
 
-	dev = alloc_etherdev(sizeof (*tp));
-	if (!dev) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
+	if (!dev)
+		return -ENOMEM;
 
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	dev->netdev_ops = &rtl_netdev_ops;
@@ -8472,13 +8446,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				     PCIE_LINK_STATE_CLKPM);
 
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
-	rc = pci_enable_device(pdev);
+	rc = pcim_enable_device(pdev);
 	if (rc < 0) {
 		netif_err(tp, probe, dev, "enable failure\n");
-		goto err_out_free_dev_1;
+		return rc;
 	}
 
-	if (pci_set_mwi(pdev) < 0)
+	if (pcim_set_mwi(pdev) < 0)
 		netif_info(tp, probe, dev, "Mem-Wr-Inval unavailable\n");
 
 	/* make sure PCI base addr 1 is MMIO */
@@ -8486,30 +8460,28 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		netif_err(tp, probe, dev,
 			  "region #%d not an MMIO resource, aborting\n",
 			  region);
-		rc = -ENODEV;
-		goto err_out_mwi_2;
+		return -ENODEV;
 	}
 
 	/* check for weird/broken PCI region reporting */
 	if (pci_resource_len(pdev, region) < R8169_REGS_SIZE) {
 		netif_err(tp, probe, dev,
 			  "Invalid PCI region size(s), aborting\n");
-		rc = -ENODEV;
-		goto err_out_mwi_2;
+		return -ENODEV;
 	}
 
 	rc = pci_request_regions(pdev, MODULENAME);
 	if (rc < 0) {
 		netif_err(tp, probe, dev, "could not request regions\n");
-		goto err_out_mwi_2;
+		return rc;
 	}
 
 	/* ioremap MMIO region */
-	ioaddr = ioremap(pci_resource_start(pdev, region), R8169_REGS_SIZE);
+	ioaddr = devm_ioremap(&pdev->dev, pci_resource_start(pdev, region),
+			      R8169_REGS_SIZE);
 	if (!ioaddr) {
 		netif_err(tp, probe, dev, "cannot remap MMIO, aborting\n");
-		rc = -EIO;
-		goto err_out_free_res_3;
+		return -EIO;
 	}
 	tp->mmio_addr = ioaddr;
 
@@ -8535,7 +8507,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
 		if (rc < 0) {
 			netif_err(tp, probe, dev, "DMA configuration failed\n");
-			goto err_out_unmap_4;
+			return rc;
 		}
 	}
 
@@ -8697,8 +8669,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	tp->rtl_fw = RTL_FIRMWARE_UNKNOWN;
 
-	tp->counters = dma_alloc_coherent (&pdev->dev, sizeof(*tp->counters),
-					   &tp->counters_phys_addr, GFP_KERNEL);
+	tp->counters = dmam_alloc_coherent (&pdev->dev, sizeof(*tp->counters),
+					    &tp->counters_phys_addr,
+					    GFP_KERNEL);
 	if (!tp->counters) {
 		rc = -ENOMEM;
 		goto err_out_msi_5;
@@ -8706,7 +8679,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	rc = register_netdev(dev);
 	if (rc < 0)
-		goto err_out_cnt_6;
+		goto err_out_msi_5;
 
 	pci_set_drvdata(pdev, dev);
 
@@ -8735,25 +8708,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	netif_carrier_off(dev);
 
-out:
-	return rc;
+	return 0;
 
-err_out_cnt_6:
-	dma_free_coherent(&pdev->dev, sizeof(*tp->counters), tp->counters,
-			  tp->counters_phys_addr);
 err_out_msi_5:
 	netif_napi_del(&tp->napi);
-	rtl_disable_msi(pdev, tp);
-err_out_unmap_4:
-	iounmap(ioaddr);
-err_out_free_res_3:
-	pci_release_regions(pdev);
-err_out_mwi_2:
-	pci_clear_mwi(pdev);
-	pci_disable_device(pdev);
-err_out_free_dev_1:
-	free_netdev(dev);
-	goto out;
+
+	return rc;
 }
 
 static struct pci_driver rtl8169_pci_driver = {
-- 
2.15.1

[PATCH 3/3] r8169: remove netif_napi_del in probe error path

[Heiner Kallweit]

netif_napi_del is called implicitely by free_netdev, therefore we
don't have to do it explicitely.

When the probe error path is reached, the net_device isn't
registered yet. Therefore reordering the call to netif_napi_del
shouldn't cause any issues.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 3c7d90d3a..857f67beb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -8672,14 +8672,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->counters = dmam_alloc_coherent (&pdev->dev, sizeof(*tp->counters),
 					    &tp->counters_phys_addr,
 					    GFP_KERNEL);
-	if (!tp->counters) {
-		rc = -ENOMEM;
-		goto err_out_msi_5;
-	}
+	if (!tp->counters)
+		return -ENOMEM;
 
 	rc = register_netdev(dev);
 	if (rc < 0)
-		goto err_out_msi_5;
+		return rc;
 
 	pci_set_drvdata(pdev, dev);
 
@@ -8709,11 +8707,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	netif_carrier_off(dev);
 
 	return 0;
-
-err_out_msi_5:
-	netif_napi_del(&tp->napi);
-
-	return rc;
 }
 
 static struct pci_driver rtl8169_pci_driver = {
-- 
2.15.1