* [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
@ 2009-10-09 9:53 Jiri Kosina
2009-10-09 9:58 ` Jens Axboe
2009-10-10 9:01 ` Fwd: " Geert Uytterhoeven
0 siblings, 2 replies; 17+ messages in thread
From: Jiri Kosina @ 2009-10-09 9:53 UTC (permalink / raw)
To: Geert Uytterhoeven, Tejun Heo, Jens Axboe; +Cc: linux-kernel
There is a nice gem in drivers/block/ataflop.c::do_fd_request()
void do_fd_request(struct request_queue * q)
{
unsigned long flags;
DPRINT(("do_fd_request for pid %d\n",current->pid));
while( fdc_busy ) sleep_on( &fdc_wait );
fdc_busy = 1;
stdma_lock(floppy_irq, NULL);
atari_disable_irq( IRQ_MFP_FDC );
local_save_flags(flags); /* The request function is called with ints
local_irq_disable(); * disabled... so must save the IPL for later */
redo_fd_request();
local_irq_restore(flags);
atari_enable_irq( IRQ_MFP_FDC );
}
If you look at the code long enough, you will notioce that the
local_irq_disable() call is actually commented out. This has been
introduced back in 2002 in [1], but as you can see, the same bug has been
there even before, with the sti() call being commented out in the very
same way :)
I am not familiar with the code myself at all, but I guess that the whole
stuff can just be removed. Why do we need save_flags/restore_flags at all,
without actually disabling the local IRQs afterwards? The
redo_fd_request() doesn't seem to do anything that would mess with flags
inconsistently.
But I'd rather anyone who has touched the surrounding code in past years
Ack it. I can then take it through trivial tree or submit to akpm.
[1] http://lkml.org/lkml/2002/12/27/58
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/block/ataflop.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 847a9e5..a5af1d6 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1478,10 +1478,7 @@ void do_fd_request(struct request_queue * q)
stdma_lock(floppy_irq, NULL);
atari_disable_irq( IRQ_MFP_FDC );
- local_save_flags(flags); /* The request function is called with ints
- local_irq_disable(); * disabled... so must save the IPL for later */
redo_fd_request();
- local_irq_restore(flags);
atari_enable_irq( IRQ_MFP_FDC );
}
--
1.5.6
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-09 9:53 [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request() Jiri Kosina
@ 2009-10-09 9:58 ` Jens Axboe
2009-10-09 10:08 ` Jiri Kosina
2009-10-10 9:01 ` Fwd: " Geert Uytterhoeven
1 sibling, 1 reply; 17+ messages in thread
From: Jens Axboe @ 2009-10-09 9:58 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Geert Uytterhoeven, Tejun Heo, linux-kernel
On Fri, Oct 09 2009, Jiri Kosina wrote:
> There is a nice gem in drivers/block/ataflop.c::do_fd_request()
>
> void do_fd_request(struct request_queue * q)
> {
> unsigned long flags;
>
> DPRINT(("do_fd_request for pid %d\n",current->pid));
> while( fdc_busy ) sleep_on( &fdc_wait );
> fdc_busy = 1;
> stdma_lock(floppy_irq, NULL);
>
> atari_disable_irq( IRQ_MFP_FDC );
> local_save_flags(flags); /* The request function is called with ints
> local_irq_disable(); * disabled... so must save the IPL for later */
> redo_fd_request();
> local_irq_restore(flags);
> atari_enable_irq( IRQ_MFP_FDC );
> }
>
> If you look at the code long enough, you will notioce that the
> local_irq_disable() call is actually commented out. This has been
> introduced back in 2002 in [1], but as you can see, the same bug has been
> there even before, with the sti() call being commented out in the very
> same way :)
>
> I am not familiar with the code myself at all, but I guess that the whole
> stuff can just be removed. Why do we need save_flags/restore_flags at all,
> without actually disabling the local IRQs afterwards? The
> redo_fd_request() doesn't seem to do anything that would mess with flags
> inconsistently.
>
> But I'd rather anyone who has touched the surrounding code in past years
> Ack it. I can then take it through trivial tree or submit to akpm.
That does look odd. The comment is correct that the function is entered
with interrupts disabled (and the queue lock held). So I'd say your
patch looks fine, the whole save/restore business looks meaningless.
Acked-by: Jens Axboe <jens.axboe@oracle.com>
--
Jens Axboe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-09 9:58 ` Jens Axboe
@ 2009-10-09 10:08 ` Jiri Kosina
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Kosina @ 2009-10-09 10:08 UTC (permalink / raw)
To: Jens Axboe; +Cc: Geert Uytterhoeven, Tejun Heo, linux-kernel
On Fri, 9 Oct 2009, Jens Axboe wrote:
> On Fri, Oct 09 2009, Jiri Kosina wrote:
> > There is a nice gem in drivers/block/ataflop.c::do_fd_request()
> >
> > void do_fd_request(struct request_queue * q)
> > {
> > unsigned long flags;
> >
> > DPRINT(("do_fd_request for pid %d\n",current->pid));
> > while( fdc_busy ) sleep_on( &fdc_wait );
> > fdc_busy = 1;
> > stdma_lock(floppy_irq, NULL);
> >
> > atari_disable_irq( IRQ_MFP_FDC );
> > local_save_flags(flags); /* The request function is called with ints
> > local_irq_disable(); * disabled... so must save the IPL for later */
> > redo_fd_request();
> > local_irq_restore(flags);
> > atari_enable_irq( IRQ_MFP_FDC );
> > }
> >
> > If you look at the code long enough, you will notioce that the
> > local_irq_disable() call is actually commented out. This has been
> > introduced back in 2002 in [1], but as you can see, the same bug has been
> > there even before, with the sti() call being commented out in the very
> > same way :)
> >
> > I am not familiar with the code myself at all, but I guess that the whole
> > stuff can just be removed. Why do we need save_flags/restore_flags at all,
> > without actually disabling the local IRQs afterwards? The
> > redo_fd_request() doesn't seem to do anything that would mess with flags
> > inconsistently.
> >
> > But I'd rather anyone who has touched the surrounding code in past years
> > Ack it. I can then take it through trivial tree or submit to akpm.
>
> That does look odd. The comment is correct that the function is entered
> with interrupts disabled (and the queue lock held). So I'd say your
> patch looks fine, the whole save/restore business looks meaningless.
>
> Acked-by: Jens Axboe <jens.axboe@oracle.com>
Thanks for confirming. I have queued it up in trivial tree.
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-09 9:53 [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request() Jiri Kosina
2009-10-09 9:58 ` Jens Axboe
@ 2009-10-10 9:01 ` Geert Uytterhoeven
2009-10-11 7:01 ` Michael Schmitz
1 sibling, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2009-10-10 9:01 UTC (permalink / raw)
To: linux-m68k
---------- Forwarded message ----------
From: Jiri Kosina <jkosina@suse.cz>
Date: Fri, 9 Oct 2009 11:53:21 +0200 (CEST)
Subject: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
To: Geert Uytterhoeven <geert@linux-m68k.org>, Tejun Heo
<tj@kernel.org>, Jens Axboe <jens.axboe@oracle.com>
Cc: linux-kernel@vger.kernel.org
There is a nice gem in drivers/block/ataflop.c::do_fd_request()
void do_fd_request(struct request_queue * q)
{
unsigned long flags;
DPRINT(("do_fd_request for pid %d\n",current->pid));
while( fdc_busy ) sleep_on( &fdc_wait );
fdc_busy = 1;
stdma_lock(floppy_irq, NULL);
atari_disable_irq( IRQ_MFP_FDC );
local_save_flags(flags); /* The request function is called with ints
local_irq_disable(); * disabled... so must save the IPL
for later */
redo_fd_request();
local_irq_restore(flags);
atari_enable_irq( IRQ_MFP_FDC );
}
If you look at the code long enough, you will notioce that the
local_irq_disable() call is actually commented out. This has been
introduced back in 2002 in [1], but as you can see, the same bug has been
there even before, with the sti() call being commented out in the very
same way :)
I am not familiar with the code myself at all, but I guess that the whole
stuff can just be removed. Why do we need save_flags/restore_flags at all,
without actually disabling the local IRQs afterwards? The
redo_fd_request() doesn't seem to do anything that would mess with flags
inconsistently.
But I'd rather anyone who has touched the surrounding code in past years
Ack it. I can then take it through trivial tree or submit to akpm.
[1] http://lkml.org/lkml/2002/12/27/58
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/block/ataflop.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 847a9e5..a5af1d6 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1478,10 +1478,7 @@ void do_fd_request(struct request_queue * q)
stdma_lock(floppy_irq, NULL);
atari_disable_irq( IRQ_MFP_FDC );
- local_save_flags(flags); /* The request function is called with ints
- local_irq_disable(); * disabled... so must save the IPL for later */
redo_fd_request();
- local_irq_restore(flags);
atari_enable_irq( IRQ_MFP_FDC );
}
--
1.5.6
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-10 9:01 ` Fwd: " Geert Uytterhoeven
@ 2009-10-11 7:01 ` Michael Schmitz
2009-10-12 7:58 ` Jens Axboe
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Michael Schmitz @ 2009-10-11 7:01 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-m68k
Hi Geert,
> There is a nice gem in drivers/block/ataflop.c::do_fd_request()
>
> void do_fd_request(struct request_queue * q)
> {
> unsigned long flags;
>
> DPRINT(("do_fd_request for pid %d\n",current->pid));
> while( fdc_busy ) sleep_on( &fdc_wait );
> fdc_busy = 1;
> stdma_lock(floppy_irq, NULL);
>
> atari_disable_irq( IRQ_MFP_FDC );
> local_save_flags(flags); /* The request function is called with ints
> local_irq_disable(); * disabled... so must save the IPL
> for later */
> redo_fd_request();
> local_irq_restore(flags);
> atari_enable_irq( IRQ_MFP_FDC );
> }
>
> If you look at the code long enough, you will notioce that the
> local_irq_disable() call is actually commented out. This has been
> introduced back in 2002 in [1], but as you can see, the same bug has been
> there even before, with the sti() call being commented out in the very
> same way :)
>
> I am not familiar with the code myself at all, but I guess that the whole
> stuff can just be removed. Why do we need save_flags/restore_flags at all,
> without actually disabling the local IRQs afterwards? The
The IRQ source has been disabled in the MFC by the
atari_disable_irq( IRQ_MFP_FDC ) call just before local_save_flags(flags).
For that reason, the fact that local_irq_disable is commented out will not
usually matter (a timer interrupt that would result in retrying the floppy
request or removing the request from the queue excepted).
I would rather suggest to leave the code in, and fix the buggy comments instead.
Note: IDE or SCSI cannot get in the way here -and I haven't seen IDE and
floppy races in recent kernels. SCSI was pretty broken anyway last time I tried
a few months ago. I cannot run any tests since my PC motherboard or CPU died
recently.
> redo_fd_request() doesn't seem to do anything that would mess with flags
> inconsistently.
>
> But I'd rather anyone who has touched the surrounding code in past years
> Ack it. I can then take it through trivial tree or submit to akpm.
The surounding code probably hasn't been touched in ages. The floppy driver in
its current state does work. If redo_fd_request could alter timers ot queues,
rmoving the locking would be dangerous, no?
> [1] http://lkml.org/lkml/2002/12/27/58
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
NAck for my part.
Michael
> ---
> drivers/block/ataflop.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index 847a9e5..a5af1d6 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -1478,10 +1478,7 @@ void do_fd_request(struct request_queue * q)
> stdma_lock(floppy_irq, NULL);
>
> atari_disable_irq( IRQ_MFP_FDC );
> - local_save_flags(flags); /* The request function is called with ints
> - local_irq_disable(); * disabled... so must save the IPL for later */
> redo_fd_request();
> - local_irq_restore(flags);
> atari_enable_irq( IRQ_MFP_FDC );
> }
>
> --
> 1.5.6
>
>
>
>
> --
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-11 7:01 ` Michael Schmitz
@ 2009-10-12 7:58 ` Jens Axboe
2009-10-18 8:24 ` Michael Schmitz
2009-10-12 9:19 ` Jiri Kosina
2009-10-12 9:19 ` Jiri Kosina
2 siblings, 1 reply; 17+ messages in thread
From: Jens Axboe @ 2009-10-12 7:58 UTC (permalink / raw)
To: Michael Schmitz; +Cc: Geert Uytterhoeven, linux-m68k
On Sun, Oct 11 2009, Michael Schmitz wrote:
> Hi Geert,
>
> > There is a nice gem in drivers/block/ataflop.c::do_fd_request()
> >
> > void do_fd_request(struct request_queue * q)
> > {
> > unsigned long flags;
> >
> > DPRINT(("do_fd_request for pid %d\n",current->pid));
> > while( fdc_busy ) sleep_on( &fdc_wait );
> > fdc_busy = 1;
> > stdma_lock(floppy_irq, NULL);
> >
> > atari_disable_irq( IRQ_MFP_FDC );
> > local_save_flags(flags); /* The request function is called with ints
> > local_irq_disable(); * disabled... so must save the IPL
> > for later */
> > redo_fd_request();
> > local_irq_restore(flags);
> > atari_enable_irq( IRQ_MFP_FDC );
> > }
> >
> > If you look at the code long enough, you will notioce that the
> > local_irq_disable() call is actually commented out. This has been
> > introduced back in 2002 in [1], but as you can see, the same bug has been
> > there even before, with the sti() call being commented out in the very
> > same way :)
> >
> > I am not familiar with the code myself at all, but I guess that the whole
> > stuff can just be removed. Why do we need save_flags/restore_flags at all,
> > without actually disabling the local IRQs afterwards? The
>
> The IRQ source has been disabled in the MFC by the atari_disable_irq(
> IRQ_MFP_FDC ) call just before local_save_flags(flags). For that
> reason, the fact that local_irq_disable is commented out will not
> usually matter (a timer interrupt that would result in retrying the
> floppy request or removing the request from the queue excepted).
>
> I would rather suggest to leave the code in, and fix the buggy
> comments instead.
What buggy comments? The comments states that interrupts are already
disabled when entering this function, which is correct. The point is
that doing a flags save and then an irq disable is pointless, since we
KNOW that interrupts are already disabled.
--
Jens Axboe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-11 7:01 ` Michael Schmitz
2009-10-12 7:58 ` Jens Axboe
2009-10-12 9:19 ` Jiri Kosina
@ 2009-10-12 9:19 ` Jiri Kosina
2009-10-18 21:14 ` Michael Schmitz
2 siblings, 1 reply; 17+ messages in thread
From: Jiri Kosina @ 2009-10-12 9:19 UTC (permalink / raw)
To: Michael Schmitz
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
[ restoring original recepient list, and thus keeping all your text
below for reference ]
On Sun, 11 Oct 2009, Michael Schmitz wrote:
> > There is a nice gem in drivers/block/ataflop.c::do_fd_request()
> >
> > void do_fd_request(struct request_queue * q)
> > {
> > unsigned long flags;
> >
> > DPRINT(("do_fd_request for pid %d\n",current->pid));
> > while( fdc_busy ) sleep_on( &fdc_wait );
> > fdc_busy = 1;
> > stdma_lock(floppy_irq, NULL);
> >
> > atari_disable_irq( IRQ_MFP_FDC );
> > local_save_flags(flags); /* The request function is called with ints
> > local_irq_disable(); * disabled... so must save the IPL
> > for later */
> > redo_fd_request();
> > local_irq_restore(flags);
> > atari_enable_irq( IRQ_MFP_FDC );
> > }
> >
> > If you look at the code long enough, you will notioce that the
> > local_irq_disable() call is actually commented out. This has been
> > introduced back in 2002 in [1], but as you can see, the same bug has been
> > there even before, with the sti() call being commented out in the very
> > same way :)
> >
> > I am not familiar with the code myself at all, but I guess that the whole
> > stuff can just be removed. Why do we need save_flags/restore_flags at all,
> > without actually disabling the local IRQs afterwards? The
>
> The IRQ source has been disabled in the MFC by the atari_disable_irq(
> IRQ_MFP_FDC ) call just before local_save_flags(flags). For that reason,
> the fact that local_irq_disable is commented out will not usually matter
> (a timer interrupt that would result in retrying the floppy request or
> removing the request from the queue excepted).
>
> I would rather suggest to leave the code in, and fix the buggy comments instead.
>
> Note: IDE or SCSI cannot get in the way here -and I haven't seen IDE and
> floppy races in recent kernels. SCSI was pretty broken anyway last time I tried
> a few months ago. I cannot run any tests since my PC motherboard or CPU died
> recently.
>
> > redo_fd_request() doesn't seem to do anything that would mess with flags
> > inconsistently.
> >
> > But I'd rather anyone who has touched the surrounding code in past years
> > Ack it. I can then take it through trivial tree or submit to akpm.
>
> The surounding code probably hasn't been touched in ages. The floppy driver in
> its current state does work. If redo_fd_request could alter timers ot queues,
> rmoving the locking would be dangerous, no?
The patch is not removing any locking. It only
1) removes the local_irq_disable() that has been commented out for many
years already anyway
2) removes the saving and restoring of CPU flags around do_fd_request(),
which is rather clearly a nop than any kind of "locking"
> > [1] http://lkml.org/lkml/2002/12/27/58
> >
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>
> NAck for my part.
Please elaborate a little bit more which of the two points above you base
your NACK on.
>
> Michael
>
>
> > ---
> > drivers/block/ataflop.c | 3 ---
> > 1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> > index 847a9e5..a5af1d6 100644
> > --- a/drivers/block/ataflop.c
> > +++ b/drivers/block/ataflop.c
> > @@ -1478,10 +1478,7 @@ void do_fd_request(struct request_queue * q)
> > stdma_lock(floppy_irq, NULL);
> >
> > atari_disable_irq( IRQ_MFP_FDC );
> > - local_save_flags(flags); /* The request function is called with ints
> > - local_irq_disable(); * disabled... so must save the IPL for later */
> > redo_fd_request();
> > - local_irq_restore(flags);
> > atari_enable_irq( IRQ_MFP_FDC );
> > }
> >
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-11 7:01 ` Michael Schmitz
2009-10-12 7:58 ` Jens Axboe
@ 2009-10-12 9:19 ` Jiri Kosina
2009-10-12 9:19 ` Jiri Kosina
2 siblings, 0 replies; 17+ messages in thread
From: Jiri Kosina @ 2009-10-12 9:19 UTC (permalink / raw)
To: Michael Schmitz
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
[ restoring original recepient list, and thus keeping all your text
below for reference ]
On Sun, 11 Oct 2009, Michael Schmitz wrote:
> > There is a nice gem in drivers/block/ataflop.c::do_fd_request()
> >
> > void do_fd_request(struct request_queue * q)
> > {
> > unsigned long flags;
> >
> > DPRINT(("do_fd_request for pid %d\n",current->pid));
> > while( fdc_busy ) sleep_on( &fdc_wait );
> > fdc_busy = 1;
> > stdma_lock(floppy_irq, NULL);
> >
> > atari_disable_irq( IRQ_MFP_FDC );
> > local_save_flags(flags); /* The request function is called with ints
> > local_irq_disable(); * disabled... so must save the IPL
> > for later */
> > redo_fd_request();
> > local_irq_restore(flags);
> > atari_enable_irq( IRQ_MFP_FDC );
> > }
> >
> > If you look at the code long enough, you will notioce that the
> > local_irq_disable() call is actually commented out. This has been
> > introduced back in 2002 in [1], but as you can see, the same bug has been
> > there even before, with the sti() call being commented out in the very
> > same way :)
> >
> > I am not familiar with the code myself at all, but I guess that the whole
> > stuff can just be removed. Why do we need save_flags/restore_flags at all,
> > without actually disabling the local IRQs afterwards? The
>
> The IRQ source has been disabled in the MFC by the atari_disable_irq(
> IRQ_MFP_FDC ) call just before local_save_flags(flags). For that reason,
> the fact that local_irq_disable is commented out will not usually matter
> (a timer interrupt that would result in retrying the floppy request or
> removing the request from the queue excepted).
>
> I would rather suggest to leave the code in, and fix the buggy comments instead.
>
> Note: IDE or SCSI cannot get in the way here -and I haven't seen IDE and
> floppy races in recent kernels. SCSI was pretty broken anyway last time I tried
> a few months ago. I cannot run any tests since my PC motherboard or CPU died
> recently.
>
> > redo_fd_request() doesn't seem to do anything that would mess with flags
> > inconsistently.
> >
> > But I'd rather anyone who has touched the surrounding code in past years
> > Ack it. I can then take it through trivial tree or submit to akpm.
>
> The surounding code probably hasn't been touched in ages. The floppy driver in
> its current state does work. If redo_fd_request could alter timers ot queues,
> rmoving the locking would be dangerous, no?
The patch is not removing any locking. It only
1) removes the local_irq_disable() that has been commented out for many
years already anyway
2) removes the saving and restoring of CPU flags around do_fd_request(),
which is rather clearly a nop than any kind of "locking"
> > [1] http://lkml.org/lkml/2002/12/27/58
> >
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>
> NAck for my part.
Please elaborate a little bit more which of the two points above you base
your NACK on.
>
> Michael
>
>
> > ---
> > drivers/block/ataflop.c | 3 ---
> > 1 files changed, 0 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> > index 847a9e5..a5af1d6 100644
> > --- a/drivers/block/ataflop.c
> > +++ b/drivers/block/ataflop.c
> > @@ -1478,10 +1478,7 @@ void do_fd_request(struct request_queue * q)
> > stdma_lock(floppy_irq, NULL);
> >
> > atari_disable_irq( IRQ_MFP_FDC );
> > - local_save_flags(flags); /* The request function is called with ints
> > - local_irq_disable(); * disabled... so must save the IPL for later */
> > redo_fd_request();
> > - local_irq_restore(flags);
> > atari_enable_irq( IRQ_MFP_FDC );
> > }
> >
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-12 7:58 ` Jens Axboe
@ 2009-10-18 8:24 ` Michael Schmitz
0 siblings, 0 replies; 17+ messages in thread
From: Michael Schmitz @ 2009-10-18 8:24 UTC (permalink / raw)
To: Jens Axboe; +Cc: Geert Uytterhoeven, linux-m68k
Hi Jens, Geert,
> > >
> > > If you look at the code long enough, you will notioce that the
> > > local_irq_disable() call is actually commented out. This has been
> > > introduced back in 2002 in [1], but as you can see, the same bug has been
> > > there even before, with the sti() call being commented out in the very
> > > same way :)
Please note above: the commented out line used to be sti() - this is still
present in 2.4.30 which I've been running for a long time. The driver uses
timers for various tasks, and this was to ensure timer interrupts are on during
redo_fd_request.
The change in 2002 substituted local_irq_disable() for the sti() which seems
clearly wrong.
> > I would rather suggest to leave the code in, and fix the buggy
> > comments instead.
>
> What buggy comments? The comments states that interrupts are already
> disabled when entering this function, which is correct. The point is
> that doing a flags save and then an irq disable is pointless, since we
> KNOW that interrupts are already disabled.
That's what is worrying me - enabling interrupts for the sake of getting timers
working used to be necessary at one time.
There is something else wrong with do_fd_request - if indeed interrupts are
disabled, stdma_lock may attempt to sleep and would deadlock.
I guess scheduling redo_fd_request as bottom half or delayed work would be
required here. I need to look into this more closely.
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-12 9:19 ` Jiri Kosina
@ 2009-10-18 21:14 ` Michael Schmitz
0 siblings, 0 replies; 17+ messages in thread
From: Michael Schmitz @ 2009-10-18 21:14 UTC (permalink / raw)
To: Jiri Kosina
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
Hi Jiri,
trying for a somewhat more coherent opinion:
> > > If you look at the code long enough, you will notioce that the
> > > local_irq_disable() call is actually commented out. This has been
> > > introduced back in 2002 in [1], but as you can see, the same bug has been
> > > there even before, with the sti() call being commented out in the very
> > > same way :)
The substitution of sti() by local_irq_disable() had me a bit confused here. The
last time I worked on this driver was when we still had a big kernel lock, so
the sti() might have been crucial there.
The code does evidently work fine without it, and redo_fd_request as well as
do_fd_action do not need to reenable local interupts or otherwise change the
interrupt level anymore. After a bit more pondering over the code I now
understand why this is ...
> > The surounding code probably hasn't been touched in ages. The floppy driver in
> > its current state does work. If redo_fd_request could alter timers ot queues,
> > rmoving the locking would be dangerous, no?
>
> The patch is not removing any locking. It only
>
> 1) removes the local_irq_disable() that has been commented out for many
> years already anyway
> 2) removes the saving and restoring of CPU flags around do_fd_request(),
> which is rather clearly a nop than any kind of "locking"
>
> > > [1] http://lkml.org/lkml/2002/12/27/58
> > >
> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> >
> > NAck for my part.
>
> Please elaborate a little bit more which of the two points above you base
> your NACK on.
The removal of local_irq_disable() (which should have been local_irq_enable())
just raised a flag, and I didn't immediately see why the interrupt enable had
been commented out.
With a bit of further thought on the matter I am satisfied that this patch will
not impact on driver function at all, and do not wish to sustain my objection.
IOW: Ack, and my sincere apologies for wasting your time.
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
@ 2009-10-18 21:14 ` Michael Schmitz
0 siblings, 0 replies; 17+ messages in thread
From: Michael Schmitz @ 2009-10-18 21:14 UTC (permalink / raw)
To: Jiri Kosina
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
Hi Jiri,
trying for a somewhat more coherent opinion:
> > > If you look at the code long enough, you will notioce that the
> > > local_irq_disable() call is actually commented out. This has been
> > > introduced back in 2002 in [1], but as you can see, the same bug has been
> > > there even before, with the sti() call being commented out in the very
> > > same way :)
The substitution of sti() by local_irq_disable() had me a bit confused here. The
last time I worked on this driver was when we still had a big kernel lock, so
the sti() might have been crucial there.
The code does evidently work fine without it, and redo_fd_request as well as
do_fd_action do not need to reenable local interupts or otherwise change the
interrupt level anymore. After a bit more pondering over the code I now
understand why this is ...
> > The surounding code probably hasn't been touched in ages. The floppy driver in
> > its current state does work. If redo_fd_request could alter timers ot queues,
> > rmoving the locking would be dangerous, no?
>
> The patch is not removing any locking. It only
>
> 1) removes the local_irq_disable() that has been commented out for many
> years already anyway
> 2) removes the saving and restoring of CPU flags around do_fd_request(),
> which is rather clearly a nop than any kind of "locking"
>
> > > [1] http://lkml.org/lkml/2002/12/27/58
> > >
> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> >
> > NAck for my part.
>
> Please elaborate a little bit more which of the two points above you base
> your NACK on.
The removal of local_irq_disable() (which should have been local_irq_enable())
just raised a flag, and I didn't immediately see why the interrupt enable had
been commented out.
With a bit of further thought on the matter I am satisfied that this patch will
not impact on driver function at all, and do not wish to sustain my objection.
IOW: Ack, and my sincere apologies for wasting your time.
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-18 21:14 ` Michael Schmitz
@ 2009-10-19 7:35 ` Jiri Kosina
-1 siblings, 0 replies; 17+ messages in thread
From: Jiri Kosina @ 2009-10-19 7:35 UTC (permalink / raw)
To: Michael Schmitz
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
On Sun, 18 Oct 2009, Michael Schmitz wrote:
> > The patch is not removing any locking. It only
> >
> > 1) removes the local_irq_disable() that has been commented out for many
> > years already anyway
> > 2) removes the saving and restoring of CPU flags around do_fd_request(),
> > which is rather clearly a nop than any kind of "locking"
> >
> > > > [1] http://lkml.org/lkml/2002/12/27/58
> > > >
> > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > >
> > > NAck for my part.
> >
> > Please elaborate a little bit more which of the two points above you base
> > your NACK on.
>
> The removal of local_irq_disable() (which should have been local_irq_enable())
> just raised a flag, and I didn't immediately see why the interrupt enable had
> been commented out.
Yes, it has been commented out in a very non-intuitive way.
> With a bit of further thought on the matter I am satisfied that this patch will
> not impact on driver function at all, and do not wish to sustain my objection.
>
> IOW: Ack, and my sincere apologies for wasting your time.
Thanks, I have added
Acked-by: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
to the patch changelog in my tree.
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
@ 2009-10-19 7:35 ` Jiri Kosina
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Kosina @ 2009-10-19 7:35 UTC (permalink / raw)
To: Michael Schmitz
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
On Sun, 18 Oct 2009, Michael Schmitz wrote:
> > The patch is not removing any locking. It only
> >
> > 1) removes the local_irq_disable() that has been commented out for many
> > years already anyway
> > 2) removes the saving and restoring of CPU flags around do_fd_request(),
> > which is rather clearly a nop than any kind of "locking"
> >
> > > > [1] http://lkml.org/lkml/2002/12/27/58
> > > >
> > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > >
> > > NAck for my part.
> >
> > Please elaborate a little bit more which of the two points above you base
> > your NACK on.
>
> The removal of local_irq_disable() (which should have been local_irq_enable())
> just raised a flag, and I didn't immediately see why the interrupt enable had
> been commented out.
Yes, it has been commented out in a very non-intuitive way.
> With a bit of further thought on the matter I am satisfied that this patch will
> not impact on driver function at all, and do not wish to sustain my objection.
>
> IOW: Ack, and my sincere apologies for wasting your time.
Thanks, I have added
Acked-by: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
to the patch changelog in my tree.
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-19 7:35 ` Jiri Kosina
(?)
(?)
@ 2009-10-20 6:51 ` Michael Schmitz
2009-10-20 9:24 ` Andreas Schwab
-1 siblings, 1 reply; 17+ messages in thread
From: Michael Schmitz @ 2009-10-20 6:51 UTC (permalink / raw)
To: Jiri Kosina
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
Hi Jiri,
> > > Please elaborate a little bit more which of the two points above you base
> > > your NACK on.
> >
> > The removal of local_irq_disable() (which should have been local_irq_enable())
> > just raised a flag, and I didn't immediately see why the interrupt enable had
> > been commented out.
>
> Yes, it has been commented out in a very non-intuitive way.
That. too :-) What I meant to say - the reason why someone chose to comment out
the original sti() wasn't really clear. I guess the reason for that particular
change has been lost in the pre-git or bk era.
> > With a bit of further thought on the matter I am satisfied that this patch will
> > not impact on driver function at all, and do not wish to sustain my objection.
> >
> > IOW: Ack, and my sincere apologies for wasting your time.
>
> Thanks, I have added
>
> Acked-by: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
>
> to the patch changelog in my tree.
That's right ...
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-19 7:35 ` Jiri Kosina
(?)
@ 2009-10-20 6:51 ` Michael Schmitz
-1 siblings, 0 replies; 17+ messages in thread
From: Michael Schmitz @ 2009-10-20 6:51 UTC (permalink / raw)
To: Jiri Kosina
Cc: Geert Uytterhoeven, linux-m68k, Tejun Heo, Jens Axboe, linux-kernel
Hi Jiri,
> > > Please elaborate a little bit more which of the two points above you base
> > > your NACK on.
> >
> > The removal of local_irq_disable() (which should have been local_irq_enable())
> > just raised a flag, and I didn't immediately see why the interrupt enable had
> > been commented out.
>
> Yes, it has been commented out in a very non-intuitive way.
That. too :-) What I meant to say - the reason why someone chose to comment out
the original sti() wasn't really clear. I guess the reason for that particular
change has been lost in the pre-git or bk era.
> > With a bit of further thought on the matter I am satisfied that this patch will
> > not impact on driver function at all, and do not wish to sustain my objection.
> >
> > IOW: Ack, and my sincere apologies for wasting your time.
>
> Thanks, I have added
>
> Acked-by: Michael Schmitz <schmitz@biophys.uni-duesseldorf.de>
>
> to the patch changelog in my tree.
That's right ...
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-20 6:51 ` Michael Schmitz
@ 2009-10-20 9:24 ` Andreas Schwab
2009-10-31 3:58 ` Michael Schmitz
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Schwab @ 2009-10-20 9:24 UTC (permalink / raw)
To: Michael Schmitz
Cc: Jiri Kosina, Geert Uytterhoeven, linux-m68k, Tejun Heo,
Jens Axboe, linux-kernel
Michael Schmitz <schmitz@biophys.uni-duesseldorf.de> writes:
> That. too :-) What I meant to say - the reason why someone chose to comment out
> the original sti() wasn't really clear. I guess the reason for that particular
> change has been lost in the pre-git or bk era.
FWIW, this can be traced back to as much as the 2.0 era (1996). Going
back further is difficult because a lot of files on ftp.uni-erlangen.de
are corrupt.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()
2009-10-20 9:24 ` Andreas Schwab
@ 2009-10-31 3:58 ` Michael Schmitz
0 siblings, 0 replies; 17+ messages in thread
From: Michael Schmitz @ 2009-10-31 3:58 UTC (permalink / raw)
To: Andreas Schwab
Cc: Jiri Kosina, Geert Uytterhoeven, linux-m68k, Tejun Heo,
Jens Axboe, linux-kernel
Hi Andreas,
> > That. too :-) What I meant to say - the reason why someone chose to comment out
> > the original sti() wasn't really clear. I guess the reason for that particular
> > change has been lost in the pre-git or bk era.
>
> FWIW, this can be traced back to as much as the 2.0 era (1996). Going
Thanks, that definitely settles it. Around that time was my only exposure to the
floppy code (making mtools work). Brings back some memories ...
> back further is difficult because a lot of files on ftp.uni-erlangen.de
> are corrupt.
You haven't really dug around on the old FTP site just for this?
Cheers,
Michael
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2009-10-31 3:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-10-09 9:53 [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request() Jiri Kosina
2009-10-09 9:58 ` Jens Axboe
2009-10-09 10:08 ` Jiri Kosina
2009-10-10 9:01 ` Fwd: " Geert Uytterhoeven
2009-10-11 7:01 ` Michael Schmitz
2009-10-12 7:58 ` Jens Axboe
2009-10-18 8:24 ` Michael Schmitz
2009-10-12 9:19 ` Jiri Kosina
2009-10-12 9:19 ` Jiri Kosina
2009-10-18 21:14 ` Michael Schmitz
2009-10-18 21:14 ` Michael Schmitz
2009-10-19 7:35 ` Jiri Kosina
2009-10-19 7:35 ` Jiri Kosina
2009-10-20 6:51 ` Michael Schmitz
2009-10-20 6:51 ` Michael Schmitz
2009-10-20 9:24 ` Andreas Schwab
2009-10-31 3:58 ` Michael Schmitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.