.../ethernet/stmicro/stmmac/dwmac-loongson.c | 31 ++++++------------- .../net/ethernet/stmicro/stmmac/stmmac_pci.c | 24 ++++---------- 2 files changed, 15 insertions(+), 40 deletions(-)
From: Philipp Stanner <pstanner@redhat.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.
Signed-off-by: Philipp Stanner <pstanner@redhat.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"
+
/* 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;
@@ -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);
}
@@ -673,7 +660,7 @@ static const struct pci_device_id loongson_dwmac_id_table[] = {
MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);
static struct pci_driver loongson_dwmac_driver = {
- .name = "dwmac-loongson-pci",
+ .name = DRIVER_NAME,
.id_table = loongson_dwmac_id_table,
.probe = loongson_dwmac_probe,
.remove = loongson_dwmac_remove,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 352b01678c22..9d45af70d8a2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -155,7 +155,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
{
struct stmmac_pci_info *info = (struct stmmac_pci_info *)id->driver_data;
struct plat_stmmacenet_data *plat;
- struct stmmac_resources res;
+ struct stmmac_resources res = {};
int i;
int ret;
@@ -188,13 +188,13 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
return ret;
}
- /* Get the base address of device */
+ /* The first BAR > 0 is the base IO addr of our 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));
- if (ret)
- return ret;
+ res.addr = pcim_iomap_region(pdev, i, STMMAC_RESOURCE_NAME);
+ if (IS_ERR(res.addr))
+ return PTR_ERR(res.addr);
break;
}
@@ -204,8 +204,6 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
if (ret)
return ret;
- memset(&res, 0, sizeof(res));
- res.addr = pcim_iomap_table(pdev)[i];
res.wol_irq = pdev->irq;
res.irq = pdev->irq;
@@ -226,21 +224,11 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
* stmmac_pci_remove
*
* @pdev: platform device pointer
- * Description: this function calls the main to free the net resources
- * and releases the PCI resources.
+ * Description: Main driver resource release function
*/
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.47.1
在 2/18/25 9:21 PM, Philipp Stanner 写道:
> From: Philipp Stanner <pstanner@redhat.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@redhat.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
On 2/18/25 2:21 PM, Philipp Stanner wrote:
> @@ -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 = {};
I'm sorry for nit picking, but please respect the reverse christmas
tree above - what is relevant is the full line lenght.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> index 352b01678c22..9d45af70d8a2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> @@ -155,7 +155,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
> {
> struct stmmac_pci_info *info = (struct stmmac_pci_info *)id->driver_data;
> struct plat_stmmacenet_data *plat;
> - struct stmmac_resources res;
> + struct stmmac_resources res = {};
> int i;
> int ret;
Even more nit-picking: since you are touching this code chunck, could
you please fix the variables declaration order above?
Also, please add the target tree in the subj prefix, in this case
'net-next'.
Thanks!
Paolo
On Tue, Feb 18, 2025 at 02:21:21PM +0100, Philipp Stanner wrote: > From: Philipp Stanner <pstanner@redhat.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. Thanks for the extended commit message. It would be nice if the loongson Maintainers comment about the removal of the loop. I will leave it a couple of days before adding a Reviewed-by. Andrew
© 2016 - 2025 Red Hat, Inc.