Double cleanup in igb_attach

[hiren panchasara]

+ Jack
On Tue, Jan 27, 2015 at 12:00:19AM +0000, Sreekanth Rupavatharam wrote:
> 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?
>

Seems reasonable to me at the first glance.

We need to call IGB_CORE_LOCK_DESTROY(adapter) before returning though.

cheers,
Hiren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 618 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20150127/c042d28a/attachment.sig>