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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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/

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.

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