Re: [PATCH v3 6/9] ethernet: stmicro: Simplify PCI devres usage

[Philipp Stanner <pstanner@xxxxxxxxxx>]

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