| Message ID | 20220918232626.1601885-5-seanga2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sunhme: Cleanups and logging improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 61 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Am Montag, 19. September 2022, 01:26:17 CEST schrieb Sean Anderson: > In order to differentiate between a missing bridge and an OOM condition, > return ERR_PTRs from quattro_pci_find. This also does some general linting > in the area. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > drivers/net/ethernet/sun/sunhme.c | 33 +++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/sun/sunhme.c > b/drivers/net/ethernet/sun/sunhme.c index 1fc16801f520..52247505d08e 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -2569,30 +2569,33 @@ static void quattro_sbus_free_irqs(void) > #ifdef CONFIG_PCI > static struct quattro *quattro_pci_find(struct pci_dev *pdev) > { > + int i; > struct pci_dev *bdev = pdev->bus->self; > struct quattro *qp; > > - if (!bdev) return NULL; > + if (!bdev) > + return ERR_PTR(-ENODEV); > + > for (qp = qfe_pci_list; qp != NULL; qp = qp->next) { > struct pci_dev *qpdev = qp->quattro_dev; > > if (qpdev == bdev) > return qp; > } > + > qp = kmalloc(sizeof(struct quattro), GFP_KERNEL); > - if (qp != NULL) { > - int i; > + if (!qp) > + return ERR_PTR(-ENOMEM); > > - for (i = 0; i < 4; i++) > - qp->happy_meals[i] = NULL; > + for (i = 0; i < 4; i++) > + qp->happy_meals[i] = NULL; I know you are only reindenting it, but I dislike moving the variable up to the top of the function. Since the kernel is C99 meanwhile the variable could be declared just in the for loop. And when touching this anyway I think we could get rid of the magic "4" by using ARRAY_SIZE(qp->happy_meals). Or just replace the whole thing with memset(qp->happy_meals, 0, sizeof(qp- >happy_meals)). Eike
On 9/19/22 09:11, Rolf Eike Beer wrote: > Am Montag, 19. September 2022, 01:26:17 CEST schrieb Sean Anderson: >> In order to differentiate between a missing bridge and an OOM condition, >> return ERR_PTRs from quattro_pci_find. This also does some general linting >> in the area. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> drivers/net/ethernet/sun/sunhme.c | 33 +++++++++++++++++++------------ >> 1 file changed, 20 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/sun/sunhme.c >> b/drivers/net/ethernet/sun/sunhme.c index 1fc16801f520..52247505d08e 100644 >> --- a/drivers/net/ethernet/sun/sunhme.c >> +++ b/drivers/net/ethernet/sun/sunhme.c >> @@ -2569,30 +2569,33 @@ static void quattro_sbus_free_irqs(void) >> #ifdef CONFIG_PCI >> static struct quattro *quattro_pci_find(struct pci_dev *pdev) >> { >> + int i; >> struct pci_dev *bdev = pdev->bus->self; >> struct quattro *qp; >> >> - if (!bdev) return NULL; >> + if (!bdev) >> + return ERR_PTR(-ENODEV); >> + >> for (qp = qfe_pci_list; qp != NULL; qp = qp->next) { >> struct pci_dev *qpdev = qp->quattro_dev; >> >> if (qpdev == bdev) >> return qp; >> } >> + >> qp = kmalloc(sizeof(struct quattro), GFP_KERNEL); >> - if (qp != NULL) { >> - int i; >> + if (!qp) >> + return ERR_PTR(-ENOMEM); >> >> - for (i = 0; i < 4; i++) >> - qp->happy_meals[i] = NULL; >> + for (i = 0; i < 4; i++) >> + qp->happy_meals[i] = NULL; > > I know you are only reindenting it, but I dislike moving the variable up to > the top of the function. Since the kernel is C99 meanwhile the variable could > be declared just in the for loop. Hm, I thought this style was discouraged. > And when touching this anyway I think we > could get rid of the magic "4" by using ARRAY_SIZE(qp->happy_meals). Or just > replace the whole thing with memset(qp->happy_meals, 0, sizeof(qp- >> happy_meals)). Yeah, that avoids the whole problem. --Sean
On Sun, 18 Sep 2022 19:26:17 -0400 Sean Anderson wrote: > + int i; > struct pci_dev *bdev = pdev->bus->self; > struct quattro *qp; reverse xmas tree variable ordering please
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index 1fc16801f520..52247505d08e 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -2569,30 +2569,33 @@ static void quattro_sbus_free_irqs(void) #ifdef CONFIG_PCI static struct quattro *quattro_pci_find(struct pci_dev *pdev) { + int i; struct pci_dev *bdev = pdev->bus->self; struct quattro *qp; - if (!bdev) return NULL; + if (!bdev) + return ERR_PTR(-ENODEV); + for (qp = qfe_pci_list; qp != NULL; qp = qp->next) { struct pci_dev *qpdev = qp->quattro_dev; if (qpdev == bdev) return qp; } + qp = kmalloc(sizeof(struct quattro), GFP_KERNEL); - if (qp != NULL) { - int i; + if (!qp) + return ERR_PTR(-ENOMEM); - for (i = 0; i < 4; i++) - qp->happy_meals[i] = NULL; + for (i = 0; i < 4; i++) + qp->happy_meals[i] = NULL; - qp->quattro_dev = bdev; - qp->next = qfe_pci_list; - qfe_pci_list = qp; + qp->quattro_dev = bdev; + qp->next = qfe_pci_list; + qfe_pci_list = qp; - /* No range tricks necessary on PCI. */ - qp->nranges = 0; - } + /* No range tricks necessary on PCI. */ + qp->nranges = 0; return qp; } #endif /* CONFIG_PCI */ @@ -2949,11 +2952,15 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) { qp = quattro_pci_find(pdev); - if (qp == NULL) + if (IS_ERR(qp)) { + err = PTR_ERR(qp); goto err_out; + } + for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) - if (qp->happy_meals[qfe_slot] == NULL) + if (!qp->happy_meals[qfe_slot]) break; + if (qfe_slot == 4) goto err_out; }
In order to differentiate between a missing bridge and an OOM condition, return ERR_PTRs from quattro_pci_find. This also does some general linting in the area. Signed-off-by: Sean Anderson <seanga2@gmail.com> --- drivers/net/ethernet/sun/sunhme.c | 33 +++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-)