| Message ID | 20240821071842.8591-8-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
On Wed, Aug 21, 2024 at 10:19 AM Philipp Stanner <pstanner@redhat.com> wrote: > > stmicro uses PCI devres in the wrong way. Resources requested > through pcim_* functions don't need to be cleaned up manually in the > remove() callback or in the error unwind path of a probe() function. > Moreover, there is an unnecessary loop which only requests and ioremaps > BAR 0, but iterates over all BARs nevertheless. Seems like loongson was cargo-culted a lot without a clear understanding of this code in the main driver... > Furthermore, pcim_iomap_regions() and pcim_iomap_table() have been > deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate > pcim_iomap_table(), pcim_iomap_regions_request_all()"). > > Replace these functions with pcim_iomap_region(). > > Remove the unnecessary manual pcim_* cleanup calls. > > Remove the unnecessary loop over all BARs. ... > - for (i = 0; i < PCI_STD_NUM_BARS; i++) { > - if (pci_resource_len(pdev, i) == 0) > - continue; > - pcim_iounmap_regions(pdev, BIT(i)); Here is the BARx, which contradicts the probe :-) > - break; > - }
On Wed, 2024-08-21 at 11:14 +0300, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 10:19 AM Philipp Stanner > <pstanner@redhat.com> wrote: > > > > stmicro uses PCI devres in the wrong way. Resources requested > > through pcim_* functions don't need to be cleaned up manually in > > the > > remove() callback or in the error unwind path of a probe() > > function. > > > Moreover, there is an unnecessary loop which only requests and > > ioremaps > > BAR 0, but iterates over all BARs nevertheless. > > Seems like loongson was cargo-culted a lot without a clear > understanding of this code in the main driver... > > > Furthermore, pcim_iomap_regions() and pcim_iomap_table() have been > > deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: > > Deprecate > > pcim_iomap_table(), pcim_iomap_regions_request_all()"). > > > > Replace these functions with pcim_iomap_region(). > > > > Remove the unnecessary manual pcim_* cleanup calls. > > > > Remove the unnecessary loop over all BARs. > > ... > > > - for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > - if (pci_resource_len(pdev, i) == 0) > > - continue; > > - pcim_iounmap_regions(pdev, BIT(i)); > > Here is the BARx, which contradicts the probe :-) I'm not sure what should be done about it. The only interesting question is whether the other code with pcim_iomap_regions(... BIT(i) does also only grap BAR 0. In that case the driver wouldn't even be knowing what its own hardware is / does, though. P. > > > - break; > > - } >
On Wed, Aug 21, 2024 at 11:36:36AM +0200, Philipp Stanner wrote: > On Wed, 2024-08-21 at 11:14 +0300, Andy Shevchenko wrote: > > On Wed, Aug 21, 2024 at 10:19 AM Philipp Stanner > > <pstanner@redhat.com> wrote: ... > > > - for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > > - if (pci_resource_len(pdev, i) == 0) > > > - continue; > > > - pcim_iounmap_regions(pdev, BIT(i)); > > > > Here is the BARx, which contradicts the probe :-) > > I'm not sure what should be done about it. The only interesting > question is whether the other code with pcim_iomap_regions(... BIT(i) > does also only grap BAR 0. > In that case the driver wouldn't even be knowing what its own hardware > is / does, though. I think your patch does the right thing already. > > > - break; > > > - }
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 9e40c28d453a..5d42a9fad672 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -50,7 +50,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id struct plat_stmmacenet_data *plat; struct stmmac_resources res; struct device_node *np; - int ret, i, phy_mode; + int ret, phy_mode; np = dev_of_node(&pdev->dev); @@ -88,14 +88,11 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id goto err_put_node; } - /* Get the base address of device */ - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i) == 0) - continue; - ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev)); - if (ret) - goto err_disable_device; - break; + memset(&res, 0, sizeof(res)); + res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev)); + if (IS_ERR(res.addr)) { + ret = PTR_ERR(res.addr); + goto err_disable_device; } plat->bus_id = of_alias_get_id(np, "ethernet"); @@ -116,8 +113,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id loongson_default_data(plat); pci_enable_msi(pdev); - memset(&res, 0, sizeof(res)); - res.addr = pcim_iomap_table(pdev)[0]; res.irq = of_irq_get_byname(np, "macirq"); if (res.irq < 0) { @@ -158,18 +153,10 @@ static void loongson_dwmac_remove(struct pci_dev *pdev) { struct net_device *ndev = dev_get_drvdata(&pdev->dev); struct stmmac_priv *priv = netdev_priv(ndev); - int i; of_node_put(priv->plat->mdio_node); stmmac_dvr_remove(&pdev->dev); - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i) == 0) - continue; - pcim_iounmap_regions(pdev, BIT(i)); - break; - } - pci_disable_msi(pdev); pci_disable_device(pdev); } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c index 352b01678c22..f89a8a54c4e8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c @@ -188,11 +188,11 @@ static int stmmac_pci_probe(struct pci_dev *pdev, return ret; } - /* Get the base address of device */ + /* Request the base address BAR of device */ for (i = 0; i < PCI_STD_NUM_BARS; i++) { if (pci_resource_len(pdev, i) == 0) continue; - ret = pcim_iomap_regions(pdev, BIT(i), pci_name(pdev)); + ret = pcim_request_region(pdev, i, pci_name(pdev)); if (ret) return ret; break; @@ -205,7 +205,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev, return ret; memset(&res, 0, sizeof(res)); - res.addr = pcim_iomap_table(pdev)[i]; + /* Get the base address of device */ + res.addr = pcim_iomap(pdev, i, 0); + if (!res.addr) + return -ENOMEM; res.wol_irq = pdev->irq; res.irq = pdev->irq; @@ -231,16 +234,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev, */ static void stmmac_pci_remove(struct pci_dev *pdev) { - int i; - stmmac_dvr_remove(&pdev->dev); - - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i) == 0) - continue; - pcim_iounmap_regions(pdev, BIT(i)); - break; - } } static int __maybe_unused stmmac_pci_suspend(struct device *dev)
stmicro uses PCI devres in the wrong way. Resources requested through pcim_* functions don't need to be cleaned up manually in the remove() callback or in the error unwind path of a probe() function. Moreover, there is an unnecessary loop which only requests and ioremaps BAR 0, but iterates over all BARs nevertheless. Furthermore, pcim_iomap_regions() and pcim_iomap_table() have been deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()"). Replace these functions with pcim_iomap_region(). Remove the unnecessary manual pcim_* cleanup calls. Remove the unnecessary loop over all BARs. Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- .../ethernet/stmicro/stmmac/dwmac-loongson.c | 25 +++++-------------- .../net/ethernet/stmicro/stmmac/stmmac_pci.c | 18 +++++-------- 2 files changed, 12 insertions(+), 31 deletions(-)