Skip to contentSkip to conversation

sunhme: some cleanups

PATCH In review Rolf Eike Beer 4 participants 23 messages Feb 3, 2022 – Aug 30, 2022

Subsystems (3)
Networking Network drivers Sun happy meal Ethernet driver
lore
Share & tools
lore

Rolf Eike Beer

author
#0 lore raw

Hi,

it still matches my sense of humor to see fries offered in a network driver, so every now and then I point someone to the driver. This time I looked a bit more around and noticed some things I have already done wrong before myself. Time to clean them up so it's still funny while serving as a good example ;)

Things that are changed:

-drop unused tx_dump_ring()
-fix version number inconsistency
-forward error code from pci_enable_device()

Things that I may also do in the, but can't test because I don't have any hardware:

-switch to devres, which fixes a memleak of the dma_alloc_coherent() memory if register_netdev() fails [mostly done]
-use netdev_* print macros to print the device name in a consistent way

I can also send an immediate fix for the first one if you prefer.

Greetings,

Eike

author
#1 lore raw

PATCH 1/3 sunhme: remove unused tx_dump_ring()

I can't find a reference to it in the entire git history.

Signed-off-by Rolf Eike Beer <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 16 ----------------
 1 file changed, 16 deletions(-)
drivers/net/ethernet/sun/sunhme.c +0 -16
index ad9029ae6848..22abfe58f728 100644
@@ -135,25 +135,9 @@ static __inline__ void tx_dump_log(void)
 		this = (this + 1) & (TX_LOG_LEN - 1);
 	}
 }
-static __inline__ void tx_dump_ring(struct happy_meal *hp)
-{
-	struct hmeal_init_block *hb = hp->happy_block;
-	struct happy_meal_txd *tp = &hb->happy_meal_txd[0];
-	int i;
-
-	for (i = 0; i < TX_RING_SIZE; i+=4) {
-		printk("TXD[%d..%d]: [%08x:%08x] [%08x:%08x] [%08x:%08x] [%08x:%08x]\n",
-		       i, i + 4,
-		       le32_to_cpu(tp[i].tx_flags), le32_to_cpu(tp[i].tx_addr),
-		       le32_to_cpu(tp[i + 1].tx_flags), le32_to_cpu(tp[i + 1].tx_addr),
-		       le32_to_cpu(tp[i + 2].tx_flags), le32_to_cpu(tp[i + 2].tx_addr),
-		       le32_to_cpu(tp[i + 3].tx_flags), le32_to_cpu(tp[i + 3].tx_addr));
-	}
-}
 #else
 #define tx_add_log(hp, a, s)		do { } while(0)
 #define tx_dump_log()			do { } while(0)
-#define tx_dump_ring(hp)		do { } while(0)
 #endif
 
 #ifdef HMEDEBUG
Signature · 1 line
2.31.1
author
#3 lore raw

PATCH 2/3 sunhme: fix the version number in struct ethtool_drvinfo

In reply to Rolf Eike Beer
Fixes 050bbb196392b9c178f82b1205a23dd2f915ee93
Signed-off-by Rolf Eike Beer <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/sun/sunhme.c +2 -2
index 22abfe58f728..43834339bb43 100644
@@ -2470,8 +2470,8 @@ static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info
 {
 	struct happy_meal *hp = netdev_priv(dev);
 
-	strlcpy(info->driver, "sunhme", sizeof(info->driver));
-	strlcpy(info->version, "2.02", sizeof(info->version));
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));
 	if (hp->happy_flags & HFLAG_PCI) {
 		struct pci_dev *pdev = hp->happy_dev;
 		strlcpy(info->bus_info, pci_name(pdev), sizeof(info->bus_info));
Signature · 1 line
2.31.1
#4 lore raw

Re: [PATCH 2/3] sunhme: fix the version number in struct ethtool_drvinfo

Quoted 11 lines · Rolf Eike Beer
Fixes 050bbb196392b9c178f82b1205a23dd2f915ee93
Signed-off-by Rolf Eike Beer <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/sun/sunhme.c +0 -0
index 22abfe58f728..43834339bb43 100644

drivers/net/ethernet/sun/sunhme.c static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info · line 2474

Quoted 7 lines
drivers/net/ethernet/sun/sunhme.c +2 -2
@@ -2470,8 +2470,8 @@ static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info
 {
 	struct happy_meal *hp = netdev_priv(dev);
 
-	strlcpy(info->driver, "sunhme", sizeof(info->driver));
-	strlcpy(info->version, "2.02", sizeof(info->version));
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));

I would suggest you drop setting info->version. The kernel will fill it with the current kernel version, which is much more meaningful.

Andrew

author
#5 lore raw

Re: [PATCH 2/3] sunhme: fix the version number in struct ethtool_drvinfo

Am Donnerstag, 3. Februar 2022, 18:12:40 CET schrieb Andrew Lunn:

Quoted 19 lines · Rolf Eike Beer
Quoted 18 lines
Fixes 050bbb196392b9c178f82b1205a23dd2f915ee93
Signed-off-by Rolf Eike Beer <[email protected]>

---

 drivers/net/ethernet/sun/sunhme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ethernet/sun/sunhme.c index 22abfe58f728..43834339bb43 100644

drivers/net/ethernet/sun/sunhme.c +0 -0
@@ -2470,8 +2470,8 @@ static void hme_get_drvinfo(struct net_device *dev,

struct ethtool_drvinfo *info>
{

struct happy_meal *hp = netdev_priv(dev);

Quoted 7 lines

drivers/net/ethernet/sun/sunhme.c static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info · line 2474

Quoted 4 lines
drivers/net/ethernet/sun/sunhme.c +2 -2
-	strlcpy(info->driver, "sunhme", sizeof(info->driver));
-	strlcpy(info->version, "2.02", sizeof(info->version));
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));

I would suggest you drop setting info->version. The kernel will fill it with the current kernel version, which is much more meaningful.

Would it make sense to completely remove the version number from the driver then?

Eike

#6 lore raw

Re: [PATCH 2/3] sunhme: fix the version number in struct ethtool_drvinfo

Quoted 13 lines
Quoted 5 lines
Quoted 5 lines

struct ethtool_drvinfo *info>
{

struct happy_meal *hp = netdev_priv(dev);

Quoted 7 lines

drivers/net/ethernet/sun/sunhme.c static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info · line 2474

Quoted 4 lines
drivers/net/ethernet/sun/sunhme.c +2 -2
-	strlcpy(info->driver, "sunhme", sizeof(info->driver));
-	strlcpy(info->version, "2.02", sizeof(info->version));
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));

I would suggest you drop setting info->version. The kernel will fill it with the current kernel version, which is much more meaningful.

Quoted 2 lines

Would it make sense to completely remove the version number from the driver then?

If it is not used anywhere else, yes, you can remove it.

Andrew

author
#7 lore raw

Re: [PATCH 2/3] sunhme: fix the version number in struct ethtool_drvinfo

Am Samstag, 5. Februar 2022, 15:48:41 CET schrieb Andrew Lunn:

Quoted 14 lines
Quoted 14 lines
Quoted 13 lines

drivers/net/ethernet/sun/sunhme.c static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info · line 2474

Quoted 10 lines

struct ethtool_drvinfo *info>

{

drivers/net/ethernet/sun/sunhme.c +2 -2
 	struct happy_meal *hp = netdev_priv(dev);
 
-	strlcpy(info->driver, "sunhme", sizeof(info->driver));
-	strlcpy(info->version, "2.02", sizeof(info->version));
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));

I would suggest you drop setting info->version. The kernel will fill it with the current kernel version, which is much more meaningful.

Quoted 5 lines
Quoted 3 lines

Would it make sense to completely remove the version number from the driver
then?

If it is not used anywhere else, yes, you can remove it.

Of course it prints the number on module load… but otherwise it does nothing with it.

#8 lore raw

Re: [PATCH 2/3] sunhme: fix the version number in struct ethtool_drvinfo

Quoted 4 lines
Quoted 1 line

If it is not used anywhere else, yes, you can remove it.

Of course it prints the number on module load… but otherwise it does nothing with it.

You could remove that as well.

The basic problem is, the version string does not identify the sources with enough accuracy. It says nothing about back ported fixes in stable kernels. It tells you nothing about vendor patches to the network core, etc.

Andrew

maintainer
#9 lore raw

Re: [PATCH 2/3] sunhme: fix the version number in struct ethtool_drvinfo

In reply to Rolf Eike Beer
Quoted 1 line · On Thu, 03 Feb 2022 17:22:23 +0100 Rolf Eike Beer wrote:
Fixes 050bbb196392b9c178f82b1205a23dd2f915ee93

This is not the correct format for a fixes tag, FWIW.

author
#10 lore raw

PATCH v2 2/3 sunhme: fix the version number in struct ethtool_drvinfo

In reply to Rolf Eike Beer

The driver prints a version number on startup, use the same one here.

Fixes 050bbb196392 ("[NET] sunhme: Convert to new SBUS driver framework.")
Signed-off-by Rolf Eike Beer <[email protected]>

---
v2: correct "Fixes" line syntax

 drivers/net/ethernet/sun/sunhme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/sun/sunhme.c +2 -2
index 22abfe58f728..43834339bb43 100644
@@ -2470,8 +2470,8 @@ static void hme_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info
 {
 	struct happy_meal *hp = netdev_priv(dev);
 
-	strlcpy(info->driver, "sunhme", sizeof(info->driver));
-	strlcpy(info->version, "2.02", sizeof(info->version));
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));
 	if (hp->happy_flags & HFLAG_PCI) {
 		struct pci_dev *pdev = hp->happy_dev;
 		strlcpy(info->bus_info, pci_name(pdev), sizeof(info->bus_info));
Signature · 1 line
2.34.1
author
#11 lore raw

PATCH 3/3 sunhme: forward the error code from pci_enable_device()

In reply to Rolf Eike Beer

This already returns a proper error value, so pass it to the caller.

Signed-off-by Rolf Eike Beer <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/ethernet/sun/sunhme.c +3 -2
index 43834339bb43..980a936ce8d1 100644
@@ -2969,11 +2969,12 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		strcpy(prom_name, "SUNW,hme");
 #endif
 
-	err = -ENODEV;
+	err = pci_enable_device(pdev);
 
-	if (pci_enable_device(pdev))
+	if (err)
 		goto err_out;
 	pci_set_master(pdev);
+	err = -ENODEV;
 
 	if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) {
 		qp = quattro_pci_find(pdev);
Signature · 1 line
2.31.1
maintainer Reviewed-by on v1 3/3
#12 lore raw

Re: [PATCH 3/3] sunhme: forward the error code from pci_enable_device()

Quoted 16 lines · Rolf Eike Beer

This already returns a proper error value, so pass it to the caller.

Signed-off-by Rolf Eike Beer <[email protected]>
---
  drivers/net/ethernet/sun/sunhme.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/ethernet/sun/sunhme.c +0 -1
index 43834339bb43..980a936ce8d1 100644
@@ -2969,11 +2969,12 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  		strcpy(prom_name, "SUNW,hme");
  #endif
 
-	err = -ENODEV;

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev *pdev, · line 2980

Quoted 10 lines
drivers/net/ethernet/sun/sunhme.c +3 -1
+	err = pci_enable_device(pdev);
 
-	if (pci_enable_device(pdev))
+	if (err)
  		goto err_out;
  	pci_set_master(pdev);
+	err = -ENODEV;
  	if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) {
  		qp = quattro_pci_find(pdev);

This is incrementally better, but there are several more calls like this. A follow up converting those as well would be good.

Reviewed-by Sean Anderson <[email protected]>
author
#13 lore raw

PATCH 1/1 sunhme: switch to devres

In reply to Rolf Eike Beer

This not only removes a lot of code, it also fixes the memleak of the DMA memory when register_netdev() fails.

Signed-off-by Rolf Eike Beer <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 55 +++++++++----------------------
 1 file changed, 16 insertions(+), 39 deletions(-)
drivers/net/ethernet/sun/sunhme.c +16 -39
index 980a936ce8d1..ec78f43f75c9 100644
@@ -2952,7 +2952,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	struct happy_meal *hp;
 	struct net_device *dev;
 	void __iomem *hpreg_base;
-	unsigned long hpreg_res;
 	int i, qfe_slot = -1;
 	char prom_name[64];
 	u8 addr[ETH_ALEN];

@@ -2969,7 +2968,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		strcpy(prom_name, "SUNW,hme");
 #endif
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 
 	if (err)
 		goto err_out;

@@ -2987,10 +2986,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 			goto err_out;
 	}
 
-	dev = alloc_etherdev(sizeof(struct happy_meal));
-	err = -ENOMEM;
-	if (!dev)
+	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
+	if (!dev) {
+		err = -ENOMEM;
 		goto err_out;
+	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
 	if (hme_version_printed++ == 0)

@@ -3009,21 +3009,23 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		qp->happy_meals[qfe_slot] = dev;
 	}
 
-	hpreg_res = pci_resource_start(pdev, 0);
-	err = -ENODEV;
 	if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
 		printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
 		goto err_out_clear_quattro;
 	}
-	if (pci_request_regions(pdev, DRV_NAME)) {
+
+	if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+				  pci_resource_len(pdev, 0),
+				  DRV_NAME)) {
 		printk(KERN_ERR "happymeal(PCI): Cannot obtain PCI resources, "
 		       "aborting.\n");
 		goto err_out_clear_quattro;
 	}
 
-	if ((hpreg_base = ioremap(hpreg_res, 0x8000)) == NULL) {
+	hpreg_base = pcim_iomap(pdev, 0, 0x8000);
+	if (!hpreg_base) {
 		printk(KERN_ERR "happymeal(PCI): Unable to remap card memory.\n");
-		goto err_out_free_res;
+		goto err_out_clear_quattro;
 	}
 
 	for (i = 0; i < 6; i++) {

@@ -3089,11 +3091,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	hp->happy_bursts = DMA_BURSTBITS;
 #endif
 
-	hp->happy_block = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+	hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
 					     &hp->hblock_dvma, GFP_KERNEL);
-	err = -ENODEV;
 	if (!hp->happy_block)
-		goto err_out_iounmap;
+		goto err_out_clear_quattro;
 
 	hp->linkcheck = 0;
 	hp->timer_state = asleep;

@@ -3127,11 +3128,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	happy_meal_set_initial_advertisement(hp);
 	spin_unlock_irq(&hp->happy_lock);
 
-	err = register_netdev(hp->dev);
+	err = devm_register_netdev(&pdev->dev, dev);
 	if (err) {
 		printk(KERN_ERR "happymeal(PCI): Cannot register net device, "
 		       "aborting.\n");
-		goto err_out_iounmap;
+		goto err_out_clear_quattro;
 	}
 
 	pci_set_drvdata(pdev, hp);

@@ -3164,37 +3165,14 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 
 	return 0;
 
-err_out_iounmap:
-	iounmap(hp->gregs);
-
-err_out_free_res:
-	pci_release_regions(pdev);
-
 err_out_clear_quattro:
 	if (qp != NULL)
 		qp->happy_meals[qfe_slot] = NULL;
 
-	free_netdev(dev);
-
 err_out:
 	return err;
 }
 
-static void happy_meal_pci_remove(struct pci_dev *pdev)
-{
-	struct happy_meal *hp = pci_get_drvdata(pdev);
-	struct net_device *net_dev = hp->dev;
-
-	unregister_netdev(net_dev);
-
-	dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-			  hp->happy_block, hp->hblock_dvma);
-	iounmap(hp->gregs);
-	pci_release_regions(hp->happy_dev);
-
-	free_netdev(net_dev);
-}
-
 static const struct pci_device_id happymeal_pci_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_HAPPYMEAL) },
 	{ }			/* Terminating entry */

@@ -3206,7 +3184,6 @@ static struct pci_driver hme_pci_driver = {
 	.name		= "hme",
 	.id_table	= happymeal_pci_ids,
 	.probe		= happy_meal_pci_probe,
-	.remove		= happy_meal_pci_remove,
 };
 
 static int __init happy_meal_pci_init(void)
Signature · 1 line
2.34.1
maintainer Reviewed-by on v1 1/1
#14 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Quoted 141 lines · Rolf Eike Beer

This not only removes a lot of code, it also fixes the memleak of the DMA memory when register_netdev() fails.

Signed-off-by Rolf Eike Beer <[email protected]>
---
  drivers/net/ethernet/sun/sunhme.c | 55 +++++++++----------------------
  1 file changed, 16 insertions(+), 39 deletions(-)
drivers/net/ethernet/sun/sunhme.c +16 -38
index 980a936ce8d1..ec78f43f75c9 100644
@@ -2952,7 +2952,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  	struct happy_meal *hp;
  	struct net_device *dev;
  	void __iomem *hpreg_base;
-	unsigned long hpreg_res;
  	int i, qfe_slot = -1;
  	char prom_name[64];
  	u8 addr[ETH_ALEN];

@@ -2969,7 +2968,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  		strcpy(prom_name, "SUNW,hme");
  #endif
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 
  	if (err)
  		goto err_out;

@@ -2987,10 +2986,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  			goto err_out;
  	}
 
-	dev = alloc_etherdev(sizeof(struct happy_meal));
-	err = -ENOMEM;
-	if (!dev)
+	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
+	if (!dev) {
+		err = -ENOMEM;
  		goto err_out;
+	}
  	SET_NETDEV_DEV(dev, &pdev->dev);
 
  	if (hme_version_printed++ == 0)

@@ -3009,21 +3009,23 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  		qp->happy_meals[qfe_slot] = dev;
  	}
 
-	hpreg_res = pci_resource_start(pdev, 0);
-	err = -ENODEV;
  	if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
  		printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
  		goto err_out_clear_quattro;
  	}
-	if (pci_request_regions(pdev, DRV_NAME)) {
+
+	if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+				  pci_resource_len(pdev, 0),
+				  DRV_NAME)) {
  		printk(KERN_ERR "happymeal(PCI): Cannot obtain PCI resources, "
  		       "aborting.\n");
  		goto err_out_clear_quattro;
  	}
 
-	if ((hpreg_base = ioremap(hpreg_res, 0x8000)) == NULL) {
+	hpreg_base = pcim_iomap(pdev, 0, 0x8000);
+	if (!hpreg_base) {
  		printk(KERN_ERR "happymeal(PCI): Unable to remap card memory.\n");
-		goto err_out_free_res;
+		goto err_out_clear_quattro;
  	}
 
  	for (i = 0; i < 6; i++) {

@@ -3089,11 +3091,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  	hp->happy_bursts = DMA_BURSTBITS;
  #endif
 
-	hp->happy_block = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+	hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
  					     &hp->hblock_dvma, GFP_KERNEL);
-	err = -ENODEV;
  	if (!hp->happy_block)
-		goto err_out_iounmap;
+		goto err_out_clear_quattro;
 
  	hp->linkcheck = 0;
  	hp->timer_state = asleep;

@@ -3127,11 +3128,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
  	happy_meal_set_initial_advertisement(hp);
  	spin_unlock_irq(&hp->happy_lock);
 
-	err = register_netdev(hp->dev);
+	err = devm_register_netdev(&pdev->dev, dev);
  	if (err) {
  		printk(KERN_ERR "happymeal(PCI): Cannot register net device, "
  		       "aborting.\n");
-		goto err_out_iounmap;
+		goto err_out_clear_quattro;
  	}
 
  	pci_set_drvdata(pdev, hp);

@@ -3164,37 +3165,14 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 
  	return 0;
 
-err_out_iounmap:
-	iounmap(hp->gregs);
-
-err_out_free_res:
-	pci_release_regions(pdev);
-
  err_out_clear_quattro:
  	if (qp != NULL)
  		qp->happy_meals[qfe_slot] = NULL;
 
-	free_netdev(dev);
-
  err_out:
  	return err;
  }
 
-static void happy_meal_pci_remove(struct pci_dev *pdev)
-{
-	struct happy_meal *hp = pci_get_drvdata(pdev);
-	struct net_device *net_dev = hp->dev;
-
-	unregister_netdev(net_dev);
-
-	dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-			  hp->happy_block, hp->hblock_dvma);
-	iounmap(hp->gregs);
-	pci_release_regions(hp->happy_dev);
-
-	free_netdev(net_dev);
-}
-
  static const struct pci_device_id happymeal_pci_ids[] = {
  	{ PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_HAPPYMEAL) },
  	{ }			/* Terminating entry */

drivers/net/ethernet/sun/sunhme.c static struct pci_driver hme_pci_driver = { · line 3189

Quoted 7 lines
drivers/net/ethernet/sun/sunhme.c +0 -1
@@ -3206,7 +3184,6 @@ static struct pci_driver hme_pci_driver = {
  	.name		= "hme",
  	.id_table	= happymeal_pci_ids,
  	.probe		= happy_meal_pci_probe,
-	.remove		= happy_meal_pci_remove,
  };
 
  static int __init happy_meal_pci_init(void)

This looks good, but doesn't apply cleanly. I rebased it as follows:

From 5acfa13935277e312361c5630b49aea02399b8b8 Mon Sep 17 00:00:00 2001
From: Rolf Eike Beer <[email protected]>
Date: Mon, 14 Feb 2022 19:31:09 +0100
Subject: [PATCH] sunhme: switch to devres

This not only removes a lot of code, it also fixes the memleak of the DMA memory when register_netdev() fails.

Signed-off-by Rolf Eike Beer <[email protected]>
[ rebased onto net-next/master ]
Signed-off-by Sean Anderson <[email protected]>
Reviewed-by Sean Anderson <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 59 +++++++++----------------------
 1 file changed, 16 insertions(+), 43 deletions(-)
drivers/net/ethernet/sun/sunhme.c +16 -43
index eebe8c5f480c..e83774ffaa7a 100644
@@ -2933,7 +2933,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	struct happy_meal *hp;
 	struct net_device *dev;
 	void __iomem *hpreg_base;
-	unsigned long hpreg_res;
 	int i, qfe_slot = -1;
 	char prom_name[64];
 	u8 addr[ETH_ALEN];

@@ -2950,7 +2949,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		strcpy(prom_name, "SUNW,hme");
 #endif
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 
 	if (err)
 		goto err_out;

@@ -2968,10 +2967,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 			goto err_out;
 	}
 
-	dev = alloc_etherdev(sizeof(struct happy_meal));
-	err = -ENOMEM;
-	if (!dev)
+	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
+	if (!dev) {
+		err = -ENOMEM;
 		goto err_out;
+	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
 	if (hme_version_printed++ == 0)

@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		qp->happy_meals[qfe_slot] = dev;
 	}
 
-	hpreg_res = pci_resource_start(pdev, 0);
-	err = -ENODEV;
 	if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
 		printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
 		goto err_out_clear_quattro;
 	}
-	if (pci_request_regions(pdev, DRV_NAME)) {
+
+	if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+				  pci_resource_len(pdev, 0),
+				  DRV_NAME)) {
 		printk(KERN_ERR "happymeal(PCI): Cannot obtain PCI resources, "
 		       "aborting.\n");
 		goto err_out_clear_quattro;
 	}
 
-	if ((hpreg_base = ioremap(hpreg_res, 0x8000)) == NULL) {
+	hpreg_base = pcim_iomap(pdev, 0, 0x8000);
+	if (!hpreg_base) {
 		printk(KERN_ERR "happymeal(PCI): Unable to remap card memory.\n");
-		goto err_out_free_res;
+		goto err_out_clear_quattro;
 	}
 
 	for (i = 0; i < 6; i++) {

@@ -3070,11 +3072,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	hp->happy_bursts = DMA_BURSTBITS;
 #endif
 
-	hp->happy_block = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+	hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
 					     &hp->hblock_dvma, GFP_KERNEL);
-	err = -ENODEV;
 	if (!hp->happy_block)
-		goto err_out_iounmap;
+		goto err_out_clear_quattro;
 
 	hp->linkcheck = 0;
 	hp->timer_state = asleep;

@@ -3108,11 +3109,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	happy_meal_set_initial_advertisement(hp);
 	spin_unlock_irq(&hp->happy_lock);
 
-	err = register_netdev(hp->dev);
+	err = devm_register_netdev(&pdev->dev, dev);
 	if (err) {
 		printk(KERN_ERR "happymeal(PCI): Cannot register net device, "
 		       "aborting.\n");
-		goto err_out_free_coherent;
+		goto err_out_clear_quattro;
 	}
 
 	pci_set_drvdata(pdev, hp);

@@ -3145,41 +3146,14 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 
 	return 0;
 
-err_out_free_coherent:
-	dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-			  hp->happy_block, hp->hblock_dvma);
-
-err_out_iounmap:
-	iounmap(hp->gregs);
-
-err_out_free_res:
-	pci_release_regions(pdev);
-
 err_out_clear_quattro:
 	if (qp != NULL)
 		qp->happy_meals[qfe_slot] = NULL;
 
-	free_netdev(dev);
-
 err_out:
 	return err;
 }
 
-static void happy_meal_pci_remove(struct pci_dev *pdev)
-{
-	struct happy_meal *hp = pci_get_drvdata(pdev);
-	struct net_device *net_dev = hp->dev;
-
-	unregister_netdev(net_dev);
-
-	dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-			  hp->happy_block, hp->hblock_dvma);
-	iounmap(hp->gregs);
-	pci_release_regions(hp->happy_dev);
-
-	free_netdev(net_dev);
-}
-
 static const struct pci_device_id happymeal_pci_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_HAPPYMEAL) },
 	{ }			/* Terminating entry */

@@ -3191,7 +3165,6 @@ static struct pci_driver hme_pci_driver = {
 	.name		= "hme",
 	.id_table	= happymeal_pci_ids,
 	.probe		= happy_meal_pci_probe,
-	.remove		= happy_meal_pci_remove,
 };
 
 static int __init happy_meal_pci_init(void)
Signature · 1 line
2.35.1
maintainer
#15 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Quoted 193 lines · Sean Anderson
Quoted 125 lines · Rolf Eike Beer

This not only removes a lot of code, it also fixes the memleak of the DMA memory when register_netdev() fails.

Signed-off-by Rolf Eike Beer <[email protected]>

---
  drivers/net/ethernet/sun/sunhme.c | 55 +++++++++----------------------

  1 file changed, 16 insertions(+), 39 deletions(-)
drivers/net/ethernet/sun/sunhme.c +9 -8
index 980a936ce8d1..ec78f43f75c9 100644
@@ -2952,7 +2952,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
       struct happy_meal *hp;
       struct net_device *dev;
       void __iomem *hpreg_base;
-    unsigned long hpreg_res;
       int i, qfe_slot = -1;
       char prom_name[64];
       u8 addr[ETH_ALEN];

@@ -2969,7 +2968,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
           strcpy(prom_name, "SUNW,hme");
   #endif
-    err = pci_enable_device(pdev);
+    err = pcim_enable_device(pdev);
       if (err)
           goto err_out;

@@ -2987,10 +2986,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
               goto err_out;
       }
-    dev = alloc_etherdev(sizeof(struct happy_meal));
-    err = -ENOMEM;
-    if (!dev)
+    dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
+    if (!dev) {
+        err = -ENOMEM;
           goto err_out;
+    }
       SET_NETDEV_DEV(dev, &pdev->dev);
       if (hme_version_printed++ == 0)

@@ -3009,21 +3009,23 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
           qp->happy_meals[qfe_slot] = dev;
       }
-    hpreg_res = pci_resource_start(pdev, 0);
-    err = -ENODEV;
       if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
           printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
           goto err_out_clear_quattro;
       }
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

          printk(KERN_ERR "happymeal(PCI): Cannot obtain PCI resources, " "aborting.\n");
          goto err_out_clear_quattro;
      }
-    if ((hpreg_base = ioremap(hpreg_res, 0x8000)) == NULL) {
+    hpreg_base = pcim_iomap(pdev, 0, 0x8000);
+    if (!hpreg_base) {
          printk(KERN_ERR "happymeal(PCI): Unable to remap card memory.\n"); -        goto err_out_free_res;
+        goto err_out_clear_quattro;
      }
      for (i = 0; i < 6; i++) {

drivers/net/ethernet/sun/sunhme.c +3 -4
@@ -3089,11 +3091,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
       hp->happy_bursts = DMA_BURSTBITS;
   #endif
-    hp->happy_block = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+    hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
                            &hp->hblock_dvma, GFP_KERNEL);
-    err = -ENODEV;
       if (!hp->happy_block)
-        goto err_out_iounmap;
+        goto err_out_clear_quattro;
       hp->linkcheck = 0;
       hp->timer_state = asleep;

@@ -3127,11 +3128,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
       happy_meal_set_initial_advertisement(hp);
       spin_unlock_irq(&hp->happy_lock);
-    err = register_netdev(hp->dev);
+    err = devm_register_netdev(&pdev->dev, dev);
       if (err) {

          printk(KERN_ERR "happymeal(PCI): Cannot register net device, " "aborting.\n");
-        goto err_out_iounmap;
+        goto err_out_clear_quattro;
      }
      pci_set_drvdata(pdev, hp);

drivers/net/ethernet/sun/sunhme.c +0 -6
@@ -3164,37 +3165,14 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
       return 0;
-err_out_iounmap:
-    iounmap(hp->gregs);
-
-err_out_free_res:
-    pci_release_regions(pdev);
-

  err_out_clear_quattro:
      if (qp != NULL)
          qp->happy_meals[qfe_slot] = NULL;
-    free_netdev(dev);
-
  err_out:
      return err;
  }

drivers/net/ethernet/sun/sunhme.c +0 -15
-static void happy_meal_pci_remove(struct pci_dev *pdev)
-{
-    struct happy_meal *hp = pci_get_drvdata(pdev);
-    struct net_device *net_dev = hp->dev;
-
-    unregister_netdev(net_dev);
-
-    dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-              hp->happy_block, hp->hblock_dvma);
-    iounmap(hp->gregs);
-    pci_release_regions(hp->happy_dev);
-
-    free_netdev(net_dev);
-}
-

  static const struct pci_device_id happymeal_pci_ids[] = { { PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_HAPPYMEAL) }, { }            /* Terminating entry */

drivers/net/ethernet/sun/sunhme.c static struct pci_driver hme_pci_driver = { · line 3189

Quoted 7 lines
drivers/net/ethernet/sun/sunhme.c +0 -1
@@ -3206,7 +3184,6 @@ static struct pci_driver hme_pci_driver = {
       .name        = "hme",
       .id_table    = happymeal_pci_ids,
       .probe        = happy_meal_pci_probe,
-    .remove        = happy_meal_pci_remove,
   };
   static int __init happy_meal_pci_init(void)

This looks good, but doesn't apply cleanly. I rebased it as follows:

From 5acfa13935277e312361c5630b49aea02399b8b8 Mon Sep 17 00:00:00 2001
From: Rolf Eike Beer <[email protected]>
Date: Mon, 14 Feb 2022 19:31:09 +0100
Subject: [PATCH] sunhme: switch to devres

This not only removes a lot of code, it also fixes the memleak of the DMA memory when register_netdev() fails.

Signed-off-by Rolf Eike Beer <[email protected]>
[ rebased onto net-next/master ]
Signed-off-by Sean Anderson <[email protected]>
Reviewed-by Sean Anderson <[email protected]>
---
  drivers/net/ethernet/sun/sunhme.c | 59 +++++++++----------------------
  1 file changed, 16 insertions(+), 43 deletions(-)
drivers/net/ethernet/sun/sunhme.c +5 -6
index eebe8c5f480c..e83774ffaa7a 100644
@@ -2933,7 +2933,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
      struct happy_meal *hp;
      struct net_device *dev;
      void __iomem *hpreg_base;
-    unsigned long hpreg_res;
      int i, qfe_slot = -1;
      char prom_name[64];
      u8 addr[ETH_ALEN];

@@ -2950,7 +2949,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
          strcpy(prom_name, "SUNW,hme");
  #endif
 
-    err = pci_enable_device(pdev);
+    err = pcim_enable_device(pdev);
 
      if (err)
          goto err_out;

@@ -2968,10 +2967,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
              goto err_out;
      }
 
-    dev = alloc_etherdev(sizeof(struct happy_meal));
-    err = -ENOMEM;
-    if (!dev)
+    dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
+    if (!dev) {
+        err = -ENOMEM;
          goto err_out;
+    }
      SET_NETDEV_DEV(dev, &pdev->dev);
 
      if (hme_version_printed++ == 0)

@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
          qp->happy_meals[qfe_slot] = dev;
      }
 
-    hpreg_res = pci_resource_start(pdev, 0);

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev *pdev, · line 3000

Quoted 10 lines
drivers/net/ethernet/sun/sunhme.c +4 -2
-    err = -ENODEV;
      if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
          printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
          goto err_out_clear_quattro;
      }
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

Actually, it looks like you are failing to set err from these *m calls, like what you fixed in patch 3. Can you address this for v2?

Quoted 92 lines
drivers/net/ethernet/sun/sunhme.c +7 -35
          printk(KERN_ERR "happymeal(PCI): Cannot obtain PCI resources, "
                 "aborting.\n");
          goto err_out_clear_quattro;
      }
 
-    if ((hpreg_base = ioremap(hpreg_res, 0x8000)) == NULL) {
+    hpreg_base = pcim_iomap(pdev, 0, 0x8000);
+    if (!hpreg_base) {
          printk(KERN_ERR "happymeal(PCI): Unable to remap card memory.\n");
-        goto err_out_free_res;
+        goto err_out_clear_quattro;
      }
 
      for (i = 0; i < 6; i++) {

@@ -3070,11 +3072,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
      hp->happy_bursts = DMA_BURSTBITS;
  #endif
 
-    hp->happy_block = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+    hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
                           &hp->hblock_dvma, GFP_KERNEL);
-    err = -ENODEV;
      if (!hp->happy_block)
-        goto err_out_iounmap;
+        goto err_out_clear_quattro;
 
      hp->linkcheck = 0;
      hp->timer_state = asleep;

@@ -3108,11 +3109,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
      happy_meal_set_initial_advertisement(hp);
      spin_unlock_irq(&hp->happy_lock);
 
-    err = register_netdev(hp->dev);
+    err = devm_register_netdev(&pdev->dev, dev);
      if (err) {
          printk(KERN_ERR "happymeal(PCI): Cannot register net device, "
                 "aborting.\n");
-        goto err_out_free_coherent;
+        goto err_out_clear_quattro;
      }
 
      pci_set_drvdata(pdev, hp);

@@ -3145,41 +3146,14 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 
      return 0;
 
-err_out_free_coherent:
-    dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-              hp->happy_block, hp->hblock_dvma);
-
-err_out_iounmap:
-    iounmap(hp->gregs);
-
-err_out_free_res:
-    pci_release_regions(pdev);
-
  err_out_clear_quattro:
      if (qp != NULL)
          qp->happy_meals[qfe_slot] = NULL;
 
-    free_netdev(dev);
-
  err_out:
      return err;
  }
 
-static void happy_meal_pci_remove(struct pci_dev *pdev)
-{
-    struct happy_meal *hp = pci_get_drvdata(pdev);
-    struct net_device *net_dev = hp->dev;
-
-    unregister_netdev(net_dev);
-
-    dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-              hp->happy_block, hp->hblock_dvma);
-    iounmap(hp->gregs);
-    pci_release_regions(hp->happy_dev);
-
-    free_netdev(net_dev);
-}
-
  static const struct pci_device_id happymeal_pci_ids[] = {
      { PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_HAPPYMEAL) },
      { }            /* Terminating entry */

@@ -3191,7 +3165,6 @@ static struct pci_driver hme_pci_driver = {
      .name        = "hme",
      .id_table    = happymeal_pci_ids,
      .probe        = happy_meal_pci_probe,
-    .remove        = happy_meal_pci_remove,
  };
 
  static int __init happy_meal_pci_init(void)
author
#16 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Am 2022-07-27 05:58, schrieb Sean Anderson:

Quoted 3 lines · Sean Anderson
Quoted 2 lines

This looks good, but doesn't apply cleanly. I rebased it as follows:

Looks like what my local rebase has also produced.

The sentence about the leak from the commitmessage can be dropped then, as this leak has already been fixed.

Quoted 9 lines
Quoted 9 lines
drivers/net/ethernet/sun/sunhme.c +0 -0
index eebe8c5f480c..e83774ffaa7a 100644
@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev 
 *pdev,
          qp->happy_meals[qfe_slot] = dev;
      }
Quoted 16 lines

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev *pdev, · line 3000

Quoted 12 lines
drivers/net/ethernet/sun/sunhme.c +4 -3
-    hpreg_res = pci_resource_start(pdev, 0);
-    err = -ENODEV;
      if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
          printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI 
 device base address.\n");
          goto err_out_clear_quattro;
      }
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

Actually, it looks like you are failing to set err from these *m calls, like what
you fixed in patch 3. Can you address this for v2?

It returns NULL on error, there is no error code I can set.

Regards,

Eike

maintainer
#17 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Quoted 34 lines · Rolf Eike Beer

Am 2022-07-27 05:58, schrieb Sean Anderson:

Quoted 3 lines · Sean Anderson
Quoted 2 lines

This looks good, but doesn't apply cleanly. I rebased it as follows:

Looks like what my local rebase has also produced.

The sentence about the leak from the commitmessage can be dropped then, as this leak has already been fixed.

Quoted 8 lines
Quoted 8 lines
drivers/net/ethernet/sun/sunhme.c +0 -0
index eebe8c5f480c..e83774ffaa7a 100644
@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
           qp->happy_meals[qfe_slot] = dev;
       }
Quoted 15 lines

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev *pdev, · line 3000

Quoted 11 lines
drivers/net/ethernet/sun/sunhme.c +4 -3
-    hpreg_res = pci_resource_start(pdev, 0);
-    err = -ENODEV;
       if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
           printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
           goto err_out_clear_quattro;
       }
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

Actually, it looks like you are failing to set err from these *m calls, like what
you fixed in patch 3. Can you address this for v2?

Quoted 1 line

It returns NULL on error, there is no error code I can set.

So it does. A quick grep shows that most drivers return -EBUSY.

--Sean

author
#18 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Am Freitag, 29. Juli 2022, 02:33:01 CEST schrieb Sean Anderson:

Quoted 34 lines · Rolf Eike Beer
Quoted 33 lines

Am 2022-07-27 05:58, schrieb Sean Anderson:

Quoted 2 lines · Sean Anderson
Quoted 1 line

This looks good, but doesn't apply cleanly. I rebased it as follows:

Looks like what my local rebase has also produced.

The sentence about the leak from the commitmessage can be dropped then, as this leak has already been fixed.

Quoted 24 lines
Quoted 10 lines
(quoted diff) +0 -0

b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a 100644

drivers/net/ethernet/sun/sunhme.c +0 -1
@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev
 *pdev, qp->happy_meals[qfe_slot] = dev;
      }
 
-    hpreg_res = pci_resource_start(pdev, 0);

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev · line 3000

Quoted 10 lines
drivers/net/ethernet/sun/sunhme.c +4 -2
-    err = -ENODEV;
      if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
          printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device
 base address.\n"); goto err_out_clear_quattro;
      }
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

Actually, it looks like you are failing to set err from these *m calls, like what
you fixed in patch 3. Can you address this for v2?

Quoted 3 lines
Quoted 1 line

It returns NULL on error, there is no error code I can set.

So it does. A quick grep shows that most drivers return -EBUSY.

Sure, I just meant that there is no error code I can pass on. I can change that to -EBUSY if you prefer that, currently it just returns -ENODEV as the old code has done before.

Eike

author
#19 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Am Montag, 1. August 2022, 17:14:39 CEST schrieb Rolf Eike Beer:

Quoted 42 lines

Am Freitag, 29. Juli 2022, 02:33:01 CEST schrieb Sean Anderson:

Quoted 41 lines · Rolf Eike Beer
Quoted 40 lines

Am 2022-07-27 05:58, schrieb Sean Anderson:

Quoted 2 lines · Sean Anderson
Quoted 1 line

This looks good, but doesn't apply cleanly. I rebased it as follows:

Looks like what my local rebase has also produced.

The sentence about the leak from the commitmessage can be dropped then, as this leak has already been fixed.

Quoted 31 lines
Quoted 17 lines
(quoted diff) +0 -0

b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a 100644

drivers/net/ethernet/sun/sunhme.c +0 -2
@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev
 *pdev, qp->happy_meals[qfe_slot] = dev;
 
      }
 
-    hpreg_res = pci_resource_start(pdev, 0);
-    err = -ENODEV;
 
      if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
 
          printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI
          device

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev · line 3007

Quoted 10 lines
drivers/net/ethernet/sun/sunhme.c +4 -1
 
 base address.\n"); goto err_out_clear_quattro;
 
      }
 
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

Actually, it looks like you are failing to set err from these *m calls, like what
you fixed in patch 3. Can you address this for v2?

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

It returns NULL on error, there is no error code I can set.

So it does. A quick grep shows that most drivers return -EBUSY.

Sure, I just meant that there is no error code I can pass on. I can change that to -EBUSY if you prefer that, currently it just returns -ENODEV as the old code has done before.

Ping?

Eike

maintainer
#20 lore raw

Re: [PATCH 4/x] sunhme: switch to devres

Quoted 45 lines · Rolf Eike Beer

Am Montag, 1. August 2022, 17:14:39 CEST schrieb Rolf Eike Beer:

Quoted 44 lines

Am Freitag, 29. Juli 2022, 02:33:01 CEST schrieb Sean Anderson:

Quoted 43 lines · Rolf Eike Beer
Quoted 41 lines

Am 2022-07-27 05:58, schrieb Sean Anderson:

Quoted 2 lines · Sean Anderson
Quoted 1 line

This looks good, but doesn't apply cleanly. I rebased it as follows:

Looks like what my local rebase has also produced.

The sentence about the leak from the commitmessage can be dropped then, as this leak has already been fixed.

Quoted 22 lines
Quoted 22 lines
(quoted diff) +0 -0

b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a 100644

drivers/net/ethernet/sun/sunhme.c +0 -2
@@ -2990,21 +2990,23 @@ static int happy_meal_pci_probe(struct pci_dev
 *pdev, qp->happy_meals[qfe_slot] = dev;
 
       }
 
-    hpreg_res = pci_resource_start(pdev, 0);
-    err = -ENODEV;
 
       if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
 
           printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI
           device
 
 base address.\n"); goto err_out_clear_quattro;
 
       }
Quoted 9 lines

drivers/net/ethernet/sun/sunhme.c static int happy_meal_pci_probe(struct pci_dev *pdev, · line 3000

Quoted 5 lines
drivers/net/ethernet/sun/sunhme.c +4 -1
-    if (pci_request_regions(pdev, DRV_NAME)) {
+
+    if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+                  pci_resource_len(pdev, 0),
+                  DRV_NAME)) {

Actually, it looks like you are failing to set err from these *m calls, like what
you fixed in patch 3. Can you address this for v2?

It returns NULL on error, there is no error code I can set.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

So it does. A quick grep shows that most drivers return -EBUSY.

Sure, I just meant that there is no error code I can pass on. I can change that to -EBUSY if you prefer that, currently it just returns -ENODEV as the old code has done before.

Ping?

I think -EBUSY is a good return here.

I have a WIP at [1] of some logging cleanups on top of your commits.

--Sean

[1] https://github.com/Forty-Bot/linux/commits/hme_base

author
#21 lore raw

PATCH v2 4/4 sunhme: switch to devres

In reply to Rolf Eike Beer
Signed-off-by Rolf Eike Beer <[email protected]>
---
 drivers/net/ethernet/sun/sunhme.c | 60 +++++++++----------------------
 1 file changed, 17 insertions(+), 43 deletions(-)

v2:
-return -EBUSY in case the PCI region can't be requested

drivers/net/ethernet/sun/sunhme.c +17 -43
index eebe8c5f480c..df6e630a5024 100644
@@ -2933,7 +2933,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	struct happy_meal *hp;
 	struct net_device *dev;
 	void __iomem *hpreg_base;
-	unsigned long hpreg_res;
 	int i, qfe_slot = -1;
 	char prom_name[64];
 	u8 addr[ETH_ALEN];

@@ -2950,7 +2949,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		strcpy(prom_name, "SUNW,hme");
 #endif
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 
 	if (err)
 		goto err_out;

@@ -2968,10 +2967,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 			goto err_out;
 	}
 
-	dev = alloc_etherdev(sizeof(struct happy_meal));
-	err = -ENOMEM;
-	if (!dev)
+	dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
+	if (!dev) {
+		err = -ENOMEM;
 		goto err_out;
+	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
 	if (hme_version_printed++ == 0)

@@ -2990,21 +2990,24 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 		qp->happy_meals[qfe_slot] = dev;
 	}
 
-	hpreg_res = pci_resource_start(pdev, 0);
-	err = -ENODEV;
 	if ((pci_resource_flags(pdev, 0) & IORESOURCE_IO) != 0) {
 		printk(KERN_ERR "happymeal(PCI): Cannot find proper PCI device base address.\n");
 		goto err_out_clear_quattro;
 	}
-	if (pci_request_regions(pdev, DRV_NAME)) {
+
+	if (!devm_request_region(&pdev->dev, pci_resource_start(pdev, 0),
+				  pci_resource_len(pdev, 0),
+				  DRV_NAME)) {
 		printk(KERN_ERR "happymeal(PCI): Cannot obtain PCI resources, "
 		       "aborting.\n");
+		err = -EBUSY;
 		goto err_out_clear_quattro;
 	}
 
-	if ((hpreg_base = ioremap(hpreg_res, 0x8000)) == NULL) {
+	hpreg_base = pcim_iomap(pdev, 0, 0x8000);
+	if (!hpreg_base) {
 		printk(KERN_ERR "happymeal(PCI): Unable to remap card memory.\n");
-		goto err_out_free_res;
+		goto err_out_clear_quattro;
 	}
 
 	for (i = 0; i < 6; i++) {

@@ -3070,11 +3073,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	hp->happy_bursts = DMA_BURSTBITS;
 #endif
 
-	hp->happy_block = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+	hp->happy_block = dmam_alloc_coherent(&pdev->dev, PAGE_SIZE,
 					     &hp->hblock_dvma, GFP_KERNEL);
-	err = -ENODEV;
 	if (!hp->happy_block)
-		goto err_out_iounmap;
+		goto err_out_clear_quattro;
 
 	hp->linkcheck = 0;
 	hp->timer_state = asleep;

@@ -3108,11 +3110,11 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 	happy_meal_set_initial_advertisement(hp);
 	spin_unlock_irq(&hp->happy_lock);
 
-	err = register_netdev(hp->dev);
+	err = devm_register_netdev(&pdev->dev, dev);
 	if (err) {
 		printk(KERN_ERR "happymeal(PCI): Cannot register net device, "
 		       "aborting.\n");
-		goto err_out_free_coherent;
+		goto err_out_clear_quattro;
 	}
 
 	pci_set_drvdata(pdev, hp);

@@ -3145,41 +3147,14 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
 
 	return 0;
 
-err_out_free_coherent:
-	dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-			  hp->happy_block, hp->hblock_dvma);
-
-err_out_iounmap:
-	iounmap(hp->gregs);
-
-err_out_free_res:
-	pci_release_regions(pdev);
-
 err_out_clear_quattro:
 	if (qp != NULL)
 		qp->happy_meals[qfe_slot] = NULL;
 
-	free_netdev(dev);
-
 err_out:
 	return err;
 }
 
-static void happy_meal_pci_remove(struct pci_dev *pdev)
-{
-	struct happy_meal *hp = pci_get_drvdata(pdev);
-	struct net_device *net_dev = hp->dev;
-
-	unregister_netdev(net_dev);
-
-	dma_free_coherent(hp->dma_dev, PAGE_SIZE,
-			  hp->happy_block, hp->hblock_dvma);
-	iounmap(hp->gregs);
-	pci_release_regions(hp->happy_dev);
-
-	free_netdev(net_dev);
-}
-
 static const struct pci_device_id happymeal_pci_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SUN, PCI_DEVICE_ID_SUN_HAPPYMEAL) },
 	{ }			/* Terminating entry */

@@ -3191,7 +3166,6 @@ static struct pci_driver hme_pci_driver = {
 	.name		= "hme",
 	.id_table	= happymeal_pci_ids,
 	.probe		= happy_meal_pci_probe,
-	.remove		= happy_meal_pci_remove,
 };
 
 static int __init happy_meal_pci_init(void)
Signature · 1 line
2.35.3
maintainer
#22 lore raw

Re: [PATCH 4/4 v2] sunhme: switch to devres

Quoted 1 line · On Mon, 29 Aug 2022 15:22:06 +0200 Rolf Eike Beer wrote:
Signed-off-by Rolf Eike Beer <[email protected]>

Please add a commit message with a justification, and realign continuation lines.

Are you using this HW?

Quoted 2 lines

v2:
-return -EBUSY in case the PCI region can't be requested

This looks like a fix which needs to be sent separately first and then you can do the re-factoring on top.