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
> vandrove@vc.cvut.cz
>
>
>
>
> 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:;
> }
> }
>
>
This archive was generated by hypermail 2b29 : Sun Jun 30 2002 - 22:00:14 EST