Double cleanup in igb_attach

[Adrian Chadd]

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"