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(-)
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.0On 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;
> - }
--
With Best Regards,
Andy Shevchenko
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;
> > > - }
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.