Double cleanup in igb_attach

[Sreekanth Rupavatharam]

Apologies if this is not the right forum. In igb_attach function, we have this code.
err_late:
igb_detach(dev);
        igb_free_transmit_structures(adapter);
        igb_free_receive_structures(adapter);
        igb_release_hw_control(adapter);
err_pci:
        igb_free_pci_resources(adapter);
        if (adapter->ifp != NULL)
                if_free(adapter->ifp);
        free(adapter->mta, M_DEVBUF);
        IGB_CORE_LOCK_DESTROY(adapter);

The problem is that igb_detach also does the same cleanup in it’s body. Only exception is this case where it just returns EBUSY
        /* Make sure VLANS are not using driver */
if (if_vlantrunkinuse(ifp)) {
device_printf(dev,"Vlan in use, detach first\n");
return (EBUSY);
}

I think the code in igb_attach should be changed to free up resources only if the igb_detach returns an error. Here’s the patch for it.


Index: if_igb.c

===================================================================

--- if_igb.c (revision 298025)

+++ if_igb.c (working copy)

@@ -723,7 +723,8 @@ igb_attach(device_t dev)

  return (0);



 err_late:

- igb_detach(dev);

+ if(igb_detach(dev) == 0) /* igb_detach did the cleanup */

+ return;

  igb_free_transmit_structures(adapter);

 Can anyone comment on it and tell me if my understanding is incorrect?

-- Thanks,

Sreekanth