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
> >
> >
>
