[KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace

[KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace

[Alexey Dobriyan]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

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.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace

[irwan.djajadi]

[-- Attachment #1: Type: text/plain, Size: 4790 bytes --]

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..

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 <irwan.djajadi@iname.com>

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);
 	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);
 
 				if (!stuffp->introk) {
 					xtrace(XFER,
 					       "error via interrupt\n");
-				} else if (!timeout) {
-					xtrace(XFER, "timeout\n");
 				} else if (signal_pending(current)) {
 					xtrace(XFER, "signal\n");
+				} else if (!timeout) {
+					xtrace(XFER, "timeout\n");
 				} else
 					continue;
 

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 5012 bytes --]

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?

> 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 <irwan.djajadi@iname.com>
> 
> 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.

>  	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

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [PATCH] drivers/cdrom/mcdx.c: replace

[Irwan Djajadi]

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 <irwan.djajadi@iname.com>
>>
>>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