| Message ID | 20240822134744.44919-7-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
On Thu, Aug 22, 2024 at 03:47:38PM +0200, Philipp Stanner 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. > > 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. ... > - /* Get the base address of device */ > + /* Request the base address BAR of device */ It's a tautology, "BAR" == Base Address ...
Hi Philipp On Thu, Aug 22, 2024 at 03:47:38PM +0200, Philipp Stanner 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. > > 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> Thanks for the series. But please note the network subsystem dev-process requires to submit the cleanup/feature changes on top of the net-next tree: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/ Just recently a Yanteng' (+cc) series https://lore.kernel.org/netdev/cover.1723014611.git.siyanteng@loongson.cn/ was merged in which significantly refactored the Loongson MAC driver. Seeing your patch isn't based on these changes, there is a high probability that the patch won't get cleanly applied onto the net-next tree. So please either rebase your patch onto the net-next tree, or at least merge in the Yanteng' series in your tree and rebase the patch onto it and let's hope there have been no other conflicting patches merged in into the net-next tree. -Serge(y) > --- > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 25 +++++-------------- > .../net/ethernet/stmicro/stmmac/stmmac_pci.c | 18 +++++-------- > 2 files changed, 12 insertions(+), 31 deletions(-) > > 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) > -- > 2.46.0 > >
On Fri, 2024-08-23 at 12:29 +0300, Serge Semin wrote: > Hi Philipp > > On Thu, Aug 22, 2024 at 03:47:38PM +0200, Philipp Stanner 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. > > > > 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> > > Thanks for the series. But please note the network subsystem > dev-process requires to submit the cleanup/feature changes on top of > the net-next tree: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/ That seems a policy I haven't seen so far; usually the assumption is that you branch out from Linus's master. Anyways, I of course am going to help with setting up something mergeable > > Just recently a Yanteng' (+cc) series > https://lore.kernel.org/netdev/cover.1723014611.git.siyanteng@loongson.cn/ > was merged in which significantly refactored the Loongson MAC driver. > Seeing your patch isn't based on these changes, there is a high > probability that the patch won't get cleanly applied onto the > net-next tree. So please either rebase your patch onto the net-next > tree, or at least merge in the Yanteng' series in your tree and > rebase the patch onto it and let's hope there have been no other > conflicting patches merged in into the net-next tree. I'll take a look into that, thx P. > > -Serge(y) > > > > --- > > .../ethernet/stmicro/stmmac/dwmac-loongson.c | 25 +++++---------- > > ---- > > .../net/ethernet/stmicro/stmmac/stmmac_pci.c | 18 +++++-------- > > 2 files changed, 12 insertions(+), 31 deletions(-) > > > > 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) > > -- > > 2.46.0 > > > > >
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(-)