Double cleanup in igb_attach

[Jack Vogel]

If you want something committed to an Intel driver, asking Intel might be
the
courteous thing to do, don't you think?

Jack


On Tue, Jan 27, 2015 at 11:51 AM, Sreekanth Rupavatharam <
rupavath at juniper.net> wrote:

>  Hiren,
> Can you help commit this?
>
> Index: if_igb.c
>
> ===================================================================
>
> --- if_igb.c (revision 298053)
>
> +++ 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(error);
>
>   igb_free_transmit_structures(adapter);
>
>   igb_free_receive_structures(adapter);
>
>   igb_release_hw_control(adapter);
>
>  -- Thanks,
>
>  Sreekanth
>
>
>
>
>
>
>  On 1/27/15, 11:28 AM, "hiren panchasara" <hiren at strugglingcoder.info>
> wrote:
>
>  + 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
>
>