linux-kernel - Re: [PATCH v2] stmmac: Replace deprecated PCI functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a4e790d1-b849-415f-b2f9-66cf162204e2@linux.dev>
Date: Fri, 21 Feb 2025 10:01:50 +0800
From: Yanteng Si <si.yanteng@...ux.dev>
To: Philipp Stanner <phasta@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Huacai Chen <chenhuacai@...nel.org>, Yinggang Gu <guyinggang@...ngson.cn>,
 Feiyang Chen <chenfeiyang@...ngson.cn>, Qunqin Zhao
 <zhaoqunqin@...ngson.cn>, Huacai Chen <chenhuacai@...ngson.cn>
Cc: netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Philipp Stanner <pstanner@...hat.com>
Subject: Re: [PATCH v2] stmmac: Replace deprecated PCI functions


在 2/18/25 9:21 PM, Philipp Stanner 写道:
> From: Philipp Stanner <pstanner@...hat.com>
>
> The PCI functions
>    - pcim_iomap_regions()
>    - pcim_iomap_table() and
>    - pcim_iounmap_regions()
> have been deprecated.
>
> The usage of pcim_* cleanup functions in the driver detach path (remove
> callback) is actually not necessary, since they perform that cleanup
> automatically.
>
> Furthermore, loongson_dwmac_probe() contains a surplus loop. That loop
> does not use index i in pcim_iomap_regions(), but costantly attempts to
> request and ioremap BAR 0. This would actually fail (since you cannot
> request the same BAR more than once), but presumably never fails because
> the preceding length check detects that all BARs except for 0 do not
> exist.

> Replace them with pcim_iomap_region(). Remove the surplus loop.

So, two things are done in one patch. How about splitting it into two 
patches?


>
> Signed-off-by: Philipp Stanner <pstanner@...hat.com>
> ---
> Changes in v2:
>    - Fix build errors because of missing ';'
>    - Address in the commit message why the patch removes a loop. (Andrew)
> ---
>   .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 31 ++++++-------------
>   .../net/ethernet/stmicro/stmmac/stmmac_pci.c  | 24 ++++----------
>   2 files changed, 15 insertions(+), 40 deletions(-)

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index bfe6e2d631bd..6f7c479c1a51 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -11,6 +11,8 @@
>   #include "dwmac_dma.h"
>   #include "dwmac1000.h"
>   
> +#define DRIVER_NAME "dwmac-loongson-pci"

This appears to have nothing to do with the commit message.

I believe it would be better if it were split off and made into

an independent patch.

> +
>   /* Normal Loongson Tx Summary */
>   #define DMA_INTR_ENA_NIE_TX_LOONGSON	0x00040000
>   /* Normal Loongson Rx Summary */
> @@ -520,9 +522,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>   {
>   	struct plat_stmmacenet_data *plat;
>   	struct stmmac_pci_info *info;
> -	struct stmmac_resources res;
> +	struct stmmac_resources res = {};
>   	struct loongson_data *ld;
> -	int ret, i;
> +	int ret;
>   
>   	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>   	if (!plat)
> @@ -552,17 +554,10 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>   	pci_set_master(pdev);
>   
>   	/* 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_table(pdev)[0];
> +	res.addr = pcim_iomap_region(pdev, 0, DRIVER_NAME);
> +	ret = PTR_ERR_OR_ZERO(res.addr);
> +	if (ret)
> +		goto err_disable_device;
>   
>   	plat->bsp_priv = ld;
>   	plat->setup = loongson_dwmac_setup;

According to the description in the commit message, the reason for

remove the surplus loop here is to fix the problem of requesting

the same BAR multiple times, that's good.

> @@ -606,7 +601,6 @@ 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);
>   	struct loongson_data *ld;
> -	int i;
>   
>   	ld = priv->plat->bsp_priv;
>   	stmmac_dvr_remove(&pdev->dev);
> @@ -617,13 +611,6 @@ static void loongson_dwmac_remove(struct pci_dev *pdev)
>   	if (ld->loongson_id == DWMAC_CORE_LS_MULTICHAN)
>   		loongson_dwmac_msi_clear(pdev);
>   
> -	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_device(pdev);
>   }

According to the commit message, the reason for removing the surplus

loop here is that there's no need to use the pcim_* cleanup functions in

the `remove()` function. This is different from the modifications in the

`probe()` function. I think it can be split out and made into a separate

patch.


How about splitting it like this?

Patch 1/3: Use the `DRIVER_NAME` macro.

Patch 2/3: remove the surplus loop to fix the problem of requesting the 
same BAR multiple times

Patch 3/3: remove the surplus pcim_* cleanup functions in the `remove()` 
function


Huacai, Qunqin, Would you mind helping to test it?


Thanks,

Yanteng