Date: Thu, 12 Feb 2015 21:36:39 +0000 From: Sreekanth Rupavatharam <rupavath@juniper.net> To: 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: <D1025BBD.1B686%rupavath@juniper.net> In-Reply-To: <CAFOYbc=ed3vrp00aQXQsTPGzerysmYk%2BYEBDzi5TG9ZtxHZJbg@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> <D102367E.1B663%rupavath@juniper.net> <CAFOYbc=ed3vrp00aQXQsTPGzerysmYk%2BYEBDzi5TG9ZtxHZJbg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Looking at the em driver, the em_detach is missing from the same place. return (0); 759<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#759> 760<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#760>err_late<http://optimus.englab.juniper.net:8180/source/s= ?defs=3Derr_late&project=3DX-FreeBSD-10>: 761<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#761> em_free_transmit_structures<http://optimus.engla= b.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev/e1000/if_em.c#em_free_t= ransmit_structures>(adapter<http://optimus.englab.juniper.net:8180/source/s= ?defs=3Dadapter&project=3DX-FreeBSD-10>); 762<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#762> em_free_receive_structures<http://optimus.englab= .juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev/e1000/if_em.c#em_free_re= ceive_structures>(adapter<http://optimus.englab.juniper.net:8180/source/s?d= efs=3Dadapter&project=3DX-FreeBSD-10>); 763<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#763> em_release_hw_control<http://optimus.englab.juni= per.net:8180/source/xref/X-FreeBSD-10/sys/dev/e1000/if_em.c#em_release_hw_c= ontrol>(adapter<http://optimus.englab.juniper.net:8180/source/s?defs=3Dadap= ter&project=3DX-FreeBSD-10>); 764<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#764> if (adapter<http://optimus.englab.juniper.net:81= 80/source/s?defs=3Dadapter&project=3DX-FreeBSD-10>->ifp<http://optimus.engl= ab.juniper.net:8180/source/s?defs=3Difp&project=3DX-FreeBSD-10> !=3D NULL<h= ttp://optimus.englab.juniper.net:8180/source/s?defs=3DNULL&project=3DX-Free= BSD-10>) 765<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#765> if_free<http://optimus.englab.juniper.ne= t:8180/source/s?defs=3Dif_free&project=3DX-FreeBSD-10>(adapter<http://optim= us.englab.juniper.net:8180/source/s?defs=3Dadapter&project=3DX-FreeBSD-10>-= >ifp<http://optimus.englab.juniper.net:8180/source/s?defs=3Difp&project=3DX= -FreeBSD-10>); 766<http://optimus.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev= /e1000/if_em.c#766>err_pci<http://optimus.englab.juniper.net:8180/source/s?= defs=3Derr_pci&project=3DX-FreeBSD-10>: -- Thanks, Sreekanth From: Jack Vogel <jfvogel@gmail.com<mailto:jfvogel@gmail.com>> Date: Thursday, February 12, 2015 at 12:00 PM To: Sreekanth Rupavatharam <rupavath@juniper.net<mailto:rupavath@juniper.ne= t>> Cc: hiren panchasara <hiren@strugglingcoder.info<mailto:hiren@strugglingcod= er.info>>, "freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>" <freeb= sd-net@freebsd.org<mailto:freebsd-net@freebsd.org>>, "jfv@freebsd.org<mailt= o:jfv@freebsd.org>" <jfv@freebsd.org<mailto:jfv@freebsd.org>> Subject: Re: Double cleanup in igb_attach 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<mailto: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 cau= ses 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 <jfvogel@gmail.com<mailto:jfvogel@gmail.com>> Date: Tuesday, January 27, 2015 at 12:42 PM To: Sreekanth Rupavatharam <rupavath@juniper.net<mailto:rupavath@juniper.ne= t>> Cc: hiren panchasara <hiren@strugglingcoder.info<mailto:hiren@strugglingcod= er.info>>, "freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>" <freeb= sd-net@freebsd.org<mailto:freebsd-net@freebsd.org>>, "jfv@freebsd.org<mailt= o:jfv@freebsd.org>" <jfv@freebsd.org<mailto: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<mailto:rupavath@juniper.net>> wrote: Thanks jack, Now, can you please review these changes? And commit if you deem it fi= t? Thanks, -Sreekanth On Jan 27, 2015, at 12:24 PM, "Jack Vogel" <jfvogel@gmail.com<mailto:jfvoge= l@gmail.com>> wrote: Errrr, I am one of those people :) (jack.vogel@intel.com<mailto:jack.vogel= @intel.com>) Jack On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam <rupavath@juniper.= net<mailto: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<mailto:jfvoge= l@gmail.com>> wrote: If you want something committed to an Intel driver, asking Intel might be t= he courteous thing to do, don't you think? Jack On Tue, Jan 27, 2015 at 11:51 AM, Sreekanth Rupavatharam <rupavath@juniper.= net<mailto: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<mailto= :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 t= his 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. Onl= y 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D1025BBD.1B686%rupavath>