Double cleanup in igb_attach

[Jack Vogel]

Yes, I will look them over.

Jack


On Tue, Jan 27, 2015 at 12:38 PM, Sreekanth Rupavatharam <
rupavath at juniper.net> wrote:

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