From mboxrd@z Thu Jan 1 00:00:00 1970 From: Irwan Djajadi Date: Fri, 28 Oct 2005 03:53:00 +0000 Subject: Re: [KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace Message-Id: <4361A09C.4040209@iname.com> List-Id: References: <20051026191842.GA9416@mipter.zuzino.mipt.ru> In-Reply-To: <20051026191842.GA9416@mipter.zuzino.mipt.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kernel-janitors@vger.kernel.org Nishanth Aravamudan wrote: > On 26.10.2005 [22:56:05 -0500], irwan.djajadi@iname.com wrote: > >>On Wed, Oct 26, 2005 at 12:48:27PM -0700, Nishanth Aravamudan wrote: >> >>>On 26.10.2005 [23:18:42 +0400], Alexey Dobriyan wrote: >>> >>>>On Sat, Oct 22, 2005 at 09:59:18PM -0500, irwan.djajadi@iname.com wrote: >>>> >>>>>drivers/cdrom/mcdx.c: replace interruptible_sleep_on() with >>>>>wait_event_interruptible() and schedule_timeout_interruptible(). Removed >>>>>sleepq wait_queue, since it's no longer needed. Reordered some conditional >>>>>prints because wait_event_interruptible_timeout() returns -ERESTARTSYS when >>>>>interrupted by a signal. >>>> >>>>>--- 2.6.14-rc5/drivers/cdrom/mcdx.c >>>>>+++ mod/drivers/cdrom/mcdx.c >>>>>@@ -909,7 +908,7 @@ static int mcdx_talk(struct s_drive_stuf >>>>> >>>>> while (stuffp->lock) { >>>>> xtrace(SLEEP, "*** talk: lockq\n"); >>>>>- interruptible_sleep_on(&stuffp->lockq); >>>>>+ wait_event_interruptible(stuffp->lockq, !stuffp->lock); >>>>> xtrace(SLEEP, "talk: awoken\n"); >>>>> } >>>> >>>>>@@ -1335,7 +1333,7 @@ static int mcdx_xfer(struct s_drive_stuf >>>>> } >>>>> >>>>> while (stuffp->lock) { >>>>>- interruptible_sleep_on(&stuffp->lockq); >>>>>+ wait_event_interruptible(stuffp->lockq, !stuffp->lock); >>>>> } >>>> >>>>Looks like the whole loops should be replaced. >>>>wait_event_interruptible() does it's own looping. >>> >>>That seems fine for the second replacement, but the first will lose >>>per-iteration tracing. Depends on whether the maintainer cares. >>> >>>Thanks, >>>Nish >>> >> >>Ehh.. well, I think those prints are just for debugging, and aren't >>necessarily useful.. >>Here's my next try for mcdx.c.. > > > Maybe because you aren't debugging? But what if someone is? That's true, but he/she can put it back on their local copy if it's important enough to do debugging.. It's not really a big change, is it ? I thought Linux kernel submission should be done as clean as possible, and shouldn't include temporary debugging code ? In which case the patch is OK ? I know there's some balance btw letting user troubleshoot and not having debugging code at all, but I thought in this case, those prints are not really important.. I'm OK either way.. > > >>However, I think there is a bigger problem here. I think the code uses the >>wrong synchronization mechanism. Even with wait_event_interruptible(), there >>is no guarantee that by the time the call returns, stuffp->lock is 0. The >>current thread might get preempted by something else that grabs the lock, then >>the current thread assumes that it's the only one who is grabbing the lock, >>and then it's screwed.. I don't know why the code uses >>wait_eent_interruptible(). This seems buggy to me. Unless, I'm missing >>something else again. >> >>It seems that wait_event_interruptible() can only work well for things that >>can only happen once for the current context. >> >>Oh well, here's the fix to the patch.. >>Thanks >>-- >>Irwan >> >> >>mcdx.c: replace interruptible_sleep_on() with proper newer calls. >> >>Signed-off-by: Irwan Djajadi >> >>diff -pruNX 2.6.14-rc5/Documentation/dontdiff 2.6.14-rc5/drivers/cdrom/mcdx.c mod/drivers/cdrom/mcdx.c >>--- 2.6.14-rc5/drivers/cdrom/mcdx.c 2005-09-16 20:02:12.000000000 -0500 >>+++ mod/drivers/cdrom/mcdx.c 2005-10-26 22:38:18.000000000 -0500 >>@@ -155,7 +155,6 @@ struct s_drive_stuff { >> /* waitqueues */ >> wait_queue_head_t busyq; >> wait_queue_head_t lockq; >>- wait_queue_head_t sleepq; >> >> /* flags */ >> volatile int introk; /* status of last irq operation */ >>@@ -838,8 +837,8 @@ static void mcdx_delay(struct s_drive_st >> if (jifs < 0) >> return; >> >>- xtrace(SLEEP, "*** delay: sleepq\n"); >>- interruptible_sleep_on_timeout(&stuff->sleepq, jifs); >>+ xtrace(SLEEP, "*** delay: %ld jiffies\n", jifs); >>+ schedule_timeout_interruptible(jifs); > > > Nobody was doing a wake_up*() on stuff->sleepq? If they were, you've > changed the functionality. > Yes, I thought it was weird as well.. AFAIK, sleepq is never wake_up()-ed, in which case the code is only using it for delay, which schedule_timeout() does in cleaner way. > >> xtrace(SLEEP, "delay awoken\n"); >> if (signal_pending(current)) { >> xtrace(SLEEP, "got signal\n"); >>@@ -907,11 +906,7 @@ static int mcdx_talk(struct s_drive_stuf >> if ((discard = (buffer = NULL))) >> buffer = &c; >> >>- while (stuffp->lock) { >>- xtrace(SLEEP, "*** talk: lockq\n"); >>- interruptible_sleep_on(&stuffp->lockq); >>- xtrace(SLEEP, "talk: awoken\n"); >>- } >>+ wait_event_interruptible(stuffp->lockq, !stuffp->lock); >> >> stuffp->lock = 1; >> >>@@ -1115,7 +1110,6 @@ static int __init mcdx_init_drive(int dr >> >> init_waitqueue_head(&stuffp->busyq); >> init_waitqueue_head(&stuffp->lockq); >>- init_waitqueue_head(&stuffp->sleepq); >> >> /* check if i/o addresses are available */ >> if (!request_region(stuffp->wreg_data, MCDX_IO_SIZE, "mcdx")) { >>@@ -1334,9 +1328,7 @@ static int mcdx_xfer(struct s_drive_stuf >> return -1; >> } >> >>- while (stuffp->lock) { >>- interruptible_sleep_on(&stuffp->lockq); >>- } >>+ wait_event_interruptible(stuffp->lockq, !stuffp->lock); >> >> if (stuffp->valid && (sector >= stuffp->pending) >> && (sector < stuffp->low_border)) { >>@@ -1360,16 +1352,16 @@ static int mcdx_xfer(struct s_drive_stuf >> while (stuffp->busy) { >> >> timeout >>- interruptible_sleep_on_timeout >>- (&stuffp->busyq, 5 * HZ); >>+ wait_event_interruptible_timeout >>+ (stuffp->busyq, !stuffp->busy, 5 * HZ); > > > This is funky to me. I'll have to think about it. > > Thanks, > Nish > Yes, please let me know if I did something stupid. Thanks! -- Irwan _______________________________________________ Kernel-janitors mailing list Kernel-janitors@lists.osdl.org https://lists.osdl.org/mailman/listinfo/kernel-janitors