atiixp: missing parentheses?

PATCH In review Roel Kluin 3 participants 6 messages Feb 17, 2009 – Feb 18, 2009 libata

Share & tools

Roel Kluin

author

2009-02-17 13:10 UTC #0 lore raw

PATCH 1/1 atiixp: missing parentheses?

This looks odd, is below what was intended? note that timing_shift can only be 16, 8 or 0. Maybe intended was 24, 16, 8 or 0? then we should do:

int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 : 8); --------------------------->8-------------8<------------------------------ Add missing parentheses

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ide/atiixp.c +2 -2

index b2735d2..b7813ec 100644
@@ -52,7 +52,7 @@ static void atiixp_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 0 : 8);
 	u32 pio_timing_data;
 	u16 pio_mode_data;

@@ -85,7 +85,7 @@ static void atiixp_set_dma_mode(ide_drive_t *drive, const u8 speed)
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 0 : 8);
 	u32 tmp32;
 	u16 tmp16;
 	u16 udma_ctl = 0;

Sergei Shtylyov

2009-02-17 14:25 UTC #1 lore raw

Quoted 2 lines · Roel Kluin

This looks odd, is below what was intended? note that timing_shift can only be 16, 8 or 0. Maybe intended was 24, 16, 8 or 0? then we should do:

Looking at the libata driver, yes, it should be 0, 8, 16, and 24.

Quoted 20 lines

int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 : 8); --------------------------->8-------------8<------------------------------ Add missing parentheses
Signed-off-by Roel Kluin <[email protected]>
drivers/ide/atiixp.c +1 -1
index b2735d2..b7813ec 100644
@@ -52,7 +52,7 @@ static void atiixp_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 0 : 8);
 	u32 pio_timing_data;
 	u16 pio_mode_data;
 

drivers/ide/atiixp.c static void atiixp_set_dma_mode(ide_drive_t *drive, const u8 speed) · line 91

Quoted 8 lines

drivers/ide/atiixp.c +1 -1

@@ -85,7 +85,7 @@ static void atiixp_set_dma_mode(ide_drive_t *drive, const u8 speed)
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 0 : 8);
 	u32 tmp32;
 	u16 tmp16;
 	u16 udma_ctl = 0;

Why didn't you add parens around the first ?: then? They are surely
needed. Though this clearly needs to be simplified to (drive->dn * 8).

MBR, Sergei

Sergei Shtylyov

2009-02-17 14:26 UTC #2 lore raw

Quoted 30 lines · I wrote:

Quoted 2 lines
This looks odd, is below what was intended? note that timing_shift can only be 16, 8 or 0. Maybe intended was 24, 16, 8 or 0? then we should do:
Looking at the libata driver, yes, it should be 0, 8, 16, and 24.
Quoted 25 lines
int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 : 8); --------------------------->8-------------8<------------------------------
Add missing parentheses
Signed-off-by Roel Kluin <[email protected]>
drivers/ide/atiixp.c +1 -1
index b2735d2..b7813ec 100644
@@ -52,7 +52,7 @@ static void atiixp_set_pio_mode(ide_drive_t *drive, 
 const u8 pio)
 {
     struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
     unsigned long flags;
-    int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 
 : 8;
+    int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 
 0 : 8);
     u32 pio_timing_data;
     u16 pio_mode_data;

Quoted 16 lines

Quoted 9 lines

drivers/ide/atiixp.c +1 -1

@@ -85,7 +85,7 @@ static void atiixp_set_dma_mode(ide_drive_t *drive, 
 const u8 speed)
 {
     struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
     unsigned long flags;
-    int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 
 : 8;
+    int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 
 0 : 8);

Quoted 3 lines

     u32 tmp32;
     u16 tmp16;
     u16 udma_ctl = 0;

Why didn't you add parens around the first ?: then? They are surely
needed. Though this clearly needs to be simplified to (drive->dn * 8).

Oh, actually, timing_shift = (drive->dn ^ 1) * 8.

MBR, Sergei

Bartlomiej Zolnierkiewicz

2009-02-17 15:09 UTC #3 lore raw

Quoted 50 lines · Sergei Shtylyov

Quoted 31 lines · I wrote:
Quoted 2 lines
This looks odd, is below what was intended? note that timing_shift can only be 16, 8 or 0. Maybe intended was 24, 16, 8 or 0? then we should do:
Looking at the libata driver, yes, it should be 0, 8, 16, and 24.
Quoted 26 lines
int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 :
8); --------------------------->8-------------8<------------------------------
Add missing parentheses
Signed-off-by Roel Kluin <[email protected]>
drivers/ide/atiixp.c +1 -1
index b2735d2..b7813ec 100644
@@ -52,7 +52,7 @@ static void atiixp_set_pio_mode(ide_drive_t *drive, 
 const u8 pio)
 {
     struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
     unsigned long flags;
-    int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 
 : 8;
+    int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 
 0 : 8);
     u32 pio_timing_data;
     u16 pio_mode_data;
Quoted 16 lines
Quoted 9 lines
drivers/ide/atiixp.c +1 -1
@@ -85,7 +85,7 @@ static void atiixp_set_dma_mode(ide_drive_t *drive, 
 const u8 speed)
 {
     struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
     unsigned long flags;
-    int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 
 : 8;
+    int timing_shift = (drive->dn & 2) ? 16 : 0 + ((drive->dn & 1) ? 
 0 : 8);
Quoted 3 lines
     u32 tmp32;
     u16 tmp16;
     u16 udma_ctl = 0;
Why didn't you add parens around the first ?: then? They are surely
needed. Though this clearly needs to be simplified to (drive->dn * 8).

Quoted 1 line

    Oh, actually, timing_shift = (drive->dn ^ 1) * 8.

Yes, this way we'll get the correct:

8 0 24 16

instead of the current

8 0 16 16

[ Luckily after DMA timings gets programmed for the third device to
offset 16 they get overwritten with DMA timings for fourth device.

Since BIOS should program DMA timings for the third device all should work fine till suspend/resume... ]

Roel, please fix the patch and resubmit.

Thanks,
Bart

Roel Kluin

author

2009-02-17 15:37 UTC #4 lore raw

Quoted 11 lines · Bartlomiej Zolnierkiewicz

Quoted 10 lines · Sergei Shtylyov
Quoted 8 lines · I wrote:
Quoted 2 lines
This looks odd, is below what was intended? note that timing_shift can only be 16, 8 or 0. Maybe intended was 24, 16, 8 or 0? then we should do:
Looking at the libata driver, yes, it should be 0, 8, 16, and 24.
Quoted 2 lines
int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 :
8);
Why didn't you add parens around the first ?: then? They are surely
needed.

I was unsure what was intended so I chose to keep semantics, but wanted to show the oddity. Thankfully you were able to clarify that it's a bug.

Quoted 15 lines

Quoted 1 line
    Oh, actually, timing_shift = (drive->dn ^ 1) * 8.
Yes, this way we'll get the correct:
8 0 24 16
instead of the current
8 0 16 16
[ Luckily after DMA timings gets programmed for the third device to
offset 16 they get overwritten with DMA timings for fourth device.

Quoted 7 lines

Since BIOS should program DMA timings for the third device all should work fine till suspend/resume... ]
Roel, please fix the patch and resubmit.
Thanks,
Bart

I am not sure what to write for the changelog, so I kept your remarks. Feel free to change them if necessary. --------------------------->8-------------8<------------------------------ DMA timings were incorrect. this way we'll get the correct:

8 0 24 16

instead of the current

8 0 16 16

Luckily after DMA timings get programmed for the third device to offset 16 they get overwritten with DMA timings for fourth device.

Since BIOS should program DMA timings for the third device all should work fine till suspend/resume...

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ide/atiixp.c +2 -2

index b2735d2..ecd1e62 100644
@@ -52,7 +52,7 @@ static void atiixp_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn ^ 1) * 8;
 	u32 pio_timing_data;
 	u16 pio_mode_data;

@@ -85,7 +85,7 @@ static void atiixp_set_dma_mode(ide_drive_t *drive, const u8 speed)
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn ^ 1) * 8;
 	u32 tmp32;
 	u16 tmp16;
 	u16 udma_ctl = 0;

Bartlomiej Zolnierkiewicz

2009-02-18 19:05 UTC #5 lore raw

Quoted 49 lines · Roel Kluin

Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 10 lines · Sergei Shtylyov
Quoted 8 lines · I wrote:
Quoted 2 lines
This looks odd, is below what was intended? note that timing_shift can only be 16, 8 or 0. Maybe intended was 24, 16, 8 or 0? then we should do:
Looking at the libata driver, yes, it should be 0, 8, 16, and 24.
Quoted 2 lines
int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 :
8);
Why didn't you add parens around the first ?: then? They are surely
needed.
I was unsure what was intended so I chose to keep semantics, but wanted to show the oddity. Thankfully you were able to clarify that it's a bug.
Quoted 15 lines
Quoted 1 line
    Oh, actually, timing_shift = (drive->dn ^ 1) * 8.
Yes, this way we'll get the correct:
8 0 24 16
instead of the current
8 0 16 16
[ Luckily after DMA timings gets programmed for the third device to
offset 16 they get overwritten with DMA timings for fourth device.
Quoted 7 lines
Since BIOS should program DMA timings for the third device all should work fine till suspend/resume... ]
Roel, please fix the patch and resubmit.
Thanks,
Bart
I am not sure what to write for the changelog, so I kept your remarks. Feel free to change them if necessary. --------------------------->8-------------8<------------------------------ DMA timings were incorrect. this way we'll get the correct:
8 0 24 16
instead of the current
8 0 16 16

Quoted 7 lines

Luckily after DMA timings get programmed for the third device to offset 16 they get overwritten with DMA timings for fourth device.
Since BIOS should program DMA timings for the third device all should work fine till suspend/resume...
Signed-off-by Roel Kluin <[email protected]>

thanks, the applied version below:

From: Roel Kluin <[email protected]>
Subject: [PATCH] atiixp: fix missing parentheses

Fix missing parentheses so PIO/DMA timings for master device on the second channel are programmed correctly (IOW "8 0 24 16" offset values should be used instead of the current "8 0 16 16").

[ The bug went unnoticed because after PIO/DMA timings get programmed
incorrectly for the third device they are overwritten with timings for the fourth device and since BIOS should also program timings for the third device everything should work fine until suspend/resume cycle or user requested transfer mode changes. ]

Signed-off-by Roel Kluin <[email protected]>

Cc Sergei Shtylyov <[email protected]>

Cc Andrew Morton <[email protected]>

[bart: update patch description]

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/atiixp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/ide/atiixp.c +2 -2

Index: b/drivers/ide/atiixp.c
===================================================================
@@ -52,7 +52,7 @@ static void atiixp_set_pio_mode(ide_driv
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn ^ 1) * 8;
 	u32 pio_timing_data;
 	u16 pio_mode_data;

@@ -85,7 +85,7 @@ static void atiixp_set_dma_mode(ide_driv
 {
 	struct pci_dev *dev = to_pci_dev(drive->hwif->dev);
 	unsigned long flags;
-	int timing_shift = (drive->dn & 2) ? 16 : 0 + (drive->dn & 1) ? 0 : 8;
+	int timing_shift = (drive->dn ^ 1) * 8;
 	u32 tmp32;
 	u16 tmp16;
 	u16 udma_ctl = 0;