[ This patch changed the code from using multiple come-from label names to using a single err label. Both are terrible ways to do error handling.
Come-From Labels: Come-from labels look like this: foo = alloc(); if (!foo) goto alloc_failed; The "alloc_failed" name is infuriating to look at because you can easily see that alloc_failed because you literally just read that on the line before. It looks like it is useful information but it's a trick. What we want to know is what does the goto do? The label name should be verb based. One Err Style Error Handling: This is the most bug prone way of handling errors. You can easily see that if you count the number of bugs from static checkers. These labels try to handle every possible condition which is more difficult than handling a specific error state. Plus, the label names for these are always vague and annoying. This patch introduces a classic One Err Bug. Kernel Style Error Handling: It's best if you use a string of error labels that each unwind a specific thing. Never free something that hasn't been allocated. Choose verb based labels like: err_release_bar: release_bar(bar); err_free_foo: kfree(foo); When you're reviewing code that uses this style of error handling you only need to track the most recently allocated thing. It works like this: foo = alloc(); if (!foo) return -ENOMEM; bar = get_bar(); if (!bar) goto err_free_foo; With this code you don't have to scroll up and down the function. It's obviously correct just from looking at those 6 lines. - dan ] Hello Nathan Fontenot, The patch 1b8955ee5f6c: "ibmvnic: Cleanup failure path in ibmvnic_open" from Mar 30, 2017, leads to the following static checker warning: drivers/net/ethernet/ibm/ibmvnic.c:672 ibmvnic_open() error: we previously assumed 'adapter->napi' could be null (see line 626) drivers/net/ethernet/ibm/ibmvnic.c 593 static int ibmvnic_open(struct net_device *netdev) 594 { 595 struct ibmvnic_adapter *adapter = netdev_priv(netdev); 596 struct device *dev = &adapter->vdev->dev; 597 union ibmvnic_crq crq; 598 int rc = 0; 599 int i; 600 601 if (adapter->is_closed) { 602 rc = ibmvnic_init(adapter); 603 if (rc) 604 return rc; 605 } 606 607 rc = ibmvnic_login(netdev); 608 if (rc) 609 return rc; ^^^^^^^^^^ See? We didn't clean up form ibmvnic_init() so this is buggy already. It should be goto uninit; The One Err Style error handling is too complicated so people just give up. But Kernel Style error handling methodically writes itself. 610 611 rc = netif_set_real_num_tx_queues(netdev, adapter->req_tx_queues); 612 if (rc) { 613 dev_err(dev, "failed to set the number of tx queues\n"); 614 return -1; 615 } 616 617 rc = init_sub_crq_irqs(adapter); 618 if (rc) { 619 dev_err(dev, "failed to initialize sub crq irqs\n"); 620 return -1; 621 } 622 623 adapter->map_id = 1; 624 adapter->napi = kcalloc(adapter->req_rx_queues, 625 sizeof(struct napi_struct), GFP_KERNEL); 626 if (!adapter->napi) 627 goto ibmvnic_open_fail; ^^^^^^^^^^^^^^^^^^^^^^ If we hit this goto then we will Oops. 628 for (i = 0; i < adapter->req_rx_queues; i++) { 629 netif_napi_add(netdev, &adapter->napi[i], ibmvnic_poll, 630 NAPI_POLL_WEIGHT); 631 napi_enable(&adapter->napi[i]); 632 } 633 634 send_map_query(adapter); 635 636 rc = init_rx_pools(netdev); 637 if (rc) 638 goto ibmvnic_open_fail; 639 640 rc = init_tx_pools(netdev); 641 if (rc) 642 goto ibmvnic_open_fail; 643 644 rc = init_bounce_buffer(netdev); 645 if (rc) 646 goto ibmvnic_open_fail; 647 648 replenish_pools(adapter); 649 650 /* We're ready to receive frames, enable the sub-crq interrupts and 651 * set the logical link state to up 652 */ 653 for (i = 0; i < adapter->req_rx_queues; i++) 654 enable_scrq_irq(adapter, adapter->rx_scrq[i]); 655 656 for (i = 0; i < adapter->req_tx_queues; i++) 657 enable_scrq_irq(adapter, adapter->tx_scrq[i]); 658 659 memset(&crq, 0, sizeof(crq)); 660 crq.logical_link_state.first = IBMVNIC_CRQ_CMD; 661 crq.logical_link_state.cmd = LOGICAL_LINK_STATE; 662 crq.logical_link_state.link_state = IBMVNIC_LOGICAL_LNK_UP; 663 ibmvnic_send_crq(adapter, &crq); 664 665 netif_tx_start_all_queues(netdev); 666 adapter->is_closed = false; 667 668 return 0; 669 670 ibmvnic_open_fail: 671 for (i = 0; i < adapter->req_rx_queues; i++) 672 napi_disable(&adapter->napi[i]); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Oops. 673 release_resources(adapter); 674 return -ENOMEM; 675 } regards, dan carpenter