Double cleanup in igb_attach

Sreekanth Rupavatharam rupavath at juniper.net
Tue Jan 27 22:36:02 UTC 2015


Done,
	I have also attached the patch in the bug report.

-- Thanks,

Sreekanth







On 1/27/15, 12:54 PM, "Adrian Chadd" <adrian at freebsd.org> wrote:

>Hi!
>
>Sreekanth - this does look like it is valid and needs fixing. Just
>file a FreeBSD PR (bugs.freebsd.org/submit/) and we'll assign it to
>the intel team.
>
>Thanks!
>
>
>-a
>
>
>On 27 January 2015 at 12:42, Jack Vogel <jfvogel at gmail.com> wrote:
>> 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
>>>>>
>>>>>
>>>>
>>>
>> _______________________________________________
>> freebsd-net at freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"



More information about the freebsd-net mailing list