| Message ID | 11922663.O9o76ZdvQC@eto.sf-tec.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [4/x] sunhme: switch to devres | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 2/14/22 1:31 PM, Rolf Eike Beer wrote: > 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 <eike-kernel@sf-tec.de> > --- > drivers/net/ethernet/sun/sunhme.c | 55 +++++++++---------------------- > 1 file changed, 16 insertions(+), 39 deletions(-) > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index 980a936ce8d1..ec78f43f75c9 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -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) > 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 <eike-kernel@sf-tec.de> 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 <eike-kernel@sf-tec.de> [ rebased onto net-next/master ] Signed-off-by: Sean Anderson <seanga2@gmail.com> Reviewed-by: Sean Anderson <seanga2@gmail.com> --- drivers/net/ethernet/sun/sunhme.c | 59 +++++++++---------------------- 1 file changed, 16 insertions(+), 43 deletions(-) diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -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)
On 7/26/22 11:49 PM, Sean Anderson wrote: > On 2/14/22 1:31 PM, Rolf Eike Beer wrote: >> 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 <eike-kernel@sf-tec.de> >> --- >> drivers/net/ethernet/sun/sunhme.c | 55 +++++++++---------------------- >> 1 file changed, 16 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c >> index 980a936ce8d1..ec78f43f75c9 100644 >> --- a/drivers/net/ethernet/sun/sunhme.c >> +++ b/drivers/net/ethernet/sun/sunhme.c >> @@ -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) >> > > 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 <eike-kernel@sf-tec.de> > 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 <eike-kernel@sf-tec.de> > [ rebased onto net-next/master ] > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Sean Anderson <seanga2@gmail.com> > --- > drivers/net/ethernet/sun/sunhme.c | 59 +++++++++---------------------- > 1 file changed, 16 insertions(+), 43 deletions(-) > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index eebe8c5f480c..e83774ffaa7a 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -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)) { 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? > 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)
Am 2022-07-27 05:58, schrieb Sean Anderson: > On 7/26/22 11:49 PM, Sean Anderson wrote: >> 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. >> diff --git a/drivers/net/ethernet/sun/sunhme.c >> b/drivers/net/ethernet/sun/sunhme.c >> index eebe8c5f480c..e83774ffaa7a 100644 >> --- a/drivers/net/ethernet/sun/sunhme.c >> +++ b/drivers/net/ethernet/sun/sunhme.c >> @@ -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)) { > > 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
On 7/28/22 3:52 PM, Rolf Eike Beer wrote: > Am 2022-07-27 05:58, schrieb Sean Anderson: >> On 7/26/22 11:49 PM, Sean Anderson wrote: > >>> 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. > >>> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c >>> index eebe8c5f480c..e83774ffaa7a 100644 >>> --- a/drivers/net/ethernet/sun/sunhme.c >>> +++ b/drivers/net/ethernet/sun/sunhme.c >>> @@ -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)) { >> >> 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. So it does. A quick grep shows that most drivers return -EBUSY. --Sean
Am Freitag, 29. Juli 2022, 02:33:01 CEST schrieb Sean Anderson: > On 7/28/22 3:52 PM, Rolf Eike Beer wrote: > > Am 2022-07-27 05:58, schrieb Sean Anderson: > >> On 7/26/22 11:49 PM, Sean Anderson wrote: > >>> 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. > > > >>> diff --git a/drivers/net/ethernet/sun/sunhme.c > >>> b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a > >>> 100644 > >>> --- a/drivers/net/ethernet/sun/sunhme.c > >>> +++ b/drivers/net/ethernet/sun/sunhme.c > >>> @@ -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)) { > >> > >> 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. > > 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
Am Montag, 1. August 2022, 17:14:39 CEST schrieb Rolf Eike Beer: > Am Freitag, 29. Juli 2022, 02:33:01 CEST schrieb Sean Anderson: > > On 7/28/22 3:52 PM, Rolf Eike Beer wrote: > > > Am 2022-07-27 05:58, schrieb Sean Anderson: > > >> On 7/26/22 11:49 PM, Sean Anderson wrote: > > >>> 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. > > > > > >>> diff --git a/drivers/net/ethernet/sun/sunhme.c > > >>> b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a > > >>> 100644 > > >>> --- a/drivers/net/ethernet/sun/sunhme.c > > >>> +++ b/drivers/net/ethernet/sun/sunhme.c > > >>> @@ -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)) { > > >> > > >> 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. > > > > 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
On 8/24/22 11:45 AM, Rolf Eike Beer wrote: > Am Montag, 1. August 2022, 17:14:39 CEST schrieb Rolf Eike Beer: >> Am Freitag, 29. Juli 2022, 02:33:01 CEST schrieb Sean Anderson: >>> On 7/28/22 3:52 PM, Rolf Eike Beer wrote: >>>> Am 2022-07-27 05:58, schrieb Sean Anderson: >>>>> On 7/26/22 11:49 PM, Sean Anderson wrote: >>>>>> 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. >>>> >>>>>> diff --git a/drivers/net/ethernet/sun/sunhme.c >>>>>> b/drivers/net/ethernet/sun/sunhme.c index eebe8c5f480c..e83774ffaa7a >>>>>> 100644 >>>>>> --- a/drivers/net/ethernet/sun/sunhme.c >>>>>> +++ b/drivers/net/ethernet/sun/sunhme.c >>>>>> @@ -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)) { >>>>> >>>>> 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. >>> >>> 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
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index 980a936ce8d1..ec78f43f75c9 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -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)
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 <eike-kernel@sf-tec.de> --- drivers/net/ethernet/sun/sunhme.c | 55 +++++++++---------------------- 1 file changed, 16 insertions(+), 39 deletions(-)