Re: LOR with netisr changes

On Sunday 03 December 2006 10:49, Munehiro Matsuda wrote:
> From: John Baldwin <jhb-at-freebsd.org>
> Date: Wed, 29 Nov 2006 12:39:56 -0500
> ::On Tuesday 28 November 2006 14:41, Sam Leffler wrote:
> ::> Robert Watson wrote:
> ::> > 
> ::> > On Wed, 29 Nov 2006, Munehiro Matsuda wrote:
> ::> > 
> ::> >> JFYI, I got following LOR started after the netisr changes:
> ::> > 
> ::> > In general, device driver locks should not be held over entry to the
> ::> > network stack.  However, things get a bit tricky in the 802.11 code due
> ::> > to lock sharing, I believe, so it could be a bit more tricky to fix that.
> ::> 
> ::> It's just a bug in the driver.  Driver locks should be dropped when
> ::> packets get passed up the stack.  This issue was pointed out before iwi
> ::> ever was committed but since nothing immediately comes back via the
> ::> bridge (or similar) it's been ignored.
> ::
> ::How about this:
> <snip>
> ::(I'm unsure if the net82011 layer protects 'ic' or if the driver is supposed 
> ::to do that.)
> 
> Sorry for the late reply, and thanks for the patch.
> I needed to tweak the patch to make it compile, but seems to work fine.
> Modified patch attached.
> 
> From: "Bjoern A. Zeeb" <bzeeb-lists-at-lists.zabbadoz.net>
> Date: Sat, 2 Dec 2006 12:25:28 +0000 (UTC)
> ::ok, whoever is going to take care of this and perhaps commit the fix
> ::let me know. I assigned it LOR ID 194 on "The LOR page":
> :: 	http://sources.zabbadoz.net/freebsd/lor.html#194
> 
> LOR seems to be gone with the patch, so it could be removed when committed.
> 
> Thanks,
>   Haro
> 
> --- if_iwi.c.org	9 Nov 2006 16:05:43 -0000
> +++ if_iwi.c	3 Dec 2006 14:17:45 -0000
> _at__at_ -1230,6 +1230,7 _at__at_
>  	struct mbuf *mnew, *m;
>  	struct ieee80211_node *ni;
>  	int type, error, framelen;
> +	IWI_LOCK_DECL;

Hmm, why this?   If it's for spl's, note that the rest of the patch is
specific to 5.x and up, shouldn't really be dropping spl in 4.x.  Also, even
if it did, it would pass a garbage value to splx(), so it would really break
things badly.

>  	framelen = le16toh(frame->len);
>  	if (framelen < IEEE80211_MIN_LEN || framelen > MCLBYTES) {
> _at__at_ -1310,6 +1311,7 _at__at_
>  
>  		bpf_mtap2(sc->sc_drvbpf, tap, sc->sc_rxtap_len, m);
>  	}
> +	IWI_UNLOCK(sc);
>  
>  	ni = ieee80211_find_rxnode(ic, mtod(m, struct ieee80211_frame_min *));
>  
> _at__at_ -1319,6 +1321,7 _at__at_
>  	/* node is no longer needed */
>  	ieee80211_free_node(ni);
>  
> +	IWI_LOCK(sc);
>  	if (sc->sc_softled) {
>  		/*
>  		 * Blink for any data frame.  Otherwise do a
> 
> =------------------------------------------------------------------------------
>            _ _    Munehiro (haro) Matsuda
>  -|- /_\  |_|_|   Internet Solution Dept., KGT Inc.
>  /|\ |_|  |_|_|   2-8-8 Shinjuku Shinjuku-ku Tokyo 160-0022, Japan
>                   Tel: +81-3-3225-0767  Fax: +81-3-3225-0740
> 

-- 
John Baldwin