I have Martin's permission to push some ide-clean patches, so here we go.
I'm running fine SMP kernel with spinlocks debugging enabled on UP.
Note that this patch brokes ide-cd/tape/floppy/scsi locking...
This patch is mainly for developers to have something to sync with... ;-)
still TODO:
- remove ide_wait commands from interrupt paths
- move locking/completion upwards from ide_do_drive_cmd()
- fix ide device setup/tear down locking
- locking of ioctls
- tape->spinlock is probably obsolete now
Thu Jun 27 13:32:35 CEST 2002 ide-clean-95
It is a merge of patches by Alexander Atanasov and Zwane Mwaikambo
(big thanks guys!) plus some corrections by me...
- Remove locking from IRQ handlers (*_intr etc.) and ata_ops->do_reqeuest(),
ata_ops->end_request().
- Add non-locking __blk_get_request() and __blk_attempt_remege() helpers
to block layer (ll_rw_blk.c).
- Move locking up to the entry points.
- Move ata_expiry_t functions to the ide_startstop_t interface
(need to call ata_error, and its return is idestartstop_t, not wait time).
- Kill ata_end_request() and restart_request() (they were locking variants).
- Disable some ide__sti() for now.
- Misc fixes.
Hi Petr!
On Sun, 30 Jun 2002, Petr Vandrovec wrote:
> Hi,
> I know that IDE95 is broken when it comes to IDE, but... not only
> that. Patch below does:
>
> (1) ide-taskfile.c: ide_do_drive_cmd(..., ide_preempt) holds channel
> lock. Do not reacquire. NMI watchdog triggered by just booting
> computer with IDE cdrom.
Mentioned in 95 changelog.
Already fixed in my tree, but thanks anyway.
> (2) ide.c: if we clear BUSY, use goto ... instead of break. There is
> set_bit(IDE_BUSY, ch->active); below loop. Channel stopped by 'hdparm
> -I /dev/hdd': it uses ide_raw_taskfile() which sets REQ_SPECIAL.
> But ide-cd:ide_cdrom_do_request() does not handle REQ_SPECIAL,
> it returns ide_stopped immediately. And ide:do_request() does not
> cope with ide_stopped + empty queue correctly.
This set_bit(IDE_BUSY, ch->active); introduced in 94 is basically wrong,
also already fixed in my tree ;-)
> (3) unfixed: hdparm -I returns garbage on CDROM: REQ_SPECIAL command
> on CD drive returns success without trying to execute it in
> current driver. I think that ide-cd should handle direct interface
> to its registers properly.
>
Have to look at it, thanks for info.
Attached patch is next ide-clean patch pre-patch ;), just not to duplicate
efforts. Changelog is also included. As always use with care, standard
disclaimer apply.
And final note: I think that previous locking (2.4.x but ch->lock instead
of global io_request_lock) was well tuned and almost 100% correct.
Recent changes just made it worse (sorry Martin :) ).
Now even if we add unmasking IRQs with disabling currently handled IRQ, it
will be less friendlier to shared PCI interrupts (especially in PIO it
will be overkill to disable shared IRQ for handling PIO intr!),
so I want to revert to previous scheme...
Greets.
> Patch is for 2.5.24+IDE94+IDE95.
> Best regards,
> Petr Vandrovec
> [email protected]
>
>
>
>
> diff -urdN linux/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
> --- linux/drivers/ide/ide-taskfile.c Sun Jun 30 00:54:05 2002
> +++ linux/drivers/ide/ide-taskfile.c Sat Jun 29 12:54:48 2002
> @@ -209,10 +209,16 @@
> rq->errors = 0;
> rq->rq_status = RQ_ACTIVE;
> rq->rq_dev = mk_kdev(major,(drive->select.b.unit)<<PARTN_BITS);
> - if (action == ide_wait)
> + if (action == ide_wait) {
> rq->waiting = &wait;
> -
> - spin_lock_irqsave(drive->channel->lock, flags);
> + spin_lock_irqsave(drive->channel->lock, flags);
> + } else if (action == ide_preempt) {
> + if (0 /* SMP... !spin_is_locked(drive->channel->lock) */) {
> + BUG();
> + }
> + } else {
> + BUG();
This is not completly correct, you forgot about ide_end (default action,
broken anyway, please read patch info).
> + }
>
> if (blk_queue_empty(&drive->queue) || action == ide_preempt) {
> if (action == ide_preempt)
> @@ -227,9 +233,9 @@
>
> do_ide_request(q);
>
> - spin_unlock_irqrestore(drive->channel->lock, flags);
> -
> if (action == ide_wait) {
> + spin_unlock_irqrestore(drive->channel->lock, flags);
> +
> wait_for_completion(&wait); /* wait for it to be serviced */
> return rq->errors ? -EIO : 0; /* return -EIO if errors */
> }
> diff -urdN linux/drivers/ide/ide.c linux/drivers/ide/ide.c
> --- linux/drivers/ide/ide.c Sun Jun 30 00:54:05 2002
> +++ linux/drivers/ide/ide.c Sat Jun 29 23:45:47 2002
> @@ -864,7 +864,7 @@
> if (!ata_can_queue(drive)) {
> if (!ata_pending_commands(drive))
> clear_bit(IDE_BUSY, ch->active);
> - break;
> + goto dontsetbusy;
> }
>
> drive->sleep = 0;
> @@ -889,7 +889,7 @@
> if (!ata_pending_commands(drive))
> clear_bit(IDE_BUSY, ch->active);
> drive->rq = NULL;
> - break;
> + goto dontsetbusy;
> }
>
> /* If there are queued commands, we can't start a
> @@ -906,6 +906,7 @@
> /* make sure the BUSY bit is set */
> /* FIXME: perhaps there is some place where we miss to set it? */
> set_bit(IDE_BUSY, ch->active);
> +dontsetbusy:;
> }
> }
>
>
Hi,
I know that IDE95 is broken when it comes to IDE, but... not only
that. Patch below does:
(1) ide-taskfile.c: ide_do_drive_cmd(..., ide_preempt) holds channel
lock. Do not reacquire. NMI watchdog triggered by just booting
computer with IDE cdrom.
(2) ide.c: if we clear BUSY, use goto ... instead of break. There is
set_bit(IDE_BUSY, ch->active); below loop. Channel stopped by 'hdparm
-I /dev/hdd': it uses ide_raw_taskfile() which sets REQ_SPECIAL.
But ide-cd:ide_cdrom_do_request() does not handle REQ_SPECIAL,
it returns ide_stopped immediately. And ide:do_request() does not
cope with ide_stopped + empty queue correctly.
(3) unfixed: hdparm -I returns garbage on CDROM: REQ_SPECIAL command
on CD drive returns success without trying to execute it in
current driver. I think that ide-cd should handle direct interface
to its registers properly.
Patch is for 2.5.24+IDE94+IDE95.
Best regards,
Petr Vandrovec
[email protected]
diff -urdN linux/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
--- linux/drivers/ide/ide-taskfile.c Sun Jun 30 00:54:05 2002
+++ linux/drivers/ide/ide-taskfile.c Sat Jun 29 12:54:48 2002
@@ -209,10 +209,16 @@
rq->errors = 0;
rq->rq_status = RQ_ACTIVE;
rq->rq_dev = mk_kdev(major,(drive->select.b.unit)<<PARTN_BITS);
- if (action == ide_wait)
+ if (action == ide_wait) {
rq->waiting = &wait;
-
- spin_lock_irqsave(drive->channel->lock, flags);
+ spin_lock_irqsave(drive->channel->lock, flags);
+ } else if (action == ide_preempt) {
+ if (0 /* SMP... !spin_is_locked(drive->channel->lock) */) {
+ BUG();
+ }
+ } else {
+ BUG();
+ }
if (blk_queue_empty(&drive->queue) || action == ide_preempt) {
if (action == ide_preempt)
@@ -227,9 +233,9 @@
do_ide_request(q);
- spin_unlock_irqrestore(drive->channel->lock, flags);
-
if (action == ide_wait) {
+ spin_unlock_irqrestore(drive->channel->lock, flags);
+
wait_for_completion(&wait); /* wait for it to be serviced */
return rq->errors ? -EIO : 0; /* return -EIO if errors */
}
diff -urdN linux/drivers/ide/ide.c linux/drivers/ide/ide.c
--- linux/drivers/ide/ide.c Sun Jun 30 00:54:05 2002
+++ linux/drivers/ide/ide.c Sat Jun 29 23:45:47 2002
@@ -864,7 +864,7 @@
if (!ata_can_queue(drive)) {
if (!ata_pending_commands(drive))
clear_bit(IDE_BUSY, ch->active);
- break;
+ goto dontsetbusy;
}
drive->sleep = 0;
@@ -889,7 +889,7 @@
if (!ata_pending_commands(drive))
clear_bit(IDE_BUSY, ch->active);
drive->rq = NULL;
- break;
+ goto dontsetbusy;
}
/* If there are queued commands, we can't start a
@@ -906,6 +906,7 @@
/* make sure the BUSY bit is set */
/* FIXME: perhaps there is some place where we miss to set it? */
set_bit(IDE_BUSY, ch->active);
+dontsetbusy:;
}
}
On Sun, 30 Jun 2002, Bartlomiej Zolnierkiewicz wrote:
> > (1) ide-taskfile.c: ide_do_drive_cmd(..., ide_preempt) holds channel
> > lock. Do not reacquire. NMI watchdog triggered by just booting
> > computer with IDE cdrom.
>
> Mentioned in 95 changelog.
> Already fixed in my tree, but thanks anyway.
Hmm i just spent some time last night trying to go through possible
paths for ide_do_drive_cmd to come up with a solution for that one, do you
use some sort of SCM so that i can keep track of whats been covered?
> Attached patch is next ide-clean patch pre-patch ;), just not to duplicate
> efforts. Changelog is also included. As always use with care, standard
> disclaimer apply.
Thanks
> And final note: I think that previous locking (2.4.x but ch->lock instead
> of global io_request_lock) was well tuned and almost 100% correct.
> Recent changes just made it worse (sorry Martin :) ).
> Now even if we add unmasking IRQs with disabling currently handled IRQ, it
> will be less friendlier to shared PCI interrupts (especially in PIO it
> will be overkill to disable shared IRQ for handling PIO intr!),
> so I want to revert to previous scheme...
Agreed there, thanks again for the patches.
Zwane Mwaikambo
--
http://function.linuxpower.ca
Thanks!
This patch solves my cd ripping problem I described yesterday at LKML
Bye
On Sun, 30 Jun 2002, Zwane Mwaikambo wrote:
> On Sun, 30 Jun 2002, Bartlomiej Zolnierkiewicz wrote:
>
> > > (1) ide-taskfile.c: ide_do_drive_cmd(..., ide_preempt) holds channel
> > > lock. Do not reacquire. NMI watchdog triggered by just booting
> > > computer with IDE cdrom.
> >
> > Mentioned in 95 changelog.
> > Already fixed in my tree, but thanks anyway.
>
> Hmm i just spent some time last night trying to go through possible
> paths for ide_do_drive_cmd to come up with a solution for that one, do you
> use some sort of SCM so that i can keep track of whats been covered?
Unfortunately no, I have only dialup...
> > Attached patch is next ide-clean patch pre-patch ;), just not to duplicate
> > efforts. Changelog is also included. As always use with care, standard
> > disclaimer apply.
>
> Thanks
>
> > And final note: I think that previous locking (2.4.x but ch->lock instead
> > of global io_request_lock) was well tuned and almost 100% correct.
> > Recent changes just made it worse (sorry Martin :) ).
> > Now even if we add unmasking IRQs with disabling currently handled IRQ, it
> > will be less friendlier to shared PCI interrupts (especially in PIO it
> > will be overkill to disable shared IRQ for handling PIO intr!),
> > so I want to revert to previous scheme...
>
> Agreed there, thanks again for the patches.
>
> Zwane Mwaikambo
>
> --
> http://function.linuxpower.ca
I will also forward You my reply to Petr, it shows my (correct?)
understanding of previous vs. actual IDE locking...
If you find any errors in thinking please let my now :)
Greets.
--
Bartlomiej