Date: Sun, 15 Feb 1998 19:14:52 +1030 From: Greg Lehey <grog@lemis.com> To: "Jordan K. Hubbard" <jkh@time.cdrom.com> Cc: Chris Csanady <ccsanady@friley585.res.iastate.edu>, freebsd-current@FreeBSD.ORG Subject: Re: CCD missing spl() call.. Message-ID: <19980215191452.63038@freebie.lemis.com> In-Reply-To: <20313.887531520@time.cdrom.com>; from Jordan K. Hubbard on Sun, Feb 15, 1998 at 12:32:00AM -0800 References: <19980215154350.44866@freebie.lemis.com> <20313.887531520@time.cdrom.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 15 February 1998 at 0:32:00 -0800, Jordan K. Hubbard wrote:
>> OK. The first one (off by 1) makes sense, but the second one (splbio)
>> was there from the start. What sources are you comparing with?
>
> Actually, there should be no splbio() in the interrupt routine
> and now that I've actually gotten the grit out of my eyes and
> looked at it again, I see that the whole addition was bogus
> for a variety of reasons:
>
> 1. There was no corresponding splx(s), even if the
> splbio() had been correct.
(omitting some #ifdef DEBUG stuff)
s = splbio();
if (cbp->cb_buf.b_flags & B_ERROR) {
bp->b_flags |= B_ERROR;
bp->b_error = cbp->cb_buf.b_error ? cbp->cb_buf.b_error : EIO;
}
if (ccd_softc[unit].sc_cflags & CCDF_MIRROR &&
(cbp->cb_buf.b_flags & B_READ) == 0)
if ((cbp->cb_pflags & CCDPF_MIRROR_DONE) == 0) {
/* I'm done before my counterpart, so just set
partner's flag and return */
cbp->cb_mirror->cb_pflags |= CCDPF_MIRROR_DONE;
putccdbuf(cbp);
splx(s);
return;
}
You were saying?
> 2. You don't need to raise the SPL level in the
> interrupt routine.
This last piece of code is dicking around with another buffer (this is
part of the mirroring code). Imagine an interrupt at the comment in
this sequence:
if ((cbp->cb_pflags & CCDPF_MIRROR_DONE) == 0) {
/* I'm done before my counterpart, so just set
partner's flag and return */
cbp->cb_mirror->cb_pflags |= CCDPF_MIRROR_DONE;
I see the possibility of a hang there.
> 3. The tabs were smashed, violating the Bruce exclusion
> filter rules. :-)
They were? That's a reason, of course.
> The off-by-one stuff was OK and has been committed, but more care
> will have to be taken with any further "OpenBSD improvements" if
> they're going to make the grade. :-)
You missed the most important thing: the splbio() has always been
there. It would have been a very serious problem to have an splx()
without an splXXX(), anyway. The only "OpenBSD improvement" in this
area is that they appear to have lost it at some point, and then
returned it. Oh yes, and I didn't really think it was worth worrying
about so much, but it *is* correct.
Greg
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19980215191452.63038>