Date: Tue, 27 Jan 2015 12:24:17 -0800 From: Jack Vogel <jfvogel@gmail.com> To: Sreekanth Rupavatharam <rupavath@juniper.net> Cc: "jfv@freebsd.org" <jfv@freebsd.org>, "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, hiren panchasara <hiren@strugglingcoder.info> Subject: Re: Double cleanup in igb_attach Message-ID: <CAFOYbckAP6t0jkhtswi%2BRnFGtJtN7=feCw6_vneS%2BSEZn4o2Pg@mail.gmail.com> In-Reply-To: <26266AD2-4743-4A7B-A87D-F68E2E2425A0@juniper.net> References: <D0EC151C.1A7B1%rupavath@juniper.net> <20150127192814.GA63990@strugglingcoder.info> <D0ED2984.1A86C%rupavath@juniper.net> <CAFOYbcmBks6s448stkVjGsWxxNZdZEwX2zxtdUh5cDnmtKEA1w@mail.gmail.com> <26266AD2-4743-4A7B-A87D-F68E2E2425A0@juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Errrr, I am one of those people :) (jack.vogel@intel.com)
Jack
On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam <
rupavath@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@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@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@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
>>
>>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFOYbckAP6t0jkhtswi%2BRnFGtJtN7=feCw6_vneS%2BSEZn4o2Pg>