From owner-freebsd-bugs Sun Oct 7 9: 0: 5 2001 Delivered-To: freebsd-bugs@hub.freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id 7FAAA37B401 for ; Sun, 7 Oct 2001 09:00:02 -0700 (PDT) Received: (from gnats@localhost) by freefall.freebsd.org (8.11.4/8.11.4) id f97G02O77773; Sun, 7 Oct 2001 09:00:02 -0700 (PDT) (envelope-from gnats) Date: Sun, 7 Oct 2001 09:00:02 -0700 (PDT) Message-Id: <200110071600.f97G02O77773@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: Dima Dorfman Subject: Re: kern/30985: incorrect signal handling in snpread() Reply-To: Dima Dorfman Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org The following reply was made to PR kern/30985; it has been noted by GNATS. From: Dima Dorfman To: Valentin Nechayev Cc: FreeBSD-gnats-submit@freebsd.org Subject: Re: kern/30985: incorrect signal handling in snpread() Date: Sun, 07 Oct 2001 08:50:41 -0700 Valentin Nechayev wrote: > >Fix: > > Following simple fix provides checking for tsleep() return code > and EINTR returning when signal is caught. It is applicable for > all late 4.* systems. > It also increases IPL around the data wait cycle. As data can be put > to snoop device during interrupt (am I wrong?), it is desirable. I don't think it's possible for data to enter snp during an interrupt. At least, there are no provisions for this anywhere else in the code. Thus, I think this is undesirable. > But it should be noted that there are some principal architectural > issues in this approach. As snp device is designed for use when > select/poll is used to wait and ioctl(FIONREAD) is desired to get > tty status, it may be desirable to avoid blocking reads of snp device > totally. I can't suppose what approach is more right. I think it's okay to sleep in snpread. snp generally tries to act like any other driver, and most allow sleeping in their read routine. > do { > if (snp->snp_len == 0) { > - if (flag & IO_NDELAY) > + if (flag & IO_NDELAY) { > + splx(s); > return (EWOULDBLOCK); > + } > snp->snp_flags |= SNOOP_RWAIT; > - tsleep((caddr_t) snp, (PZERO + 1) | PCATCH, "snoopread" > , 0); > + error = tsleep((caddr_t) snp, (PZERO + 1) | PCATCH, "snoopread", 0); > + if (error == EINTR || error == ERESTART) { > + splx(s); > + return EINTR; > + } Why can't we just return whatever tsleep() returns, as most (all?) other drivers do? Like so (untested): Index: snp.c =================================================================== RCS file: /ref/cvsf/src/sys/dev/snp/snp.c,v retrieving revision 1.63 diff -u -r1.63 snp.c --- snp.c 2001/09/12 08:37:11 1.63 +++ snp.c 2001/10/07 15:44:47 @@ -255,7 +255,10 @@ if (flag & IO_NDELAY) return (EWOULDBLOCK); snp->snp_flags |= SNOOP_RWAIT; - tsleep((caddr_t)snp, (PZERO + 1) | PCATCH, "snprd", 0); + error = tsleep((caddr_t)snp, (PZERO + 1) | PCATCH, + "snprd", 0); + if (error != 0) + return (error); } } while (snp->snp_len == 0); To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message