Double cleanup in igb_attach

[Sreekanth Rupavatharam]

Thanks jack,
     Now, can you please review these changes? And commit if you deem it fit?

Thanks,

-Sreekanth

On Jan 27, 2015, at 12:24 PM, "Jack Vogel" <jfvogel at gmail.com<mailto:jfvogel at gmail.com>> wrote:

Errrr, I am one of those people :)  (jack.vogel at intel.com<mailto:jack.vogel at intel.com>)

Jack


On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam <rupavath at juniper.net<mailto:rupavath at juniper.net>> wrote:
Definitely, but I didn't have the contact info of those people.

Thanks,

-Sreekanth

On Jan 27, 2015, at 12:15 PM, "Jack Vogel" <jfvogel at gmail.com<mailto:jfvogel at gmail.com>> wrote:

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<mailto: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<mailto: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