Hi,
The blacklist stuff is broken. When set_using_dma() calls into
ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This
of course succeeds, returning success to the caller of ide_dma_check().
Not so good... It then uncondtionally calls ide_dma_on(), which turns on
dma for the drive.
This moves the check to ide_dma_on() so we also catch the buggy
->ide_dma_check() defined by various chipset drivers.
--- drivers/ide/ide-dma.c~ 2004-10-05 16:11:49.631910586 +0200
+++ drivers/ide/ide-dma.c 2004-10-05 16:21:58.828330845 +0200
@@ -354,11 +355,13 @@
struct hd_driveid *id = drive->id;
ide_hwif_t *hwif = HWIF(drive);
- if ((id->capability & 1) && hwif->autodma) {
- /* Consult the list of known "bad" drives */
- if (__ide_dma_bad_drive(drive))
- return __ide_dma_off(drive);
+ /* Consult the list of known "bad" drives */
+ if (__ide_dma_bad_drive(drive)) {
+ __ide_dma_off(drive);
+ return 1;
+ }
+ if ((id->capability & 1) && hwif->autodma) {
/*
* Enable DMA on any drive that has
* UltraDMA (mode 0/1/2/3/4/5/6) enabled
@@ -512,6 +515,9 @@
int __ide_dma_on (ide_drive_t *drive)
{
+ if (__ide_dma_bad_drive(drive))
+ return 1;
+
drive->using_dma = 1;
ide_toggle_bounce(drive, 1);
--
Jens Axboe
On Tue, Oct 05, 2004 at 04:20:01PM +0200, Jens Axboe wrote:
> Hi,
>
> The blacklist stuff is broken. When set_using_dma() calls into
> ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This
> of course succeeds, returning success to the caller of ide_dma_check().
> Not so good... It then uncondtionally calls ide_dma_on(), which turns on
> dma for the drive.
>
> This moves the check to ide_dma_on() so we also catch the buggy
> ->ide_dma_check() defined by various chipset drivers.
Is this a bug introduced in the 2.6.9ish IDE changes or has it been there
for a longer time?
On Tue, Oct 05 2004, Christoph Hellwig wrote:
> On Tue, Oct 05, 2004 at 04:20:01PM +0200, Jens Axboe wrote:
> > Hi,
> >
> > The blacklist stuff is broken. When set_using_dma() calls into
> > ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This
> > of course succeeds, returning success to the caller of ide_dma_check().
> > Not so good... It then uncondtionally calls ide_dma_on(), which turns on
> > dma for the drive.
> >
> > This moves the check to ide_dma_on() so we also catch the buggy
> > ->ide_dma_check() defined by various chipset drivers.
>
> Is this a bug introduced in the 2.6.9ish IDE changes or has it been there
> for a longer time?
I didn't check, someone just reported today. But looking at eg 2.6.5, it
seems to have the same bug. So it's likely very old.
--
Jens Axboe
Christoph Hellwig wrote:
> On Tue, Oct 05, 2004 at 04:20:01PM +0200, Jens Axboe wrote:
>> Hi,
>>
>> The blacklist stuff is broken. When set_using_dma() calls into
>> ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This
>> of course succeeds, returning success to the caller of ide_dma_check().
>> Not so good... It then uncondtionally calls ide_dma_on(), which turns on
>> dma for the drive.
>>
>> This moves the check to ide_dma_on() so we also catch the buggy
>> ->ide_dma_check() defined by various chipset drivers.
>
> Is this a bug introduced in the 2.6.9ish IDE changes or has it been there
> for a longer time?
Looks like it is also in 2.4.27.
Juri
On Maw, 2004-10-05 at 16:46, Jens Axboe wrote:
> I didn't check, someone just reported today. But looking at eg 2.6.5, it
> seems to have the same bug. So it's likely very old.
We should actually probably nuke most of the IDE blacklist, much of the
CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.
On Tue, Oct 05 2004, Alan Cox wrote:
> On Maw, 2004-10-05 at 16:46, Jens Axboe wrote:
> > I didn't check, someone just reported today. But looking at eg 2.6.5, it
> > seems to have the same bug. So it's likely very old.
>
> We should actually probably nuke most of the IDE blacklist, much of the
> CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.
Hmm? When have we ever done that?
--
Jens Axboe
On Mer, 2004-10-06 at 06:45, Jens Axboe wrote:
> > We should actually probably nuke most of the IDE blacklist, much of the
> > CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.
>
> Hmm? When have we ever done that?
2.0, 2.2, 2.4 to about 2.4.18 or so (Khalid Aziz eventually pinned it
down and fixed it).
On Wed, Oct 06 2004, Alan Cox wrote:
> On Mer, 2004-10-06 at 06:45, Jens Axboe wrote:
> > > We should actually probably nuke most of the IDE blacklist, much of the
> > > CD-ROM blacklist arose because we DMA rather than PIO'd the ATAPI CDB.
> >
> > Hmm? When have we ever done that?
>
> 2.0, 2.2, 2.4 to about 2.4.18 or so (Khalid Aziz eventually pinned it
> down and fixed it).
Ah, I think you are misreading it. It wasn't the DMA'ing of the atapi
cdb, that was always pio'ed to the drive. But DMA for the command itself
was turned on before the cdb had been transferred.
--
Jens Axboe
On Mer, 2004-10-06 at 15:41, Jens Axboe wrote:
> Ah, I think you are misreading it. It wasn't the DMA'ing of the atapi
> cdb, that was always pio'ed to the drive. But DMA for the command itself
> was turned on before the cdb had been transferred.
Yep. I may have the detail wrong, its a long time ago. That fixed
several CD's that were on the blacklist and most of the others may well
never have been tested.
On Wed, Oct 06 2004, Alan Cox wrote:
> On Mer, 2004-10-06 at 15:41, Jens Axboe wrote:
> > Ah, I think you are misreading it. It wasn't the DMA'ing of the atapi
> > cdb, that was always pio'ed to the drive. But DMA for the command itself
> > was turned on before the cdb had been transferred.
>
> Yep. I may have the detail wrong, its a long time ago. That fixed
> several CD's that were on the blacklist and most of the others may well
> never have been tested.
Yeah, it was a very nasty bug. So do you suggest we try and scrap the
atapi drives from the blacklist?
--
Jens Axboe
[ linux-ide MIA ]
On Tuesday 05 October 2004 16:20, Jens Axboe wrote:
> Hi,
>
> The blacklist stuff is broken. When set_using_dma() calls into
> ide_dma_check(), it returns ide_dma_off() for a blacklisted drive. This
> of course succeeds, returning success to the caller of ide_dma_check().
> Not so good... It then uncondtionally calls ide_dma_on(), which turns on
> dma for the drive.
- s/ide_dma_check/->ide_dma_check/
- s/ide_dma_off/__ide_dma_off/
> This moves the check to ide_dma_on() so we also catch the buggy
> ->ide_dma_check() defined by various chipset drivers.
Yep, good catch.
> --- drivers/ide/ide-dma.c~ 2004-10-05 16:11:49.631910586 +0200
> +++ drivers/ide/ide-dma.c 2004-10-05 16:21:58.828330845 +0200
> @@ -354,11 +355,13 @@
> struct hd_driveid *id = drive->id;
> ide_hwif_t *hwif = HWIF(drive);
>
> - if ((id->capability & 1) && hwif->autodma) {
> - /* Consult the list of known "bad" drives */
> - if (__ide_dma_bad_drive(drive))
> - return __ide_dma_off(drive);
> + /* Consult the list of known "bad" drives */
> + if (__ide_dma_bad_drive(drive)) {
> + __ide_dma_off(drive);
> + return 1;
> + }
>
> + if ((id->capability & 1) && hwif->autodma) {
> /*
> * Enable DMA on any drive that has
> * UltraDMA (mode 0/1/2/3/4/5/6) enabled
Is __ide_dma_bad_drive() check still needed?
Doesn't __ide_dma_on() fix handle this now?
> @@ -512,6 +515,9 @@
>
> int __ide_dma_on (ide_drive_t *drive)
> {
> + if (__ide_dma_bad_drive(drive))
> + return 1;
> +
> drive->using_dma = 1;
> ide_toggle_bounce(drive, 1);
>
On Sun, Oct 10 2004, Bartlomiej Zolnierkiewicz wrote:
> > --- drivers/ide/ide-dma.c~ 2004-10-05 16:11:49.631910586 +0200
> > +++ drivers/ide/ide-dma.c 2004-10-05 16:21:58.828330845 +0200
> > @@ -354,11 +355,13 @@
> > struct hd_driveid *id = drive->id;
> > ide_hwif_t *hwif = HWIF(drive);
> >
> > - if ((id->capability & 1) && hwif->autodma) {
> > - /* Consult the list of known "bad" drives */
> > - if (__ide_dma_bad_drive(drive))
> > - return __ide_dma_off(drive);
> > + /* Consult the list of known "bad" drives */
> > + if (__ide_dma_bad_drive(drive)) {
> > + __ide_dma_off(drive);
> > + return 1;
> > + }
> >
> > + if ((id->capability & 1) && hwif->autodma) {
> > /*
> > * Enable DMA on any drive that has
> > * UltraDMA (mode 0/1/2/3/4/5/6) enabled
>
> Is __ide_dma_bad_drive() check still needed?
> Doesn't __ide_dma_on() fix handle this now?
Yeah, we can solely rely on that if we so wish.
--
Jens Axboe