2005-03-06 10:32:18

by Domen Puncer

[permalink] [raw]
Subject: [patch 2/6] 12/34: cdrom/cdu31a: replace interruptible_sleep_on() with wait_event_interruptible()



Use wait_event_interruptible() instead of the deprecated
interruptible_sleep_on(). The patch is straight-forward as the macros should
result in the same execution. Patch is compile-tested (still throws out warnings
regarding {save,restore}_flags()).

Signed-off-by: Nishanth Aravamudan <[email protected]>
Signed-off-by: Domen Puncer <[email protected]>
---


kj-domen/drivers/cdrom/cdu31a.c | 40 +++++++++++++++++-----------------------
1 files changed, 17 insertions(+), 23 deletions(-)

diff -puN drivers/cdrom/cdu31a.c~int_sleep_on-drivers_cdrom_cdu31a drivers/cdrom/cdu31a.c
--- kj/drivers/cdrom/cdu31a.c~int_sleep_on-drivers_cdrom_cdu31a 2005-03-05 16:11:33.000000000 +0100
+++ kj-domen/drivers/cdrom/cdu31a.c 2005-03-05 16:11:33.000000000 +0100
@@ -166,6 +166,7 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/wait.h>

#include <asm/system.h>
#include <asm/io.h>
@@ -865,15 +866,13 @@ do_sony_cd_cmd(unsigned char cmd,
save_flags(flags);
cli();
if (current != has_cd_task) { /* Allow recursive calls to this routine */
- while (sony_inuse) {
- interruptible_sleep_on(&sony_wait);
- if (signal_pending(current)) {
- result_buffer[0] = 0x20;
- result_buffer[1] = SONY_SIGNAL_OP_ERR;
- *result_size = 2;
- restore_flags(flags);
- return;
- }
+ wait_event_interruptible(sony_wait, !sony_inuse);
+ if (signal_pending(current)) {
+ result_buffer[0] = 0x20;
+ result_buffer[1] = SONY_SIGNAL_OP_ERR;
+ *result_size = 2;
+ restore_flags(flags);
+ return;
}
sony_inuse = 1;
has_cd_task = current;
@@ -1372,16 +1371,13 @@ static void do_cdu31a_request(request_qu
*/
save_flags(flags);
cli();
- while (sony_inuse) {
- interruptible_sleep_on(&sony_wait);
- if (signal_pending(current)) {
- restore_flags(flags);
+ wait_event_interruptible(sony_wait, !sony_inuse);
+ if (signal_pending(current)) {
+ restore_flags(flags);
#if DEBUG
- printk("Leaving do_cdu31a_request at %d\n",
- __LINE__);
+ printk("Leaving do_cdu31a_request at %d\n", __LINE__);
#endif
- return;
- }
+ return;
}
sony_inuse = 1;
has_cd_task = current;
@@ -2326,12 +2322,10 @@ static int read_audio(struct cdrom_read_
*/
save_flags(flags);
cli();
- while (sony_inuse) {
- interruptible_sleep_on(&sony_wait);
- if (signal_pending(current)) {
- restore_flags(flags);
- return -EAGAIN;
- }
+ wait_event_interruptible(sony_wait, sony_inuse);
+ if (signal_pending(current)) {
+ restore_flags(flags);
+ return -EAGAIN;
}
sony_inuse = 1;
has_cd_task = current;
_


2005-03-06 11:42:20

by Ondrej Zary

[permalink] [raw]
Subject: Re: [patch 2/6] 12/34: cdrom/cdu31a: replace interruptible_sleep_on() with wait_event_interruptible()

[email protected] wrote:
> Use wait_event_interruptible() instead of the deprecated
> interruptible_sleep_on(). The patch is straight-forward as the macros should
> result in the same execution. Patch is compile-tested (still throws out warnings
> regarding {save,restore}_flags()).
>
> Signed-off-by: Nishanth Aravamudan <[email protected]>
> Signed-off-by: Domen Puncer <[email protected]>

I've posted a patch for the cdu31a driver some time ago that removes
almost all usage of interruptible_sleep_on() and also
{save,restore}_flags() - it uses semaphore instead.
The only remaining code is in sony_sleep() function when using
IRQ-driven operation.

See http://lkml.org/lkml/2004/12/18/107
The patch is big because I've messed with the formatting...

--
Ondrej Zary

2005-03-06 21:04:36

by Domen Puncer

[permalink] [raw]
Subject: Re: [patch 2/6] 12/34: cdrom/cdu31a: replace interruptible_sleep_on() with wait_event_interruptible()

On 06/03/05 12:42 +0100, Ondrej Zary wrote:
> [email protected] wrote:
> >Use wait_event_interruptible() instead of the deprecated
> >interruptible_sleep_on(). The patch is straight-forward as the macros
> >should result in the same execution. Patch is compile-tested (still throws
> >out warnings
> >regarding {save,restore}_flags()).
> >
> >Signed-off-by: Nishanth Aravamudan <[email protected]>
> >Signed-off-by: Domen Puncer <[email protected]>
>

Uh, this one escaped me, as schedule() (sleep_on) after cli() is clearly
wrong. Btw. what was the reason for this?

> I've posted a patch for the cdu31a driver some time ago that removes
> almost all usage of interruptible_sleep_on() and also
> {save,restore}_flags() - it uses semaphore instead.
> The only remaining code is in sony_sleep() function when using
> IRQ-driven operation.
>
> See http://lkml.org/lkml/2004/12/18/107
> The patch is big because I've messed with the formatting...

I looked at it, and rewrote some of it into smaller patches. If you don't
mind, can i send them to you for review and testing?


Domen

2005-03-06 22:16:29

by Ondrej Zary

[permalink] [raw]
Subject: Re: [patch 2/6] 12/34: cdrom/cdu31a: replace interruptible_sleep_on() with wait_event_interruptible()

Domen Puncer wrote:
> On 06/03/05 12:42 +0100, Ondrej Zary wrote:
>
>>[email protected] wrote:
>>
>>>Use wait_event_interruptible() instead of the deprecated
>>>interruptible_sleep_on(). The patch is straight-forward as the macros
>>>should result in the same execution. Patch is compile-tested (still throws
>>>out warnings
>>>regarding {save,restore}_flags()).
>>>
>>>Signed-off-by: Nishanth Aravamudan <[email protected]>
>>>Signed-off-by: Domen Puncer <[email protected]>
>>
>
> Uh, this one escaped me, as schedule() (sleep_on) after cli() is clearly
> wrong. Btw. what was the reason for this?

I've never tried the IRQ-driven operation so I don't know if that code
still works (it certainly used to for the original author some years
ago). I think that it should wait until the IRQ from the drive arrives.

>>I've posted a patch for the cdu31a driver some time ago that removes
>>almost all usage of interruptible_sleep_on() and also
>>{save,restore}_flags() - it uses semaphore instead.
>>The only remaining code is in sony_sleep() function when using
>>IRQ-driven operation.
>>
>>See http://lkml.org/lkml/2004/12/18/107
>>The patch is big because I've messed with the formatting...
>
>
> I looked at it, and rewrote some of it into smaller patches. If you don't
> mind, can i send them to you for review and testing?

Thanks. I'll test them. I guess that these smaller patches are more
likely to be accepted into mainline.

--
Ondrej Zary