Date: Tue, 27 Jan 2015 21:03:10 +0000 From: Sreekanth Rupavatharam <rupavath@juniper.net> To: Adrian Chadd <adrian@freebsd.org>, Jack Vogel <jfvogel@gmail.com> 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: <D0ED3CFE.1A893%rupavath@juniper.net> In-Reply-To: <CAJ-VmomWoKyK26GdWNigePhf4aibgbRV-od2P8TmswNaY6xD9w@mail.gmail.com> 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> <CAFOYbckAP6t0jkhtswi%2BRnFGtJtN7=feCw6_vneS%2BSEZn4o2Pg@mail.gmail.com> <BABD0D32-30E3-4521-8838-976FF77AE244@juniper.net> <CAFOYbcmPF70yf%2B7BmTdDrrfUtyPP7pkCx8_%2BQdZcfOSA_dAK1w@mail.gmail.com> <CAJ-VmomWoKyK26GdWNigePhf4aibgbRV-od2P8TmswNaY6xD9w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Done, I have also attached the patch in the bug report. -- Thanks, Sreekanth On 1/27/15, 12:54 PM, "Adrian Chadd" <adrian@freebsd.org> wrote: >Hi! > >Sreekanth - this does look like it is valid and needs fixing. Just >file a FreeBSD PR (bugs.freebsd.org/submit/) and we'll assign it to >the intel team. > >Thanks! > > >-a > > >On 27 January 2015 at 12:42, Jack Vogel <jfvogel@gmail.com> wrote: >> 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" <jfvogel@gmail.com> 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" <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 >>>>> >>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>> >>>>> --- 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) =3D=3D 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 !=3D 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 >>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>> --- 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) =3D=3D 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 >>>>> >>>>> >>>> >>> >> _______________________________________________ >> freebsd-net@freebsd.org mailing list >> http://lists.freebsd.org/mailman/listinfo/freebsd-net >> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D0ED3CFE.1A893%rupavath>