Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Geert Uytterhoeven]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Michael Schmitz]

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
> 

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Jens Axboe]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Michael Schmitz]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Jiri Kosina]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Michael Schmitz]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Jiri Kosina]

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.

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Michael Schmitz]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Andreas Schwab]

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

Re: Fwd: [PATCH] [RFC] ataflop: remove buggy IRQ disable from do_fd_request()

[Michael Schmitz]

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