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@xxxxxxxxxx> > > 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@xxxxxxxxxxx/ > 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 > > > > >