* [PATCH] atiixp: missing parentheses?
@ 2009-02-17 13:10 Roel Kluin
2009-02-17 14:25 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Roel Kluin @ 2009-02-17 13:10 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide, Andrew Morton
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 <roel.kluin@gmail.com>
---
diff --git a/drivers/ide/atiixp.c b/drivers/ide/atiixp.c
index b2735d2..b7813ec 100644
--- a/drivers/ide/atiixp.c
+++ b/drivers/ide/atiixp.c
@@ -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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] atiixp: missing parentheses?
2009-02-17 13:10 [PATCH] atiixp: missing parentheses? Roel Kluin
@ 2009-02-17 14:25 ` Sergei Shtylyov
2009-02-17 14:26 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2009-02-17 14:25 UTC (permalink / raw)
To: Roel Kluin; +Cc: Bartlomiej Zolnierkiewicz, linux-ide, Andrew Morton
Roel Kluin wrote:
> 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.
> int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 : 8);
> --------------------------->8-------------8<------------------------------
> Add missing parentheses
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> diff --git a/drivers/ide/atiixp.c b/drivers/ide/atiixp.c
> index b2735d2..b7813ec 100644
> --- a/drivers/ide/atiixp.c
> +++ b/drivers/ide/atiixp.c
> @@ -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;
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] atiixp: missing parentheses?
2009-02-17 14:25 ` Sergei Shtylyov
@ 2009-02-17 14:26 ` Sergei Shtylyov
2009-02-17 15:10 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2009-02-17 14:26 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Roel Kluin, Bartlomiej Zolnierkiewicz, linux-ide, Andrew Morton
I wrote:
>> 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.
>> int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 :
>> 8);
>> --------------------------->8-------------8<------------------------------
>>
>> Add missing parentheses
>
>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>
>
>> diff --git a/drivers/ide/atiixp.c b/drivers/ide/atiixp.c
>> index b2735d2..b7813ec 100644
>> --- a/drivers/ide/atiixp.c
>> +++ b/drivers/ide/atiixp.c
>> @@ -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;
>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] atiixp: missing parentheses?
2009-02-17 14:26 ` Sergei Shtylyov
@ 2009-02-17 15:10 ` Bartlomiej Zolnierkiewicz
2009-02-17 15:37 ` Roel Kluin
0 siblings, 1 reply; 6+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-02-17 15:10 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Roel Kluin, linux-ide, Andrew Morton
On Tuesday 17 February 2009, Sergei Shtylyov wrote:
> I wrote:
>
> >> 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.
>
> >> int timing_shift = ((drive->dn & 2) ? 16 : 0) + ((drive->dn & 1) ? 0 :
> >> 8);
> >> --------------------------->8-------------8<------------------------------
> >>
> >> Add missing parentheses
> >
> >
> >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >
> >
> >> diff --git a/drivers/ide/atiixp.c b/drivers/ide/atiixp.c
> >> index b2735d2..b7813ec 100644
> >> --- a/drivers/ide/atiixp.c
> >> +++ b/drivers/ide/atiixp.c
> >> @@ -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;
> >
> >
> > 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.
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] atiixp: missing parentheses?
2009-02-17 15:10 ` Bartlomiej Zolnierkiewicz
@ 2009-02-17 15:37 ` Roel Kluin
2009-02-18 18:53 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 6+ messages in thread
From: Roel Kluin @ 2009-02-17 15:37 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: Sergei Shtylyov, linux-ide, Andrew Morton
Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 17 February 2009, Sergei Shtylyov wrote:
>> I wrote:
>>
>>>> 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.
>>>> 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.
>> 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
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 <roel.kluin@gmail.com>
---
diff --git a/drivers/ide/atiixp.c b/drivers/ide/atiixp.c
index b2735d2..ecd1e62 100644
--- a/drivers/ide/atiixp.c
+++ b/drivers/ide/atiixp.c
@@ -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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] atiixp: missing parentheses?
2009-02-17 15:37 ` Roel Kluin
@ 2009-02-18 18:53 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 6+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2009-02-18 18:53 UTC (permalink / raw)
To: Roel Kluin; +Cc: Sergei Shtylyov, linux-ide, Andrew Morton
On Tuesday 17 February 2009, Roel Kluin wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 17 February 2009, Sergei Shtylyov wrote:
> >> I wrote:
> >>
> >>>> 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.
> >>>> 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.
>
> >> 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
> 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 <roel.kluin@gmail.com>
thanks, the applied version below:
From: Roel Kluin <roel.kluin@gmail.com>
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 <roel.kluin@gmail.com>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
[bart: update patch description]
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
drivers/ide/atiixp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: b/drivers/ide/atiixp.c
===================================================================
--- a/drivers/ide/atiixp.c
+++ 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-02-18 19:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-02-17 13:10 [PATCH] atiixp: missing parentheses? Roel Kluin
2009-02-17 14:25 ` Sergei Shtylyov
2009-02-17 14:26 ` Sergei Shtylyov
2009-02-17 15:10 ` Bartlomiej Zolnierkiewicz
2009-02-17 15:37 ` Roel Kluin
2009-02-18 18:53 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).