From owner-freebsd-net@FreeBSD.ORG Tue Jan 27 20:15:49 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 EC473E7; Tue, 27 Jan 2015 20:15:49 +0000 (UTC) Received: from mail-wi0-x235.google.com (mail-wi0-x235.google.com [IPv6:2a00:1450:400c:c05::235]) (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 725DA25B; Tue, 27 Jan 2015 20:15:49 +0000 (UTC) Received: by mail-wi0-f181.google.com with SMTP id fb4so7530703wid.2; Tue, 27 Jan 2015 12:15:47 -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=EkHU9iaE90NvKIpvOVO/esllh24KrWgZGiVnXzxwseY=; b=oKlq8aL0A4m3yxt9BzQsTK14Cy6RK5etyXmageBLhU8PSsd5yE596G18NxtN9413t+ lEE4+5VDlP2XAIXkmjYnpXjc+lVg70jXFEGn2vJL+0ghlJQ7qEKzxvc5YibDpNsp/Rp2 z/lqvKK1mn2BN5C0VWb8t35/ZFoRXI2Qjr0CNaEP8nBbnewYIYwcRY22dxs1RDb3k/Ha 1HochiPI7w74XcY4qRmkIJeWGij+3eE/ad423Ua+ZtMvlf1yKyh6g/8lFh+44OFCiG2W RPWCUkiTbELynCSueejGRPodneUKZG1hIphyME7ouATc+s2aUySpAi/CA7wPp1/3win4 NyKA== MIME-Version: 1.0 X-Received: by 10.180.218.39 with SMTP id pd7mr357007wic.21.1422389747395; Tue, 27 Jan 2015 12:15:47 -0800 (PST) Received: by 10.194.101.106 with HTTP; Tue, 27 Jan 2015 12:15:47 -0800 (PST) In-Reply-To: References: <20150127192814.GA63990@strugglingcoder.info> Date: Tue, 27 Jan 2015 12:15:47 -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:15:50 -0000 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 > >