* [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
@ 2005-10-23 2:55 ` irwan.djajadi
2005-10-23 2:56 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace irwan.djajadi
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: irwan.djajadi @ 2005-10-23 2:55 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 636 bytes --]
drivers/block/swim3.c: replace interruptible_sleep_on() with
wait_event_interruptible()
Signed-off-by: Irwan Djajadi <irwan.djajadi@iname.com>
diff -pruNX 2.6.14-rc5/Documentation/dontdiff 2.6.14-rc5/drivers/block/swim3.c mod/drivers/block/swim3.c
--- 2.6.14-rc5/drivers/block/swim3.c 2005-10-22 00:06:17.000000000 -0500
+++ mod/drivers/block/swim3.c 2005-10-22 15:31:52.000000000 -0500
@@ -800,7 +800,7 @@ static int grab_drive(struct floppy_stat
restore_flags(flags);
return -EINTR;
}
- interruptible_sleep_on(&fs->wait);
+ wait_event_interruptible(fs->wait, fs->state == available);
}
--fs->wanted;
}
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
2005-10-23 2:55 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace irwan.djajadi
@ 2005-10-23 2:56 ` irwan.djajadi
2005-10-24 20:24 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace Alexey Dobriyan
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: irwan.djajadi @ 2005-10-23 2:56 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 655 bytes --]
drivers/block/swim_iop.c: replace interruptible_sleep_on() with
wait_event_interruptible()
Signed-off-by: Irwan Djajadi <irwan.djajadi@iname.com>
diff -pruNX 2.6.14-rc5/Documentation/dontdiff 2.6.14-rc5/drivers/block/swim_iop.c mod/drivers/block/swim_iop.c
--- 2.6.14-rc5/drivers/block/swim_iop.c 2005-10-22 00:06:17.000000000 -0500
+++ mod/drivers/block/swim_iop.c 2005-10-22 15:33:26.000000000 -0500
@@ -436,7 +436,7 @@ static int grab_drive(struct floppy_stat
local_irq_restore(flags);
return -EINTR;
}
- interruptible_sleep_on(&fs->wait);
+ wait_event_interruptible(fs->wait, fs->state == available);
}
--fs->wanted;
}
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
2005-10-23 2:55 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace irwan.djajadi
2005-10-23 2:56 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace irwan.djajadi
@ 2005-10-24 20:24 ` Alexey Dobriyan
2005-10-24 21:23 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace Alexey Dobriyan
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Alexey Dobriyan @ 2005-10-24 20:24 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]
On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com wrote:
> drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
> wait_event_interruptible()
> --- 2.6.14-rc5/drivers/block/acsi_slm.c
> +++ mod/drivers/block/acsi_slm.c
> @@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
>
> while( SLMState == PRINTING ||
> (SLMState == FILLING && SLMBufOwner != device) ) {
> - interruptible_sleep_on( &slm_wait );
> + wait_event_interruptible( slm_wait, SLMState==IDLE );
> if (signal_pending(current))
> return( -ERESTARTSYS );
> }
acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
from -kj looks more correct.
From: Nishanth Aravamudan
Use wait_event_interruptible() instead of the deprecated
interruptible_sleep_on(). The sleep_on() call later in the same
function is replaced with inline wait-queue code which achieves the
same. This required adding a local wait-queue, though.
--- a/drivers/block/acsi_slm.c
+++ b/drivers/block/acsi_slm.c
@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
int device = iminor(node);
int n, filled, w, h;
- while( SLMState == PRINTING ||
- (SLMState == FILLING && SLMBufOwner != device) ) {
- interruptible_sleep_on( &slm_wait );
- if (signal_pending(current))
- return( -ERESTARTSYS );
- }
+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
+ (SLMState != FILLING || SLMBufOwner == device)));
+ if (signal_pending(current))
+ return -ERESTARTSYS;
if (SLMState == IDLE) {
/* first data of page: get current page size */
if (slm_get_pagesize( device, &w, &h ))
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (2 preceding siblings ...)
2005-10-24 20:24 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace Alexey Dobriyan
@ 2005-10-24 21:23 ` Alexey Dobriyan
2005-10-24 21:25 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace Alexey Dobriyan
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Alexey Dobriyan @ 2005-10-24 21:23 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
On Sat, Oct 22, 2005 at 09:55:25PM -0500, irwan.djajadi@iname.com wrote:
> --- 2.6.14-rc5/drivers/block/swim3.c
> +++ mod/drivers/block/swim3.c
> @@ -800,7 +800,7 @@ static int grab_drive(struct floppy_stat
save_flags(flags);
cli();
if (fs->state != idle) {
++fs->wanted;
while (fs->state != available) {
if (interruptible && signal_pending(current)) {
> restore_flags(flags);
> return -EINTR;
> }
> - interruptible_sleep_on(&fs->wait);
> + wait_event_interruptible(fs->wait, fs->state == available);
Correct me if I'm wrong, but level of bugginess remains the same:
scheduling with interrupts disabled.
> }
> --fs->wanted;
> }
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (3 preceding siblings ...)
2005-10-24 21:23 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace Alexey Dobriyan
@ 2005-10-24 21:25 ` Alexey Dobriyan
2005-10-24 23:46 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace Irwan Djajadi
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Alexey Dobriyan @ 2005-10-24 21:25 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 442 bytes --]
On Sat, Oct 22, 2005 at 09:56:31PM -0500, irwan.djajadi@iname.com wrote:
> --- 2.6.14-rc5/drivers/block/swim_iop.c
> +++ mod/drivers/block/swim_iop.c
> @@ -436,7 +436,7 @@ static int grab_drive(struct floppy_stat
> local_irq_restore(flags);
> return -EINTR;
> }
> - interruptible_sleep_on(&fs->wait);
> + wait_event_interruptible(fs->wait, fs->state == available);
> }
> --fs->wanted;
> }
Ditto. local_irq_save().
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (4 preceding siblings ...)
2005-10-24 21:25 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace Alexey Dobriyan
@ 2005-10-24 23:46 ` Irwan Djajadi
2005-10-25 3:12 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace irwan.djajadi
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Irwan Djajadi @ 2005-10-24 23:46 UTC (permalink / raw)
To: kernel-janitors
Alexey Dobriyan wrote:
>On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com wrote:
>
>
>>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
>>wait_event_interruptible()
>>
>>
>
>
>
>>--- 2.6.14-rc5/drivers/block/acsi_slm.c
>>+++ mod/drivers/block/acsi_slm.c
>>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
>>
>> while( SLMState = PRINTING ||
>> (SLMState = FILLING && SLMBufOwner != device) ) {
>>- interruptible_sleep_on( &slm_wait );
>>+ wait_event_interruptible( slm_wait, SLMState=IDLE );
>> if (signal_pending(current))
>> return( -ERESTARTSYS );
>> }
>>
>>
>
>acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
>from -kj looks more correct.
>
>From: Nishanth Aravamudan
>
>Use wait_event_interruptible() instead of the deprecated
>interruptible_sleep_on(). The sleep_on() call later in the same
>function is replaced with inline wait-queue code which achieves the
>same. This required adding a local wait-queue, though.
>
>--- a/drivers/block/acsi_slm.c
>+++ b/drivers/block/acsi_slm.c
>@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
> int device = iminor(node);
> int n, filled, w, h;
>
>- while( SLMState = PRINTING ||
>- (SLMState = FILLING && SLMBufOwner != device) ) {
>- interruptible_sleep_on( &slm_wait );
>- if (signal_pending(current))
>- return( -ERESTARTSYS );
>- }
>+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
>+ (SLMState != FILLING || SLMBufOwner = device)));
>+ if (signal_pending(current))
>+ return -ERESTARTSYS;
> if (SLMState = IDLE) {
> /* first data of page: get current page size */
> if (slm_get_pagesize( device, &w, &h ))
>
>
>
That does look more correct. Please ignore my patch.
Thanks!
--
Irwan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (5 preceding siblings ...)
2005-10-24 23:46 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace Irwan Djajadi
@ 2005-10-25 3:12 ` irwan.djajadi
2005-10-25 3:15 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace irwan.djajadi
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: irwan.djajadi @ 2005-10-25 3:12 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]
On Tue, Oct 25, 2005 at 01:23:20AM +0400, Alexey Dobriyan wrote:
> On Sat, Oct 22, 2005 at 09:55:25PM -0500, irwan.djajadi@iname.com wrote:
> > --- 2.6.14-rc5/drivers/block/swim3.c
> > +++ mod/drivers/block/swim3.c
> > @@ -800,7 +800,7 @@ static int grab_drive(struct floppy_stat
>
> save_flags(flags);
> cli();
> if (fs->state != idle) {
> ++fs->wanted;
> while (fs->state != available) {
> if (interruptible && signal_pending(current)) {
> > restore_flags(flags);
> > return -EINTR;
> > }
> > - interruptible_sleep_on(&fs->wait);
> > + wait_event_interruptible(fs->wait, fs->state == available);
>
> Correct me if I'm wrong, but level of bugginess remains the same:
> scheduling with interrupts disabled.
>
> > }
> > --fs->wanted;
> > }
>
Nope, you're right.. Thanks for catching that.. I didn't think you can sleep
while interrupt is disabled.. Wouldn't the kernel BUG_ON() that ?
Anyway, here's my next try. A word of warning though, I don't have PPC machine
here, so I can't even compile this change. Please let me know if there's
anymore problem with this.
Thanks
--
Irwan
Fix interruptible_sleep_on() with wait_event_interruptible() or wait_event(),
and fix waiting while interrupt is disabled.
Signed-off-by: Irwan Djajadi <irwan.djajadi@iname.com>
diff -pruNX 2.6.14-rc5/Documentation/dontdiff 2.6.14-rc5/drivers/block/swim3.c mod/drivers/block/swim3.c
--- 2.6.14-rc5/drivers/block/swim3.c 2005-10-22 00:06:17.000000000 -0500
+++ mod/drivers/block/swim3.c 2005-10-24 21:32:44.000000000 -0500
@@ -789,24 +789,29 @@ static int grab_drive(struct floppy_stat
int interruptible)
{
unsigned long flags;
+ int err = 0;
save_flags(flags);
cli();
- if (fs->state != idle) {
+ while ((fs->state != idle) && !err) {
++fs->wanted;
- while (fs->state != available) {
- if (interruptible && signal_pending(current)) {
- --fs->wanted;
- restore_flags(flags);
- return -EINTR;
- }
- interruptible_sleep_on(&fs->wait);
+ restore_flags(flags);
+ if (interruptible) {
+ err = wait_event_interruptible(fs->wait,
+ fs->state == available);
+ if (err)
+ err = -EINTR;
+ } else {
+ wait_event(fs->wait, fs->state == available);
}
+ save_flags(flags);
+ cli();
--fs->wanted;
}
- fs->state = state;
+ if (!err)
+ fs->state = state;
restore_flags(flags);
- return 0;
+ return err;
}
static void release_drive(struct floppy_state *fs)
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (6 preceding siblings ...)
2005-10-25 3:12 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim3.c: replace irwan.djajadi
@ 2005-10-25 3:15 ` irwan.djajadi
2005-10-25 4:05 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace Nishanth Aravamudan
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: irwan.djajadi @ 2005-10-25 3:15 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]
On Tue, Oct 25, 2005 at 01:25:21AM +0400, Alexey Dobriyan wrote:
> On Sat, Oct 22, 2005 at 09:56:31PM -0500, irwan.djajadi@iname.com wrote:
> > --- 2.6.14-rc5/drivers/block/swim_iop.c
> > +++ mod/drivers/block/swim_iop.c
> > @@ -436,7 +436,7 @@ static int grab_drive(struct floppy_stat
> > local_irq_restore(flags);
> > return -EINTR;
> > }
> > - interruptible_sleep_on(&fs->wait);
> > + wait_event_interruptible(fs->wait, fs->state == available);
> > }
> > --fs->wanted;
> > }
>
> Ditto. local_irq_save().
>
Yup, ditto as well.. :-)
Here's my next try. Thanks man
--
Irwan
Fix interruptible_sleep_on() with wait_event_interruptible() or wait_event(),
and fix waiting while interrupt is disabled.
Signed-off-by: Irwan Djajadi <irwan.djajadi@iname.com>
diff -pruNX 2.6.14-rc5/Documentation/dontdiff 2.6.14-rc5/drivers/block/swim_iop.c mod/drivers/block/swim_iop.c
--- 2.6.14-rc5/drivers/block/swim_iop.c 2005-10-22 00:06:17.000000000 -0500
+++ mod/drivers/block/swim_iop.c 2005-10-24 22:01:34.000000000 -0500
@@ -426,23 +426,27 @@ static int grab_drive(struct floppy_stat
int interruptible)
{
unsigned long flags;
+ int err = 0;
local_irq_save(flags);
- if (fs->state != idle) {
+ while ((fs->state != idle) && !err) {
++fs->wanted;
- while (fs->state != available) {
- if (interruptible && signal_pending(current)) {
- --fs->wanted;
- local_irq_restore(flags);
- return -EINTR;
- }
- interruptible_sleep_on(&fs->wait);
+ local_irq_restore(flags);
+ if (interruptible) {
+ err = wait_event_interruptible(fs->wait,
+ fs->state == available);
+ if (err)
+ err = -EINTR;
+ } else {
+ wait_event(fs->wait, fs->state == available);
}
+ local_irq_save(flags);
--fs->wanted;
}
- fs->state = state;
+ if (!err)
+ fs->state = state;
local_irq_restore(flags);
- return 0;
+ return err;
}
static void release_drive(struct floppy_state *fs)
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (7 preceding siblings ...)
2005-10-25 3:15 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/swim_iop.c: replace irwan.djajadi
@ 2005-10-25 4:05 ` Nishanth Aravamudan
2005-10-26 2:44 ` Irwan Djajadi
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Nishanth Aravamudan @ 2005-10-25 4:05 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2632 bytes --]
On 24.10.2005 [18:46:01 -0500], Irwan Djajadi wrote:
> Alexey Dobriyan wrote:
>
> >On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com wrote:
> >
> >
> >>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
> >>wait_event_interruptible()
> >>
> >>
> >
> >
> >
> >>--- 2.6.14-rc5/drivers/block/acsi_slm.c
> >>+++ mod/drivers/block/acsi_slm.c
> >>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
> >>
> >> while( SLMState == PRINTING ||
> >> (SLMState == FILLING && SLMBufOwner != device) ) {
> >>- interruptible_sleep_on( &slm_wait );
> >>+ wait_event_interruptible( slm_wait, SLMState==IDLE );
> >> if (signal_pending(current))
> >> return( -ERESTARTSYS );
> >> }
> >>
> >>
> >
> >acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
> >from -kj looks more correct.
> >
> >From: Nishanth Aravamudan
> >
> >Use wait_event_interruptible() instead of the deprecated
> >interruptible_sleep_on(). The sleep_on() call later in the same
> >function is replaced with inline wait-queue code which achieves the
> >same. This required adding a local wait-queue, though.
> >
> >--- a/drivers/block/acsi_slm.c
> >+++ b/drivers/block/acsi_slm.c
> >@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
> > int device = iminor(node);
> > int n, filled, w, h;
> >
> >- while( SLMState == PRINTING ||
> >- (SLMState == FILLING && SLMBufOwner != device) ) {
> >- interruptible_sleep_on( &slm_wait );
> >- if (signal_pending(current))
> >- return( -ERESTARTSYS );
> >- }
> >+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
> >+ (SLMState != FILLING || SLMBufOwner ==
> >device)));
> >+ if (signal_pending(current))
> >+ return -ERESTARTSYS;
> > if (SLMState == IDLE) {
> > /* first data of page: get current page size */
> > if (slm_get_pagesize( device, &w, &h ))
> >
> >
> >
> That does look more correct. Please ignore my patch.
> Thanks!
@Irwan:
Have you tested any of the patches you are sending? I stopped replacing
the *sleep_on*() family of functions once I was outside of the domain of
trivial substitution.
You can't simply s/interruptible_sleep_on/wait_event_interruptible/ as
in many places the sleep_on() caller is not a loop, but one-time sleep!
I have noticed at least one case of this in the patches you are sending
out, but will try to make a more complete analysis soon.
@Alexey:
Be very careful pulling in these patches. I don't think any of the
remaining *sleep_on*() replacements are easy or trivial (at least the
ones w/o patches already in -KJ). I'll try to take a look more in depth
soon.
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (8 preceding siblings ...)
2005-10-25 4:05 ` [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace Nishanth Aravamudan
@ 2005-10-26 2:44 ` Irwan Djajadi
2005-10-26 6:04 ` Nishanth Aravamudan
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Irwan Djajadi @ 2005-10-26 2:44 UTC (permalink / raw)
To: kernel-janitors
Nishanth Aravamudan wrote:
> On 24.10.2005 [18:46:01 -0500], Irwan Djajadi wrote:
>
>>Alexey Dobriyan wrote:
>>
>>
>>>On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com wrote:
>>>
>>>
>>>
>>>>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
>>>>wait_event_interruptible()
>>>>
>>>>
>>>
>>>
>>>
>>>>--- 2.6.14-rc5/drivers/block/acsi_slm.c
>>>>+++ mod/drivers/block/acsi_slm.c
>>>>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
>>>>
>>>> while( SLMState = PRINTING ||
>>>> (SLMState = FILLING && SLMBufOwner != device) ) {
>>>>- interruptible_sleep_on( &slm_wait );
>>>>+ wait_event_interruptible( slm_wait, SLMState=IDLE );
>>>> if (signal_pending(current))
>>>> return( -ERESTARTSYS );
>>>> }
>>>>
>>>>
>>>
>>>acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
>>
>>>from -kj looks more correct.
>>
>>>From: Nishanth Aravamudan
>>>
>>>Use wait_event_interruptible() instead of the deprecated
>>>interruptible_sleep_on(). The sleep_on() call later in the same
>>>function is replaced with inline wait-queue code which achieves the
>>>same. This required adding a local wait-queue, though.
>>>
>>>--- a/drivers/block/acsi_slm.c
>>>+++ b/drivers/block/acsi_slm.c
>>>@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
>>> int device = iminor(node);
>>> int n, filled, w, h;
>>>
>>>- while( SLMState = PRINTING ||
>>>- (SLMState = FILLING && SLMBufOwner != device) ) {
>>>- interruptible_sleep_on( &slm_wait );
>>>- if (signal_pending(current))
>>>- return( -ERESTARTSYS );
>>>- }
>>>+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
>>>+ (SLMState != FILLING || SLMBufOwner =
>>>device)));
>>>+ if (signal_pending(current))
>>>+ return -ERESTARTSYS;
>>> if (SLMState = IDLE) {
>>> /* first data of page: get current page size */
>>> if (slm_get_pagesize( device, &w, &h ))
>>>
>>>
>>>
>>
>>That does look more correct. Please ignore my patch.
>>Thanks!
>
>
> @Irwan:
>
> Have you tested any of the patches you are sending? I stopped replacing
> the *sleep_on*() family of functions once I was outside of the domain of
> trivial substitution.
>
> You can't simply s/interruptible_sleep_on/wait_event_interruptible/ as
> in many places the sleep_on() caller is not a loop, but one-time sleep!
> I have noticed at least one case of this in the patches you are sending
> out, but will try to make a more complete analysis soon.
>
> @Alexey:
>
> Be very careful pulling in these patches. I don't think any of the
> remaining *sleep_on*() replacements are easy or trivial (at least the
> ones w/o patches already in -KJ). I'll try to take a look more in depth
> soon.
>
> Thanks,
> Nish
>
Hi Nish,
I only compile-test most of my patch over the weekend.. There are some
that I didn't even do compile test on because it's for a different
platform (I'm on x86). But yes, I am aware that some of *sleep_on*()
family function fixes are not trivial. I only changed ones that I
thought trivial. When the end-wait-condition is not clear to me, I left
it alone, and I went to the next.
Maybe I'm missing something here, but I don't really see why
interruptible_sleep_on() or wait_event_interruptible() is not a straight
substitution when the caller is not in a loop..
interruptible_sleep_on() does not have a loop inside, and it's a single
incantation of wait. When it wakes up, it could be because of a signal
or end-condition.
I see that wait_event_interruptible() has a check outside and a loop
inside to ensure that when it returns it's because of a signal or
end-condition. (So it's a more correct version of interruptible_sleep_on())
The semantic looks the same to me, so why can't it be a straight
substitution when the end-wait-condition is straightforward?
I don't intend to inject bugs, so I appreciate y'all's review of my
patches, and let me know/smack me when I do dumb things.
Thanks!
--
Irwan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (9 preceding siblings ...)
2005-10-26 2:44 ` Irwan Djajadi
@ 2005-10-26 6:04 ` Nishanth Aravamudan
2005-10-27 4:22 ` Irwan Djajadi
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Nishanth Aravamudan @ 2005-10-26 6:04 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 5339 bytes --]
On 25.10.2005 [21:44:09 -0500], Irwan Djajadi wrote:
> Nishanth Aravamudan wrote:
> >On 24.10.2005 [18:46:01 -0500], Irwan Djajadi wrote:
> >
> >>Alexey Dobriyan wrote:
> >>
> >>
> >>>On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com wrote:
> >>>
> >>>
> >>>
> >>>>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
> >>>>wait_event_interruptible()
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>>>--- 2.6.14-rc5/drivers/block/acsi_slm.c
> >>>>+++ mod/drivers/block/acsi_slm.c
> >>>>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
> >>>>
> >>>> while( SLMState == PRINTING ||
> >>>> (SLMState == FILLING && SLMBufOwner != device) ) {
> >>>>- interruptible_sleep_on( &slm_wait );
> >>>>+ wait_event_interruptible( slm_wait, SLMState==IDLE );
> >>>> if (signal_pending(current))
> >>>> return( -ERESTARTSYS );
> >>>> }
> >>>>
> >>>>
> >>>
> >>>acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
> >>
> >>>from -kj looks more correct.
> >>
> >>>From: Nishanth Aravamudan
> >>>
> >>>Use wait_event_interruptible() instead of the deprecated
> >>>interruptible_sleep_on(). The sleep_on() call later in the same
> >>>function is replaced with inline wait-queue code which achieves the
> >>>same. This required adding a local wait-queue, though.
> >>>
> >>>--- a/drivers/block/acsi_slm.c
> >>>+++ b/drivers/block/acsi_slm.c
> >>>@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
> >>> int device = iminor(node);
> >>> int n, filled, w, h;
> >>>
> >>>- while( SLMState == PRINTING ||
> >>>- (SLMState == FILLING && SLMBufOwner != device) ) {
> >>>- interruptible_sleep_on( &slm_wait );
> >>>- if (signal_pending(current))
> >>>- return( -ERESTARTSYS );
> >>>- }
> >>>+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
> >>>+ (SLMState != FILLING || SLMBufOwner ==
> >>>device)));
> >>>+ if (signal_pending(current))
> >>>+ return -ERESTARTSYS;
> >>> if (SLMState == IDLE) {
> >>> /* first data of page: get current page size */
> >>> if (slm_get_pagesize( device, &w, &h ))
> >>>
> >>>
> >>>
> >>
> >>That does look more correct. Please ignore my patch.
> >>Thanks!
> >
> >
> >@Irwan:
> >
> >Have you tested any of the patches you are sending? I stopped replacing
> >the *sleep_on*() family of functions once I was outside of the domain of
> >trivial substitution.
> >
> >You can't simply s/interruptible_sleep_on/wait_event_interruptible/ as
> >in many places the sleep_on() caller is not a loop, but one-time sleep!
> >I have noticed at least one case of this in the patches you are sending
> >out, but will try to make a more complete analysis soon.
> >
> >@Alexey:
> >
> >Be very careful pulling in these patches. I don't think any of the
> >remaining *sleep_on*() replacements are easy or trivial (at least the
> >ones w/o patches already in -KJ). I'll try to take a look more in depth
> >soon.
> >
> >Thanks,
> >Nish
> >
>
> Hi Nish,
>
> I only compile-test most of my patch over the weekend.. There are some
> that I didn't even do compile test on because it's for a different
> platform (I'm on x86). But yes, I am aware that some of *sleep_on*()
> family function fixes are not trivial. I only changed ones that I
> thought trivial. When the end-wait-condition is not clear to me, I left
> it alone, and I went to the next.
It's not just the wait-condition, it's also the surrounding code
(including how the wait-queue gets woken).
[ On a sidenote: it took me a while to drive this through my own thick
skull, but the *most* important thing to do with KJ patches is
compile-test. If you don't have the arch at hand, get a cross-compiler,
I know there are guides for setting one up around. ]
> Maybe I'm missing something here, but I don't really see why
> interruptible_sleep_on() or wait_event_interruptible() is not a straight
> substitution when the caller is not in a loop..
First of all, there's the straightforward semantic difference between
sleeping on a wait queue and waiting for an event! Second, because the
caller is not in a loop, you are introducing a *new* loop by using
wait_event*(). That may not be desired. Depends on the condition you
put in, as well.
> interruptible_sleep_on() does not have a loop inside, and it's a single
> incantation of wait. When it wakes up, it could be because of a signal
> or end-condition.
Or someone manually waking up the wait-queue. There is no "end
condition" with sleep_on().
> I see that wait_event_interruptible() has a check outside and a loop
> inside to ensure that when it returns it's because of a signal or
> end-condition. (So it's a more correct version of interruptible_sleep_on())
Right, so whereas with sleep_on, waking-up the wait-queue is sufficient
to terminate the sleep, it is *not* for wait_event() -- either the event
*must* be satisfied or a signal *must* have arrived for wait_event() to
return.
> The semantic looks the same to me, so why can't it be a straight
> substitution when the end-wait-condition is straightforward?
>
> I don't intend to inject bugs, so I appreciate y'all's review of my
> patches, and let me know/smack me when I do dumb things.
I appreciate the effort. Getting rid of sleep_on() & co. would be a nice
benefit IMO (or getting it closer to the point where we can mark them
deprecated).
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (10 preceding siblings ...)
2005-10-26 6:04 ` Nishanth Aravamudan
@ 2005-10-27 4:22 ` Irwan Djajadi
2005-10-27 19:08 ` Nishanth Aravamudan
2005-10-28 3:44 ` Irwan Djajadi
13 siblings, 0 replies; 15+ messages in thread
From: Irwan Djajadi @ 2005-10-27 4:22 UTC (permalink / raw)
To: kernel-janitors
Nishanth Aravamudan wrote:
> On 25.10.2005 [21:44:09 -0500], Irwan Djajadi wrote:
>
>>Nishanth Aravamudan wrote:
>>
>>>On 24.10.2005 [18:46:01 -0500], Irwan Djajadi wrote:
>>>
>>>
>>>>Alexey Dobriyan wrote:
>>>>
>>>>
>>>>
>>>>>On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
>>>>>>wait_event_interruptible()
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>--- 2.6.14-rc5/drivers/block/acsi_slm.c
>>>>>>+++ mod/drivers/block/acsi_slm.c
>>>>>>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
>>>>>>
>>>>>> while( SLMState = PRINTING ||
>>>>>> (SLMState = FILLING && SLMBufOwner != device) ) {
>>>>>>- interruptible_sleep_on( &slm_wait );
>>>>>>+ wait_event_interruptible( slm_wait, SLMState=IDLE );
>>>>>> if (signal_pending(current))
>>>>>> return( -ERESTARTSYS );
>>>>>> }
>>>>>>
>>>>>>
>>>>>
>>>>>acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
>>>>
>>>>>from -kj looks more correct.
>>>>
>>>>
>>>>>From: Nishanth Aravamudan
>>>>>
>>>>>Use wait_event_interruptible() instead of the deprecated
>>>>>interruptible_sleep_on(). The sleep_on() call later in the same
>>>>>function is replaced with inline wait-queue code which achieves the
>>>>>same. This required adding a local wait-queue, though.
>>>>>
>>>>>--- a/drivers/block/acsi_slm.c
>>>>>+++ b/drivers/block/acsi_slm.c
>>>>>@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
>>>>> int device = iminor(node);
>>>>> int n, filled, w, h;
>>>>>
>>>>>- while( SLMState = PRINTING ||
>>>>>- (SLMState = FILLING && SLMBufOwner != device) ) {
>>>>>- interruptible_sleep_on( &slm_wait );
>>>>>- if (signal_pending(current))
>>>>>- return( -ERESTARTSYS );
>>>>>- }
>>>>>+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
>>>>>+ (SLMState != FILLING || SLMBufOwner =
>>>>>device)));
>>>>>+ if (signal_pending(current))
>>>>>+ return -ERESTARTSYS;
>>>>> if (SLMState = IDLE) {
>>>>> /* first data of page: get current page size */
>>>>> if (slm_get_pagesize( device, &w, &h ))
>>>>>
>>>>>
>>>>>
>>>>
>>>>That does look more correct. Please ignore my patch.
>>>>Thanks!
>>>
>>>
>>>@Irwan:
>>>
>>>Have you tested any of the patches you are sending? I stopped replacing
>>>the *sleep_on*() family of functions once I was outside of the domain of
>>>trivial substitution.
>>>
>>>You can't simply s/interruptible_sleep_on/wait_event_interruptible/ as
>>>in many places the sleep_on() caller is not a loop, but one-time sleep!
>>>I have noticed at least one case of this in the patches you are sending
>>>out, but will try to make a more complete analysis soon.
>>>
>>>@Alexey:
>>>
>>>Be very careful pulling in these patches. I don't think any of the
>>>remaining *sleep_on*() replacements are easy or trivial (at least the
>>>ones w/o patches already in -KJ). I'll try to take a look more in depth
>>>soon.
>>>
>>>Thanks,
>>>Nish
>>>
>>
>>Hi Nish,
>>
>>I only compile-test most of my patch over the weekend.. There are some
>>that I didn't even do compile test on because it's for a different
>>platform (I'm on x86). But yes, I am aware that some of *sleep_on*()
>>family function fixes are not trivial. I only changed ones that I
>>thought trivial. When the end-wait-condition is not clear to me, I left
>>it alone, and I went to the next.
>
>
> It's not just the wait-condition, it's also the surrounding code
> (including how the wait-queue gets woken).
>
> [ On a sidenote: it took me a while to drive this through my own thick
> skull, but the *most* important thing to do with KJ patches is
> compile-test. If you don't have the arch at hand, get a cross-compiler,
> I know there are guides for setting one up around. ]
>
>
>>Maybe I'm missing something here, but I don't really see why
>>interruptible_sleep_on() or wait_event_interruptible() is not a straight
>>substitution when the caller is not in a loop..
>
>
> First of all, there's the straightforward semantic difference between
> sleeping on a wait queue and waiting for an event! Second, because the
> caller is not in a loop, you are introducing a *new* loop by using
> wait_event*(). That may not be desired. Depends on the condition you
> put in, as well.
>
>
>>interruptible_sleep_on() does not have a loop inside, and it's a single
>>incantation of wait. When it wakes up, it could be because of a signal
>>or end-condition.
>
>
> Or someone manually waking up the wait-queue. There is no "end
> condition" with sleep_on().
>
>
>>I see that wait_event_interruptible() has a check outside and a loop
>>inside to ensure that when it returns it's because of a signal or
>>end-condition. (So it's a more correct version of interruptible_sleep_on())
>
>
> Right, so whereas with sleep_on, waking-up the wait-queue is sufficient
> to terminate the sleep, it is *not* for wait_event() -- either the event
> *must* be satisfied or a signal *must* have arrived for wait_event() to
> return.
>
>
>>The semantic looks the same to me, so why can't it be a straight
>>substitution when the end-wait-condition is straightforward?
>>
>>I don't intend to inject bugs, so I appreciate y'all's review of my
>>patches, and let me know/smack me when I do dumb things.
>
>
> I appreciate the effort. Getting rid of sleep_on() & co. would be a nice
> benefit IMO (or getting it closer to the point where we can mark them
> deprecated).
>
> Thanks,
> Nish
>
OK, you're right, the semantic is different..
I've gone back and reviewed my changes, and I think the code before was
buggy because it should've used the end-condition loop surrounding the
interruptible_sleep_on, so I think my changes are OK.. IMHO..
Anyway, I think there is a bigger problem even with
wait_event_interruptible.. When it returns, there is only a guarantee
that the end-condition happened at the time, but there may not be a
guarantee that the end-condition is still happening on the current time.
I noticed that some drivers uses this synchronization construct to do
resource management, and I think this is dangerous..
For example: mcdx.c uses it to do locking, swim3.c and swim_iop.c also
use it for doing "locking" (it waits till fs->state = available, but by
the time we wake up, fs->state might not be available anymore)..
The rest of the changes mostly have something to do with ASYNC_CLOSING
stuff with serial port, and using wait_event_interruptible() is probably
OK here, since hopefully we can only close the serial port once.. or
something.. :-)
OK, well, lemme know what you think, or maybe I'm just smoking crack
Thanks guys,
--
Irwan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (11 preceding siblings ...)
2005-10-27 4:22 ` Irwan Djajadi
@ 2005-10-27 19:08 ` Nishanth Aravamudan
2005-10-28 3:44 ` Irwan Djajadi
13 siblings, 0 replies; 15+ messages in thread
From: Nishanth Aravamudan @ 2005-10-27 19:08 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 7486 bytes --]
On 26.10.2005 [23:22:31 -0500], Irwan Djajadi wrote:
> Nishanth Aravamudan wrote:
> >On 25.10.2005 [21:44:09 -0500], Irwan Djajadi wrote:
> >
> >>Nishanth Aravamudan wrote:
> >>
> >>>On 24.10.2005 [18:46:01 -0500], Irwan Djajadi wrote:
> >>>
> >>>
> >>>>Alexey Dobriyan wrote:
> >>>>
> >>>>
> >>>>
> >>>>>On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com
> >>>>>wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
> >>>>>>wait_event_interruptible()
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>--- 2.6.14-rc5/drivers/block/acsi_slm.c
> >>>>>>+++ mod/drivers/block/acsi_slm.c
> >>>>>>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
> >>>>>>
> >>>>>> while( SLMState == PRINTING ||
> >>>>>> (SLMState == FILLING && SLMBufOwner != device) ) {
> >>>>>>- interruptible_sleep_on( &slm_wait );
> >>>>>>+ wait_event_interruptible( slm_wait, SLMState==IDLE );
> >>>>>> if (signal_pending(current))
> >>>>>> return( -ERESTARTSYS );
> >>>>>> }
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
> >>>>
> >>>>>from -kj looks more correct.
> >>>>
> >>>>
> >>>>>From: Nishanth Aravamudan
> >>>>>
> >>>>>Use wait_event_interruptible() instead of the deprecated
> >>>>>interruptible_sleep_on(). The sleep_on() call later in the same
> >>>>>function is replaced with inline wait-queue code which achieves the
> >>>>>same. This required adding a local wait-queue, though.
> >>>>>
> >>>>>--- a/drivers/block/acsi_slm.c
> >>>>>+++ b/drivers/block/acsi_slm.c
> >>>>>@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
> >>>>> int device = iminor(node);
> >>>>> int n, filled, w, h;
> >>>>>
> >>>>>- while( SLMState == PRINTING ||
> >>>>>- (SLMState == FILLING && SLMBufOwner != device) ) {
> >>>>>- interruptible_sleep_on( &slm_wait );
> >>>>>- if (signal_pending(current))
> >>>>>- return( -ERESTARTSYS );
> >>>>>- }
> >>>>>+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
> >>>>>+ (SLMState != FILLING || SLMBufOwner ==
> >>>>>device)));
> >>>>>+ if (signal_pending(current))
> >>>>>+ return -ERESTARTSYS;
> >>>>> if (SLMState == IDLE) {
> >>>>> /* first data of page: get current page size */
> >>>>> if (slm_get_pagesize( device, &w, &h ))
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>That does look more correct. Please ignore my patch.
> >>>>Thanks!
> >>>
> >>>
> >>>@Irwan:
> >>>
> >>>Have you tested any of the patches you are sending? I stopped replacing
> >>>the *sleep_on*() family of functions once I was outside of the domain of
> >>>trivial substitution.
> >>>
> >>>You can't simply s/interruptible_sleep_on/wait_event_interruptible/ as
> >>>in many places the sleep_on() caller is not a loop, but one-time sleep!
> >>>I have noticed at least one case of this in the patches you are sending
> >>>out, but will try to make a more complete analysis soon.
> >>>
> >>>@Alexey:
> >>>
> >>>Be very careful pulling in these patches. I don't think any of the
> >>>remaining *sleep_on*() replacements are easy or trivial (at least the
> >>>ones w/o patches already in -KJ). I'll try to take a look more in depth
> >>>soon.
> >>>
> >>>Thanks,
> >>>Nish
> >>>
> >>
> >>Hi Nish,
> >>
> >>I only compile-test most of my patch over the weekend.. There are some
> >>that I didn't even do compile test on because it's for a different
> >>platform (I'm on x86). But yes, I am aware that some of *sleep_on*()
> >>family function fixes are not trivial. I only changed ones that I
> >>thought trivial. When the end-wait-condition is not clear to me, I left
> >>it alone, and I went to the next.
> >
> >
> >It's not just the wait-condition, it's also the surrounding code
> >(including how the wait-queue gets woken).
> >
> >[ On a sidenote: it took me a while to drive this through my own thick
> >skull, but the *most* important thing to do with KJ patches is
> >compile-test. If you don't have the arch at hand, get a cross-compiler,
> >I know there are guides for setting one up around. ]
> >
> >
> >>Maybe I'm missing something here, but I don't really see why
> >>interruptible_sleep_on() or wait_event_interruptible() is not a straight
> >>substitution when the caller is not in a loop..
> >
> >
> >First of all, there's the straightforward semantic difference between
> >sleeping on a wait queue and waiting for an event! Second, because the
> >caller is not in a loop, you are introducing a *new* loop by using
> >wait_event*(). That may not be desired. Depends on the condition you
> >put in, as well.
> >
> >
> >>interruptible_sleep_on() does not have a loop inside, and it's a single
> >>incantation of wait. When it wakes up, it could be because of a signal
> >>or end-condition.
> >
> >
> >Or someone manually waking up the wait-queue. There is no "end
> >condition" with sleep_on().
> >
> >
> >>I see that wait_event_interruptible() has a check outside and a loop
> >>inside to ensure that when it returns it's because of a signal or
> >>end-condition. (So it's a more correct version of
> >>interruptible_sleep_on())
> >
> >
> >Right, so whereas with sleep_on, waking-up the wait-queue is sufficient
> >to terminate the sleep, it is *not* for wait_event() -- either the event
> >*must* be satisfied or a signal *must* have arrived for wait_event() to
> >return.
> >
> >
> >>The semantic looks the same to me, so why can't it be a straight
> >>substitution when the end-wait-condition is straightforward?
> >>
> >>I don't intend to inject bugs, so I appreciate y'all's review of my
> >>patches, and let me know/smack me when I do dumb things.
> >
> >
> >I appreciate the effort. Getting rid of sleep_on() & co. would be a nice
> >benefit IMO (or getting it closer to the point where we can mark them
> >deprecated).
> >
> >Thanks,
> >Nish
> >
>
> OK, you're right, the semantic is different..
>
> I've gone back and reviewed my changes, and I think the code before was
> buggy because it should've used the end-condition loop surrounding the
> interruptible_sleep_on, so I think my changes are OK.. IMHO..
>
> Anyway, I think there is a bigger problem even with
> wait_event_interruptible.. When it returns, there is only a guarantee
> that the end-condition happened at the time, but there may not be a
> guarantee that the end-condition is still happening on the current time.
> I noticed that some drivers uses this synchronization construct to do
> resource management, and I think this is dangerous..
That would be buggy code, not broken wait_event(). wait_event() waits
for an event and returns when the event is true (or a timeout or a
signal occurs, depending on the actual call). There is never any
gurantee said even is still true.
> For example: mcdx.c uses it to do locking, swim3.c and swim_iop.c also
> use it for doing "locking" (it waits till fs->state == available, but by
> the time we wake up, fs->state might not be available anymore)..
> The rest of the changes mostly have something to do with ASYNC_CLOSING
> stuff with serial port, and using wait_event_interruptible() is probably
> OK here, since hopefully we can only close the serial port once.. or
> something.. :-)
It also depends on the codes design. Maybe there is a guarantee, e.g.
that fs->state being made available means that it won't change until our
sleeping task is done. I don't know without looking at the code more
in-depth (which I unfortunately have the time for.
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace
2005-10-23 2:53 [KJ] [PATCH 2.6.14-rc5 1/1] drivers/block/acsi_slm.c: replace irwan.djajadi
` (12 preceding siblings ...)
2005-10-27 19:08 ` Nishanth Aravamudan
@ 2005-10-28 3:44 ` Irwan Djajadi
13 siblings, 0 replies; 15+ messages in thread
From: Irwan Djajadi @ 2005-10-28 3:44 UTC (permalink / raw)
To: kernel-janitors
Nishanth Aravamudan wrote:
> On 26.10.2005 [23:22:31 -0500], Irwan Djajadi wrote:
>
>>Nishanth Aravamudan wrote:
>>
>>>On 25.10.2005 [21:44:09 -0500], Irwan Djajadi wrote:
>>>
>>>
>>>>Nishanth Aravamudan wrote:
>>>>
>>>>
>>>>>On 24.10.2005 [18:46:01 -0500], Irwan Djajadi wrote:
>>>>>
>>>>>
>>>>>
>>>>>>Alexey Dobriyan wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>On Sat, Oct 22, 2005 at 09:53:51PM -0500, irwan.djajadi@iname.com
>>>>>>>wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>drivers/block/acsi_slm.c: replace interruptible_sleep_on() with
>>>>>>>>wait_event_interruptible()
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>--- 2.6.14-rc5/drivers/block/acsi_slm.c
>>>>>>>>+++ mod/drivers/block/acsi_slm.c
>>>>>>>>@@ -627,7 +627,7 @@ static ssize_t slm_write( struct file *f
>>>>>>>>
>>>>>>>> while( SLMState = PRINTING ||
>>>>>>>> (SLMState = FILLING && SLMBufOwner != device) ) {
>>>>>>>>- interruptible_sleep_on( &slm_wait );
>>>>>>>>+ wait_event_interruptible( slm_wait, SLMState=IDLE );
>>>>>>>> if (signal_pending(current))
>>>>>>>> return( -ERESTARTSYS );
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>acsi_slm_replace_interruptible_sleep_on_with_wait_event_interruptible.patch
>>>>>>
>>>>>>>from -kj looks more correct.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>From: Nishanth Aravamudan
>>>>>>>
>>>>>>>Use wait_event_interruptible() instead of the deprecated
>>>>>>>interruptible_sleep_on(). The sleep_on() call later in the same
>>>>>>>function is replaced with inline wait-queue code which achieves the
>>>>>>>same. This required adding a local wait-queue, though.
>>>>>>>
>>>>>>>--- a/drivers/block/acsi_slm.c
>>>>>>>+++ b/drivers/block/acsi_slm.c
>>>>>>>@@ -625,12 +626,10 @@ static ssize_t slm_write( struct file *f
>>>>>>> int device = iminor(node);
>>>>>>> int n, filled, w, h;
>>>>>>>
>>>>>>>- while( SLMState = PRINTING ||
>>>>>>>- (SLMState = FILLING && SLMBufOwner != device) ) {
>>>>>>>- interruptible_sleep_on( &slm_wait );
>>>>>>>- if (signal_pending(current))
>>>>>>>- return( -ERESTARTSYS );
>>>>>>>- }
>>>>>>>+ wait_event_interruptible(slm_wait, (SLMState != PRINTING &&
>>>>>>>+ (SLMState != FILLING || SLMBufOwner =
>>>>>>>device)));
>>>>>>>+ if (signal_pending(current))
>>>>>>>+ return -ERESTARTSYS;
>>>>>>> if (SLMState = IDLE) {
>>>>>>> /* first data of page: get current page size */
>>>>>>> if (slm_get_pagesize( device, &w, &h ))
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>That does look more correct. Please ignore my patch.
>>>>>>Thanks!
>>>>>
>>>>>
>>>>>@Irwan:
>>>>>
>>>>>Have you tested any of the patches you are sending? I stopped replacing
>>>>>the *sleep_on*() family of functions once I was outside of the domain of
>>>>>trivial substitution.
>>>>>
>>>>>You can't simply s/interruptible_sleep_on/wait_event_interruptible/ as
>>>>>in many places the sleep_on() caller is not a loop, but one-time sleep!
>>>>>I have noticed at least one case of this in the patches you are sending
>>>>>out, but will try to make a more complete analysis soon.
>>>>>
>>>>>@Alexey:
>>>>>
>>>>>Be very careful pulling in these patches. I don't think any of the
>>>>>remaining *sleep_on*() replacements are easy or trivial (at least the
>>>>>ones w/o patches already in -KJ). I'll try to take a look more in depth
>>>>>soon.
>>>>>
>>>>>Thanks,
>>>>>Nish
>>>>>
>>>>
>>>>Hi Nish,
>>>>
>>>>I only compile-test most of my patch over the weekend.. There are some
>>>>that I didn't even do compile test on because it's for a different
>>>>platform (I'm on x86). But yes, I am aware that some of *sleep_on*()
>>>>family function fixes are not trivial. I only changed ones that I
>>>>thought trivial. When the end-wait-condition is not clear to me, I left
>>>>it alone, and I went to the next.
>>>
>>>
>>>It's not just the wait-condition, it's also the surrounding code
>>>(including how the wait-queue gets woken).
>>>
>>>[ On a sidenote: it took me a while to drive this through my own thick
>>>skull, but the *most* important thing to do with KJ patches is
>>>compile-test. If you don't have the arch at hand, get a cross-compiler,
>>>I know there are guides for setting one up around. ]
>>>
>>>
>>>
>>>>Maybe I'm missing something here, but I don't really see why
>>>>interruptible_sleep_on() or wait_event_interruptible() is not a straight
>>>>substitution when the caller is not in a loop..
>>>
>>>
>>>First of all, there's the straightforward semantic difference between
>>>sleeping on a wait queue and waiting for an event! Second, because the
>>>caller is not in a loop, you are introducing a *new* loop by using
>>>wait_event*(). That may not be desired. Depends on the condition you
>>>put in, as well.
>>>
>>>
>>>
>>>>interruptible_sleep_on() does not have a loop inside, and it's a single
>>>>incantation of wait. When it wakes up, it could be because of a signal
>>>>or end-condition.
>>>
>>>
>>>Or someone manually waking up the wait-queue. There is no "end
>>>condition" with sleep_on().
>>>
>>>
>>>
>>>>I see that wait_event_interruptible() has a check outside and a loop
>>>>inside to ensure that when it returns it's because of a signal or
>>>>end-condition. (So it's a more correct version of
>>>>interruptible_sleep_on())
>>>
>>>
>>>Right, so whereas with sleep_on, waking-up the wait-queue is sufficient
>>>to terminate the sleep, it is *not* for wait_event() -- either the event
>>>*must* be satisfied or a signal *must* have arrived for wait_event() to
>>>return.
>>>
>>>
>>>
>>>>The semantic looks the same to me, so why can't it be a straight
>>>>substitution when the end-wait-condition is straightforward?
>>>>
>>>>I don't intend to inject bugs, so I appreciate y'all's review of my
>>>>patches, and let me know/smack me when I do dumb things.
>>>
>>>
>>>I appreciate the effort. Getting rid of sleep_on() & co. would be a nice
>>>benefit IMO (or getting it closer to the point where we can mark them
>>>deprecated).
>>>
>>>Thanks,
>>>Nish
>>>
>>
>>OK, you're right, the semantic is different..
>>
>>I've gone back and reviewed my changes, and I think the code before was
>>buggy because it should've used the end-condition loop surrounding the
>>interruptible_sleep_on, so I think my changes are OK.. IMHO..
>>
>>Anyway, I think there is a bigger problem even with
>>wait_event_interruptible.. When it returns, there is only a guarantee
>>that the end-condition happened at the time, but there may not be a
>>guarantee that the end-condition is still happening on the current time.
>> I noticed that some drivers uses this synchronization construct to do
>>resource management, and I think this is dangerous..
>
>
> That would be buggy code, not broken wait_event(). wait_event() waits
> for an event and returns when the event is true (or a timeout or a
> signal occurs, depending on the actual call). There is never any
> gurantee said even is still true.
>
>
>>For example: mcdx.c uses it to do locking, swim3.c and swim_iop.c also
>>use it for doing "locking" (it waits till fs->state = available, but by
>>the time we wake up, fs->state might not be available anymore)..
>>The rest of the changes mostly have something to do with ASYNC_CLOSING
>>stuff with serial port, and using wait_event_interruptible() is probably
>>OK here, since hopefully we can only close the serial port once.. or
>>something.. :-)
>
>
> It also depends on the codes design. Maybe there is a guarantee, e.g.
> that fs->state being made available means that it won't change until our
> sleeping task is done. I don't know without looking at the code more
> in-depth (which I unfortunately have the time for.
>
> Thanks,
> Nish
>
Yes, I agree.. That was what I was trying to say, that those drivers
maybe using the wrong synchronization construct.. and not really
wait_event*() problem.
:-) Yeah, I chose to just replace sleep_on() functions with the blessed
ones, and not trying to fix the buggy drivers. At least with my patch,
they're "better" in terms of using the non-deprecated funcs, but
bugginess-wise, they'll be about the same... oh well..
Thanks man
--
Irwan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 15+ messages in thread