Date: Tue, 27 Jan 2015 12:15:47 -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: <CAFOYbcmBks6s448stkVjGsWxxNZdZEwX2zxtdUh5cDnmtKEA1w@mail.gmail.com> In-Reply-To: <D0ED2984.1A86C%rupavath@juniper.net> References: <D0EC151C.1A7B1%rupavath@juniper.net> <20150127192814.GA63990@strugglingcoder.info> <D0ED2984.1A86C%rupavath@juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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?CAFOYbcmBks6s448stkVjGsWxxNZdZEwX2zxtdUh5cDnmtKEA1w>