From owner-freebsd-net@FreeBSD.ORG Thu Feb 12 20:00:14 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 A2ECA2A4; Thu, 12 Feb 2015 20:00:14 +0000 (UTC) Received: from mail-wg0-x22b.google.com (mail-wg0-x22b.google.com [IPv6:2a00:1450:400c:c00::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 24785257; Thu, 12 Feb 2015 20:00:14 +0000 (UTC) Received: by mail-wg0-f43.google.com with SMTP id n12so12475579wgh.2; Thu, 12 Feb 2015 12:00:12 -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=UCKNYlxN5ys/T1JV5k1pCgT0PozS0yxTbefLZCnx0ZM=; b=ACqqyJeg/fdj4NmDUaVOhs1zZHAFcFymqsNnHXVZ9GQ6BaH9or4UpUJYeNIiZDLeg+ Y0ei6aD48QdQ7CRzHpiYKucc7WowiB+X81kCHwJh9E4cAmih1DXNvuuGFsosCBjQjC0d OC3RIsFQZ1VucQMl4ElPc8wWTYFdF0vNs5u9J1zkORfQ0CApK692VGDgcO8SZzzYL/pI kihSAL3DOWArNKQl8Cb0zt7Z7wtvDiC0LfWjDG71V3RL5z8SPhh07HK++53cvi744cgy 6p+iAQ2S9XahNHOMalv8I7ccfYJVLWsIwxfuThJmzoQs8VsCe3+K/zc6CeQBBJ8DTUf1 NkVw== MIME-Version: 1.0 X-Received: by 10.194.5.168 with SMTP id t8mr11386248wjt.150.1423771212550; Thu, 12 Feb 2015 12:00:12 -0800 (PST) Received: by 10.194.101.106 with HTTP; Thu, 12 Feb 2015 12:00:12 -0800 (PST) In-Reply-To: References: <20150127192814.GA63990@strugglingcoder.info> <26266AD2-4743-4A7B-A87D-F68E2E2425A0@juniper.net> Date: Thu, 12 Feb 2015 12:00:12 -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: Thu, 12 Feb 2015 20:00:14 -0000 I do not recall if I put that call in myself and, yes, it seems odd. It was probably trying to clean up some bad state a failed attach left things in. If it is removed it should be thoroughly regression tested. Jack On Thu, Feb 12, 2015 at 10:56 AM, Sreekanth Rupavatharam < rupavath@juniper.net> wrote: > Hi Jack, > Actually, looking at the code again, it seems to me that igb_detach is not > supposed to be called from igb_attach at all. It causes more problems than > previously mentioned. E.g., in case of branching to err_late *before* > igb_setup_interface(where the ifp is initialized), calling igb_detach there > causes a NULL pointer access. The best way to deal with it is to take out > the igb_detach call from the err_late: label. Thoughts? Comments? > -- Thanks, > > Sreekanth > > > > From: Jack Vogel > Date: Tuesday, January 27, 2015 at 12:42 PM > To: Sreekanth Rupavatharam > Cc: hiren panchasara , " > freebsd-net@freebsd.org" , "jfv@freebsd.org" < > jfv@freebsd.org> > Subject: Re: Double cleanup in igb_attach > > 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 >>>> >>>> >>> >> >