Messages in this thread | | | From | Coiby Xu <> | Date | Thu, 15 Oct 2020 12:22:08 +0800 | Subject | Re: [PATCH v2 2/7] staging: qlge: Initialize devlink health dump framework |
| |
On Wed, Oct 14, 2020 at 04:08:46PM +0300, Dan Carpenter wrote: >On Wed, Oct 14, 2020 at 06:43:01PM +0800, Coiby Xu wrote: >> static int qlge_probe(struct pci_dev *pdev, >> const struct pci_device_id *pci_entry) >> { >> struct net_device *ndev = NULL; >> struct qlge_adapter *qdev = NULL; >> + struct devlink *devlink; >> static int cards_found; >> int err = 0; >> >> - ndev = alloc_etherdev_mq(sizeof(struct qlge_adapter), >> + devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_adapter)); >> + if (!devlink) >> + return -ENOMEM; >> + >> + qdev = devlink_priv(devlink); >> + >> + ndev = alloc_etherdev_mq(sizeof(struct qlge_netdev_priv), >> min(MAX_CPUS, >> netif_get_num_default_rss_queues())); >> if (!ndev) >> - return -ENOMEM; >> + goto devlink_free; >> >> - err = qlge_init_device(pdev, ndev, cards_found); >> - if (err < 0) { >> - free_netdev(ndev); >> - return err; > >In the old code, if qlge_init_device() fails then it frees "ndev". > >> - } >> + qdev->ndev = ndev; >> + err = qlge_init_device(pdev, qdev, cards_found); >> + if (err < 0) >> + goto devlink_free; > >But the patch introduces a new resource leak.
Thank you for spotting this issue!
> >> >> - qdev = netdev_priv(ndev); >> SET_NETDEV_DEV(ndev, &pdev->dev); >> ndev->hw_features = NETIF_F_SG | >> NETIF_F_IP_CSUM | >> @@ -4611,8 +4619,14 @@ static int qlge_probe(struct pci_dev *pdev, >> qlge_release_all(pdev); >> pci_disable_device(pdev); >> free_netdev(ndev); >> - return err; >> + goto devlink_free; >> } >> + >> + err = devlink_register(devlink, &pdev->dev); >> + if (err) >> + goto devlink_free; >> + >> + qlge_health_create_reporters(qdev); >> /* Start up the timer to trigger EEH if >> * the bus goes dead >> */ >> @@ -4623,6 +4637,10 @@ static int qlge_probe(struct pci_dev *pdev, >> atomic_set(&qdev->lb_count, 0); >> cards_found++; >> return 0; >> + >> +devlink_free: >> + devlink_free(devlink); >> + return err; >> } > >The best way to write error handling code is keep tracke of the most >recent allocation which was allocated successfully. > > one = alloc(); > if (!one) > return -ENOMEM; // <-- nothing allocated successfully > > two = alloc(); > if (!two) { > ret = -ENOMEM; > goto free_one; // <-- one was allocated successfully > // Notice that the label name says what > // the goto does. > } > > three = alloc(); > if (!three) { > ret = -ENOMEM; > goto free_two; // <-- two allocated, two freed. > } > > ... > > return 0; > >free_two: > free(two); >free_one: > free(one); > > return ret; > Thank you for teaching me this pattern!
>In the old code qlge_probe() freed things before returning, and that's >fine if there is only two allocations in the function but when there are >three or more allocations, then use gotos to unwind. > >Ideally there would be a ql_deinit_device() function to mirror the >ql_init_device() function. The ql_init_device() is staging quality >code with leaks and bad label names. It should be re-written to free >things one step at a time instead of calling ql_release_all(). > I'll see how I can improve ql_init_device. Thank you for the suggestion!
>Anyway, let's not introduce new leaks at least. > >regards, >dan carpenter
-- Best regards, Coiby
| |