[ 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 <[email protected]>
>
> 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.
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 <[email protected]>
> >
> > 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
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 <[email protected]>
> > >
> > > 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 <[email protected]>
to the patch changelog in my tree.
--
Jiri Kosina
SUSE Labs, Novell Inc.
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 <[email protected]>
>
> to the patch changelog in my tree.
That's right ...
Michael
Michael Schmitz <[email protected]> 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, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
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