Double cleanup in igb_attach

[Jack Vogel]

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

Jack


On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam <
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> 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> 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
>>
>>
>