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=err_late&project=X-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.englab.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev/e1000/if_em.c#em_free_transmit_structures>(adapter<http://optimus.englab.juniper.net:8180/source/s?defs=adapter&project=X-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_receive_structures>(adapter<http://optimus.englab.juniper.net:8180/source/s?defs=adapter&project=X-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.juniper.net:8180/source/xref/X-FreeBSD-10/sys/dev/e1000/if_em.c#em_release_hw_control>(adapter<http://optimus.englab.juniper.net:8180/source/s?defs=adapter&project=X-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:8180/source/s?defs=adapter&project=X-FreeBSD-10>->ifp<http://optimus.englab.juniper.net:8180/source/s?defs=ifp&project=X-FreeBSD-10>
!=
NULL<http://optimus.englab.juniper.net:8180/source/s?defs=NULL&project=X-FreeBSD-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.net:8180/source/s?defs=if_free&project=X-FreeBSD-10>(adapter<http://optimus.englab.juniper.net:8180/source/s?defs=adapter&project=X-FreeBSD-10>->ifp<http://optimus.englab.juniper.net:8180/source/s?defs=ifp&project=X-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=err_pci&project=X-FreeBSD-10>:
-- Thanks,
Sreekanth
From: Jack Vogel <jfvo...@gmail.com<mailto:jfvo...@gmail.com>>
Date: Thursday, February 12, 2015 at 12:00 PM
To: Sreekanth Rupavatharam <rupav...@juniper.net<mailto:rupav...@juniper.net>>
Cc: hiren panchasara
<hi...@strugglingcoder.info<mailto:hi...@strugglingcoder.info>>,
"freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>"
<freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>>,
"j...@freebsd.org<mailto:j...@freebsd.org>"
<j...@freebsd.org<mailto:j...@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
<rupav...@juniper.net<mailto:rupav...@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 <jfvo...@gmail.com<mailto:jfvo...@gmail.com>>
Date: Tuesday, January 27, 2015 at 12:42 PM
To: Sreekanth Rupavatharam <rupav...@juniper.net<mailto:rupav...@juniper.net>>
Cc: hiren panchasara
<hi...@strugglingcoder.info<mailto:hi...@strugglingcoder.info>>,
"freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>"
<freebsd-net@freebsd.org<mailto:freebsd-net@freebsd.org>>,
"j...@freebsd.org<mailto:j...@freebsd.org>"
<j...@freebsd.org<mailto:j...@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
<rupav...@juniper.net<mailto:rupav...@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"
<jfvo...@gmail.com<mailto:jfvo...@gmail.com>> wrote:
Errrr, I am one of those people :)
(jack.vo...@intel.com<mailto:jack.vo...@intel.com>)
Jack
On Tue, Jan 27, 2015 at 12:21 PM, Sreekanth Rupavatharam
<rupav...@juniper.net<mailto:rupav...@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"
<jfvo...@gmail.com<mailto:jfvo...@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
<rupav...@juniper.net<mailto:rupav...@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"
<hi...@strugglingcoder.info<mailto:hi...@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
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
