From owner-freebsd-net@FreeBSD.ORG Tue Jan 27 20:42:24 2015 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 9DC9AD15; Tue, 27 Jan 2015 20:42:24 +0000 (UTC) Received: from mail-we0-x231.google.com (mail-we0-x231.google.com [IPv6:2a00:1450:400c:c03::231]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 35B61843; Tue, 27 Jan 2015 20:42:24 +0000 (UTC) Received: by mail-we0-f177.google.com with SMTP id l61so17059382wev.8; Tue, 27 Jan 2015 12:42:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=+gnTg0ciSd9tyUnf1D0zY9t6mp1Sz6BsCDkAIBrZuMg=; b=s/pwAVy5aaqj6PiOt0+34dsp5nz3LEdFCJl96AjilGGfmiXnWbCoJyo1SaHS70q8yp EF/nq0VlnSV5nMr5L7G7I+jwQWy9+G1KFnFvWMcBpzHEQbimk6BwMDRykq3ziA0KoPSQ 4HjTIFLrQRaZivliEhv1l7l8IEQUwtHiJaO4vkg3okSaT0UG+bKuwJHQ+YRZrkwsE8kb q+9pnWhs/RX6P57h6xgYwZ/gCXVNqwTrzWDdCutMGOPj/9eFWqfSkL44wbvNmTA47/sq pgEPnYMPsv5+XGvw9Vk7QltujVqJvVkKiwnQaSl2+saj3S35Ndsz6r5fiQRDZt3UTlFA pIrw== MIME-Version: 1.0 X-Received: by 10.194.219.68 with SMTP id pm4mr2663079wjc.71.1422391342735; Tue, 27 Jan 2015 12:42:22 -0800 (PST) Received: by 10.194.101.106 with HTTP; Tue, 27 Jan 2015 12:42:22 -0800 (PST) In-Reply-To: References: <20150127192814.GA63990@strugglingcoder.info> <26266AD2-4743-4A7B-A87D-F68E2E2425A0@juniper.net> Date: Tue, 27 Jan 2015 12:42:22 -0800 Message-ID: Subject: Re: Double cleanup in igb_attach From: Jack Vogel To: Sreekanth Rupavatharam Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.18-1 Cc: "jfv@freebsd.org" , "freebsd-net@freebsd.org" , hiren panchasara X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Jan 2015 20:42:24 -0000 Yes, I will look them over. Jack On Tue, Jan 27, 2015 at 12:38 PM, Sreekanth Rupavatharam < rupavath@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" wrote: > > 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" 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" >>> 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 >>> >>> >> >