IDE PIO write Fix #2

PATCH In review tomita 5 participants 9 messages May 14, 2002 – May 20, 2002

Share & tools

tomita

author

2002-05-14 02:41 UTC #0 lore raw

PATCH 1/1 IDE PIO write Fix #2

Hi.
This patch solves problem (for me)
"kernel stops without message at heavy usage of IDE HDD". But, "hda: lost interrupt" message appears, instead. My BOX has both IDE and SCSI HDD.
This message appears at accessing SCSI HDD by another task, during IDE heavy accsess.
I guess, IDE driver has "critical section" needing "cli" (and so on).
Any suggestions ?

drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +1 -1

@@ -606,7 +606,7 @@
 		if (!ide_end_request(drive, rq, 1))
 			return ide_stopped;
 
-	if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+	if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {
 		pBuf = ide_map_rq(rq, &flags);
 		DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);

Martin Dalecki

2002-05-14 10:23 UTC #1 lore raw

Uz.ytkownik tomita napisa?:

Quoted 14 lines

Hi.
This patch solves problem (for me)
"kernel stops without message at heavy usage of IDE HDD". But, "hda: lost interrupt" message appears, instead. My BOX has both IDE and SCSI HDD.
This message appears at accessing SCSI HDD by another task, during IDE heavy accsess.
I guess, IDE driver has "critical section" needing "cli" (and so on).
Any suggestions ?

drivers/ide/ide-taskfile.c line 611

Quoted 7 lines

drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +1 -1

@@ -606,7 +606,7 @@
 		if (!ide_end_request(drive, rq, 1))
 			return ide_stopped;
 
-	if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+	if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {
 		pBuf = ide_map_rq(rq, &flags);
 		DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);

Hmm. There is something else that smells in the above, since the XOR operator doesn't seem to be proper. Why shouldn't we get DRQ_STAT at all on short request? Could you perhaps just try to replace it with an OR?

Thinking about the kernel hang and SCSI - I would have to ask whatever there is really interrupt sharing between IDE and SCSI. If the hangs happen due to the usage of ide-scsi then I already know what's going - ide-scsi doesn't do proper locking on command submission to the ATA layer.

Linus Torvalds

2002-05-14 19:30 UTC #2 lore raw

In reply to tomita

In article <[email protected]>,

Quoted 9 lines · Martin Dalecki

drivers/ide/ide-taskfile.c line 609
Quoted 9 lines
drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +1 -1
@@ -606,7 +606,7 @@
 		if (!ide_end_request(drive, rq, 1))
 			return ide_stopped;
 
-	if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+	if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {

Well, that's definitely an improvement - the original code makes no sense at all, since it's doing a bitwise xor on two bits that are not the same, and then uses that as a boolean value.

Your change at least makes it use the bitwise xor on properly logical values, making the bitwise xor work as a _logical_ xor.

Although at that point I'd just get rid of the xor, and replace it by the "!=" operation - which is equivalent on logical ops.

Quoted 7 lines

drivers/ide/ide-taskfile.c line 611
Quoted 2 lines
drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +0 -0
 		pBuf = ide_map_rq(rq, &flags);
 		DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
Hmm. There is something else that smells in the above, since the XOR operator doesn't seem to be proper. Why shouldn't we get DRQ_STAT at all on short request? Could you perhaps just try to replace it with an OR?

The XOR operation is a valid op, if you just use it on valid values, which the patch does seem to make it do.

I don't know whether the logic is _correct_ after that, but at least there is some remote chance that it might make sense.

Linus

Andre Hedrick

2002-05-15 05:50 UTC #3 lore raw

Quoted 30 lines · Linus Torvalds

In article <[email protected]>,
Quoted 9 lines · Martin Dalecki
drivers/ide/ide-taskfile.c line 609
Quoted 9 lines
drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +1 -1
@@ -606,7 +606,7 @@
 		if (!ide_end_request(drive, rq, 1))
 			return ide_stopped;
 
-	if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+	if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {
Well, that's definitely an improvement - the original code makes no sense at all, since it's doing a bitwise xor on two bits that are not the same, and then uses that as a boolean value.
Your change at least makes it use the bitwise xor on properly logical values, making the bitwise xor work as a _logical_ xor.
Although at that point I'd just get rid of the xor, and replace it by the "!=" operation - which is equivalent on logical ops.
Quoted 7 lines
drivers/ide/ide-taskfile.c line 611
Quoted 2 lines
drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +0 -0
 		pBuf = ide_map_rq(rq, &flags);
 		DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
Hmm. There is something else that smells in the above, since the XOR operator doesn't seem to be proper. Why shouldn't we get DRQ_STAT at all on short request? Could you perhaps just try to replace it with an OR?

Quoted 5 lines

The XOR operation is a valid op, if you just use it on valid values, which the patch does seem to make it do.
I don't know whether the logic is _correct_ after that, but at least there is some remote chance that it might make sense.

And here is the key why your solutions pio out was wrong in the past. You think like a geek and not like the hardware. But it is your^W the masses data you continue to screw over. Again you may be a wiz kid with a cool OS, but you are still clueless about storage.

Maybe you should learn from your mistakes.

Try removing the HBA hotrodding of the DMA engine.
If the hardware is not designed to run at the latest standard, sane people do not push it there. ATA 100 takes 4 clock where ATA 133 takes three clocks to move the same amount of data. I hope this is a big enough clue for you back out most of the experimental crap inserted. Now to be even more clear, the dropped data clock (ie data corruption lost) happens in both directions and at the same location on the trace. However, who cares it is a development kernel, lets continues to trash peoples data silently!

Later,

Andre Hedrick
LAD Storage Consulting Group

Martin Dalecki

2002-05-15 10:50 UTC #4 lore raw

In reply to tomita

Quoted 33 lines · Użytkownik Linus Torvalds

In article <[email protected]>,
Quoted 9 lines · Martin Dalecki
drivers/ide/ide-taskfile.c line 609
Quoted 8 lines
drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +1 -1
@@ -606,7 +606,7 @@
 		if (!ide_end_request(drive, rq, 1))
 			return ide_stopped;
 
-	if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+	if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {
Well, that's definitely an improvement - the original code makes no sense at all, since it's doing a bitwise xor on two bits that are not the same, and then uses that as a boolean value.
Your change at least makes it use the bitwise xor on properly logical values, making the bitwise xor work as a _logical_ xor.
Although at that point I'd just get rid of the xor, and replace it by the "!=" operation - which is equivalent on logical ops.
Quoted 7 lines
drivers/ide/ide-taskfile.c line 611
Quoted 2 lines
drivers/ide/ide-taskfile.c renamed from drivers/ide/ide-taskfile.c.orig +0 -0
 		pBuf = ide_map_rq(rq, &flags);
 		DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
Hmm. There is something else that smells in the above, since the XOR operator doesn't seem to be proper. Why shouldn't we get DRQ_STAT at all on short request? Could you perhaps just try to replace it with an OR?

Quoted 7 lines

The XOR operation is a valid op, if you just use it on valid values, which the patch does seem to make it do.
I don't know whether the logic is _correct_ after that, but at least there is some remote chance that it might make sense.
Linus

As far as I can see the patch makes sense. It is just exposing a problem which was hidden before.

Gunther Mayer

2002-05-17 20:24 UTC #5 lore raw

In reply to tomita

Quoted 37 lines · Martin Dalecki

U¿ytkownik Linus Torvalds napisa³:
Quoted 36 lines
In article <[email protected]>,
Quoted 9 lines · Martin Dalecki
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 line 609
Quoted 8 lines
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 renamed from drivers/ide/ide-taskfile.c.orig Fri May 10 11:49:35 2002 +1 -1
@@ -606,7 +606,7 @@
            if (!ide_end_request(drive, rq, 1))
                    return ide_stopped;
 
-    if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+    if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {
Well, that's definitely an improvement - the original code makes no sense at all, since it's doing a bitwise xor on two bits that are not the same, and then uses that as a boolean value.
Your change at least makes it use the bitwise xor on properly logical values, making the bitwise xor work as a _logical_ xor.
Although at that point I'd just get rid of the xor, and replace it by the "!=" operation - which is equivalent on logical ops.
Quoted 7 lines
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 line 611
Quoted 2 lines
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 renamed from drivers/ide/ide-taskfile.c.orig Fri May 10 11:49:35 2002 +0 -0
            pBuf = ide_map_rq(rq, &flags);
            DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
Hmm. There is something else that smells in the above, since the XOR operator doesn't seem to be proper. Why shouldn't we get DRQ_STAT at all on short request? Could you perhaps just try to replace it with an OR?
The XOR operation is a valid op, if you just use it on valid values, which the patch does seem to make it do.

Quoted 7 lines

Quoted 4 lines
I don't know whether the logic is _correct_ after that, but at least there is some remote chance that it might make sense.
Linus
As far as I can see the patch makes sense. It is just exposing a problem which was hidden before.

The original code:
a) if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
is being compared to these expressions:
b) logical equ. if ((rq->current_nr_sectors==1) || ((stat & DRQ_STAT)!=0)) { (me)

 c) not eqival.  if ((rq->current_nr_sectors==1) ^  ((stat & DRQ_STAT)!=0)) {  (tomita)
 d) same as c    if ((rq->current_nr_sectors==1) != ((stat & DRQ_STAT)!=0)) {  (linus)

Note: DRQ_STAT will be set when this routine is entered per "PIO Out protocol spec".

c+d will deadlock.
(it will _not_ send the last sector)

a+b will send the last sector to the drive when it isn't ready to receive.
(Although most of the time it _will_ be ready when we enter this routine)

So:
if( (stat & DRQ_STAT) && !(stat & BSY_STAT)) {
should be correct.

The original intent for a) was probably:
check DRQ only on the _first_ sector we transfer as
shown in "ATA-4 PIO data out command protocol", which would translate to:
e) if ( !(rq->are_we_transferring_the_first_sector_just_now) || ((stat & DRQ_STAT)!=0)) {

-
Gunther

Andre Hedrick

2002-05-20 01:42 UTC #6 lore raw

Hi Gunther,

If you isolate that single section of code you are correct! However taking into the entire state diagram handler, you are wrong.

Your issue of BUSY_STAT is addressed long before we even consider transferring a sector. Additionally since we exit the state diagram after the last interrupt and check status we have met the requirements. DRIVE_READY exclusive to BUSY_STAT.

Also note, my code base properly attempts to rewind the one whole sector on a media failure, this works since it is single sector transfers. Also note it is perfectly safe for partial completions and updating to the upper layers.

/*
* Handler for command with PIO data-out phase WRITE */
ide_startstop_t task_out_intr (ide_drive_t *drive)

{
        byte stat               = INB(drive, IDE_STATUS_REG);
        struct request *rq      = HWGROUP(drive)->rq;
        char *pBuf              = NULL;
        unsigned long flags;

        if (!OK_STAT(stat,DRIVE_READY,drive->bad_wstat)) {
                DTF("%s: WRITE attempting to recover last " \
                        "sector counter status=0x%02x\n",
                        drive->name, stat);
                rq->current_nr_sectors++;
                return DRIVER(drive)->error(drive, "task_out_intr", stat);
        }
        /*
         * Safe to update request for partial completions.
         * We have a good STATUS CHECK!!!
         */
        if (!rq->current_nr_sectors)
                if (!DRIVER(drive)->end_request(drive, 1))
                        return ide_stopped;
        if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
                rq = HWGROUP(drive)->rq;
                pBuf = task_map_rq(rq, &flags);
                DTF("write: %p, rq->current_nr_sectors: %d\n",
                        pBuf, (int) rq->current_nr_sectors);
//              rq->current_nr_sectors--;
                taskfile_output_data(drive, pBuf, SECTOR_WORDS);
                task_unmap_rq(rq, pBuf, &flags);
                rq->errors = 0;
                rq->current_nr_sectors--;
        }
        if (HWGROUP(drive)->handler == NULL)
                ide_set_handler(drive, &task_out_intr, WAIT_CMD, NULL);
        return ide_started;
}

Cheers,

Andre Hedrick
LAD Storage Consulting Group

PS Gunther, my apology for the contents in the last email we transacted.

Quoted 80 lines · Gunther Mayer

Quoted 37 lines · Martin Dalecki
U¿ytkownik Linus Torvalds napisa³:
Quoted 36 lines
In article <[email protected]>,
Quoted 9 lines · Martin Dalecki
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 line 609
Quoted 8 lines
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 renamed from drivers/ide/ide-taskfile.c.orig Fri May 10 11:49:35 2002 +1 -1
@@ -606,7 +606,7 @@
            if (!ide_end_request(drive, rq, 1))
                    return ide_stopped;
 
-    if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
+    if ((rq->nr_sectors == 1) ^ ((stat & DRQ_STAT) != 0)) {
Well, that's definitely an improvement - the original code makes no sense at all, since it's doing a bitwise xor on two bits that are not the same, and then uses that as a boolean value.
Your change at least makes it use the bitwise xor on properly logical values, making the bitwise xor work as a _logical_ xor.
Although at that point I'd just get rid of the xor, and replace it by the "!=" operation - which is equivalent on logical ops.
Quoted 7 lines
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 line 611
Quoted 2 lines
drivers/ide/ide-taskfile.c Tue May 14 10:40:43 2002 renamed from drivers/ide/ide-taskfile.c.orig Fri May 10 11:49:35 2002 +0 -0
            pBuf = ide_map_rq(rq, &flags);
            DTF("write: %p, rq->current_nr_sectors: %d\n", pBuf, (int) rq->current_nr_sectors);
Hmm. There is something else that smells in the above, since the XOR operator doesn't seem to be proper. Why shouldn't we get DRQ_STAT at all on short request? Could you perhaps just try to replace it with an OR?
The XOR operation is a valid op, if you just use it on valid values, which the patch does seem to make it do.
Quoted 7 lines
Quoted 4 lines
I don't know whether the logic is _correct_ after that, but at least there is some remote chance that it might make sense.
Linus
As far as I can see the patch makes sense. It is just exposing a problem which was hidden before.
The original code:
a) if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {
is being compared to these expressions:
b) logical equ. if ((rq->current_nr_sectors==1) || ((stat & DRQ_STAT)!=0)) { (me)
 c) not eqival.  if ((rq->current_nr_sectors==1) ^  ((stat & DRQ_STAT)!=0)) {  (tomita)
 d) same as c    if ((rq->current_nr_sectors==1) != ((stat & DRQ_STAT)!=0)) {  (linus)
Note: DRQ_STAT will be set when this routine is entered per "PIO Out protocol spec".
c+d will deadlock.
(it will _not_ send the last sector)
a+b will send the last sector to the drive when it isn't ready to receive.
(Although most of the time it _will_ be ready when we enter this routine)
So:
   if( (stat & DRQ_STAT) && !(stat & BSY_STAT)) {
should be correct.
The original intent for a) was probably:
check DRQ only on the _first_ sector we transfer as
shown in "ATA-4 PIO data out command protocol", which would translate to:
e) if ( !(rq->are_we_transferring_the_first_sector_just_now) || ((stat & DRQ_STAT)!=0)) {
-
Gunther
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Gunther Mayer

2002-05-20 07:48 UTC #7 lore raw

Hi Andre,

1)
I follow your arguments regarding BUSY_STAT handled earlier.

2)
Can you explain, why the code looks at "rq->current_nr_sectors==1" ? In ATA-4 there is no special handling for "single-sector-transfer" or "last-sector-transfer".

Quoted 41 lines · Andre Hedrick

Hi Gunther,
If you isolate that single section of code you are correct! However taking into the entire state diagram handler, you are wrong.
Your issue of BUSY_STAT is addressed long before we even consider transferring a sector. Additionally since we exit the state diagram after the last interrupt and check status we have met the requirements. DRIVE_READY exclusive to BUSY_STAT.
Also note, my code base properly attempts to rewind the one whole sector on a media failure, this works since it is single sector transfers. Also note it is perfectly safe for partial completions and updating to the upper layers.
/*
* Handler for command with PIO data-out phase WRITE */
ide_startstop_t task_out_intr (ide_drive_t *drive)
{
        byte stat               = INB(drive, IDE_STATUS_REG);
        struct request *rq      = HWGROUP(drive)->rq;
        char *pBuf              = NULL;
        unsigned long flags;

        if (!OK_STAT(stat,DRIVE_READY,drive->bad_wstat)) {
                DTF("%s: WRITE attempting to recover last " \
                        "sector counter status=0x%02x\n",
                        drive->name, stat);
                rq->current_nr_sectors++;
                return DRIVER(drive)->error(drive, "task_out_intr", stat);
        }
        /*
         * Safe to update request for partial completions.
         * We have a good STATUS CHECK!!!
         */
        if (!rq->current_nr_sectors)
                if (!DRIVER(drive)->end_request(drive, 1))
                        return ide_stopped;
        if ((rq->current_nr_sectors==1) ^ (stat & DRQ_STAT)) {

...

Andre Hedrick

2002-05-20 20:27 UTC #8 lore raw

Quoted 9 lines · Gunther Mayer

Hi Andre,
1)
I follow your arguments regarding BUSY_STAT handled earlier.
2)
Can you explain, why the code looks at "rq->current_nr_sectors==1" ? In ATA-4 there is no special handling for "single-sector-transfer" or "last-sector-transfer".

There is a test just not clearly stated, "data block transferred". It is hidden in the state diagram in the transition states. Since there is no accounting process in the driver, as it has been exported to block because of partial completions and page walking.

But obviously most people question my proven judgement on how things work, so I expect this to be ignored by the masses on lkml. However you I expect to see the point and issues.

Cheers,

Andre Hedrick
LAD Storage Consulting Group