* [PATCH] ide_dma_speed() fixes
@ 2006-05-12 2:36 Sergei Shtylyov
2006-05-14 12:05 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2006-05-12 2:36 UTC (permalink / raw)
To: Andrew Morton; +Cc: Bartlomiej Zolnierkiewicz, Linux IDE, LKML
[-- Attachment #1: Type: text/plain, Size: 416 bytes --]
Hello.
ide_dma_speed() fails to actually honor the IDE drivers' mode support
masks) because of the bogus checks -- thus, selecting the DMA transfer mode
that the driver explicitly refuses to support is possible. Additionally, there
is no check for validity of the UltraDMA mode data in the drive ID, and the
function is misdocumented.
MBR, Sergei
Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
[-- Attachment #2: ide_dma_speed-fix.patch --]
[-- Type: text/plain, Size: 3165 bytes --]
Index: linus/drivers/ide/ide-lib.c
===================================================================
--- linus.orig/drivers/ide/ide-lib.c
+++ linus/drivers/ide/ide-lib.c
@@ -72,66 +72,65 @@ EXPORT_SYMBOL(ide_xfer_verbose);
/**
* ide_dma_speed - compute DMA speed
* @drive: drive
- * @mode; intended mode
+ * @mode: modes available
*
* Checks the drive capabilities and returns the speed to use
- * for the transfer. Returns -1 if the requested mode is unknown
- * (eg PIO)
+ * for the DMA transfer. Returns 0 if the drive is incapable
+ * of DMA transfers.
*/
u8 ide_dma_speed(ide_drive_t *drive, u8 mode)
{
struct hd_driveid *id = drive->id;
ide_hwif_t *hwif = HWIF(drive);
+ u8 ultra_mask, mwdma_mask, swdma_mask;
u8 speed = 0;
if (drive->media != ide_disk && hwif->atapi_dma == 0)
return 0;
- switch(mode) {
- case 0x04:
- if ((id->dma_ultra & 0x0040) &&
- (id->dma_ultra & hwif->ultra_mask))
+ /* Capable of UltraDMA modes? */
+ if (id->field_valid & 4)
+ ultra_mask = id->dma_ultra & hwif->ultra_mask;
+ else
+ mode = 0; /* fallback to MW/SW DMA if no UltraDMA */
+
+ switch (mode) {
+ case 4:
+ if (ultra_mask & 0x40)
{ speed = XFER_UDMA_6; break; }
- case 0x03:
- if ((id->dma_ultra & 0x0020) &&
- (id->dma_ultra & hwif->ultra_mask))
+ case 3:
+ if (ultra_mask & 0x20)
{ speed = XFER_UDMA_5; break; }
- case 0x02:
- if ((id->dma_ultra & 0x0010) &&
- (id->dma_ultra & hwif->ultra_mask))
+ case 2:
+ if (ultra_mask & 0x10)
{ speed = XFER_UDMA_4; break; }
- if ((id->dma_ultra & 0x0008) &&
- (id->dma_ultra & hwif->ultra_mask))
+ if (ultra_mask & 0x08)
{ speed = XFER_UDMA_3; break; }
- case 0x01:
- if ((id->dma_ultra & 0x0004) &&
- (id->dma_ultra & hwif->ultra_mask))
+ case 1:
+ if (ultra_mask & 0x04)
{ speed = XFER_UDMA_2; break; }
- if ((id->dma_ultra & 0x0002) &&
- (id->dma_ultra & hwif->ultra_mask))
+ if (ultra_mask & 0x02)
{ speed = XFER_UDMA_1; break; }
- if ((id->dma_ultra & 0x0001) &&
- (id->dma_ultra & hwif->ultra_mask))
+ if (ultra_mask & 0x01)
{ speed = XFER_UDMA_0; break; }
- case 0x00:
- if ((id->dma_mword & 0x0004) &&
- (id->dma_mword & hwif->mwdma_mask))
+ case 0:
+ mwdma_mask = id->dma_mword & hwif->mwdma_mask;
+
+ if (mwdma_mask & 0x04)
{ speed = XFER_MW_DMA_2; break; }
- if ((id->dma_mword & 0x0002) &&
- (id->dma_mword & hwif->mwdma_mask))
+ if (mwdma_mask & 0x02)
{ speed = XFER_MW_DMA_1; break; }
- if ((id->dma_mword & 0x0001) &&
- (id->dma_mword & hwif->mwdma_mask))
+ if (mwdma_mask & 0x01)
{ speed = XFER_MW_DMA_0; break; }
- if ((id->dma_1word & 0x0004) &&
- (id->dma_1word & hwif->swdma_mask))
+
+ swdma_mask = id->dma_1word & hwif->swdma_mask;
+
+ if (swdma_mask & 0x04)
{ speed = XFER_SW_DMA_2; break; }
- if ((id->dma_1word & 0x0002) &&
- (id->dma_1word & hwif->swdma_mask))
+ if (swdma_mask & 0x02)
{ speed = XFER_SW_DMA_1; break; }
- if ((id->dma_1word & 0x0001) &&
- (id->dma_1word & hwif->swdma_mask))
+ if (swdma_mask & 0x01)
{ speed = XFER_SW_DMA_0; break; }
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ide_dma_speed() fixes
2006-05-12 2:36 [PATCH] ide_dma_speed() fixes Sergei Shtylyov
@ 2006-05-14 12:05 ` Andrew Morton
2006-05-15 13:44 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-05-14 12:05 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: bzolnier, linux-ide, linux-kernel
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
>
> ide_dma_speed() fails to actually honor the IDE drivers' mode support
> masks) because of the bogus checks -- thus, selecting the DMA transfer mode
> that the driver explicitly refuses to support is possible. Additionally, there
> is no check for validity of the UltraDMA mode data in the drive ID, and the
> function is misdocumented.
drivers/ide/ide-lib.c: In function `ide_dma_speed':
drivers/ide/ide-lib.c:86: warning: `ultra_mask' might be used uninitialized in this function
Looks like a real bug to me - it depends up on the values of `mode' and
id->field_value.
Anyway, I'll drop it, please review and fix. I assume that warning was
occurring for you as well - please spend more time over these things.
Especially when working on IDE, where bugs are slow to show themselves and
have particularly bad consequences.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ide_dma_speed() fixes
2006-05-14 12:05 ` Andrew Morton
@ 2006-05-15 13:44 ` Sergei Shtylyov
2006-05-15 14:08 ` Alan Cox
2006-05-15 15:00 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2006-05-15 13:44 UTC (permalink / raw)
To: Andrew Morton; +Cc: bzolnier, linux-ide, linux-kernel
Hello.
Andrew Morton wrote:
>> ide_dma_speed() fails to actually honor the IDE drivers' mode support
>> masks) because of the bogus checks -- thus, selecting the DMA transfer mode
>> that the driver explicitly refuses to support is possible. Additionally, there
>> is no check for validity of the UltraDMA mode data in the drive ID, and the
>> function is misdocumented.
> drivers/ide/ide-lib.c: In function `ide_dma_speed':
> drivers/ide/ide-lib.c:86: warning: `ultra_mask' might be used uninitialized in this function
> Looks like a real bug to me - it depends up on the values of `mode' and
> id->field_value.
> Anyway, I'll drop it, please review and fix. I assume that warning was
> occurring for you as well - please spend more time over these things.
> Especially when working on IDE, where bugs are slow to show themselves and
> have particularly bad consequences.
That's what gcc thinks. The code is 100% correct -- it will never reach
the switch statement with mode > 0 (in which case ultra_mask isn't used) and
ultra_mask unitialized. I may add an explicit initializer in the declaration
if you like...
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ide_dma_speed() fixes
2006-05-15 13:44 ` Sergei Shtylyov
@ 2006-05-15 14:08 ` Alan Cox
2006-05-15 15:00 ` Andrew Morton
1 sibling, 0 replies; 6+ messages in thread
From: Alan Cox @ 2006-05-15 14:08 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Andrew Morton, bzolnier, linux-ide, linux-kernel
On Llu, 2006-05-15 at 17:44 +0400, Sergei Shtylyov wrote:
> That's what gcc thinks. The code is 100% correct -- it will never reach
> the switch statement with mode > 0 (in which case ultra_mask isn't used) and
> ultra_mask unitialized. I may add an explicit initializer in the declaration
> if you like...
Please leave the warning, this was discussed recently in other threads
and hiding warnings is very dangerous.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ide_dma_speed() fixes
2006-05-15 13:44 ` Sergei Shtylyov
2006-05-15 14:08 ` Alan Cox
@ 2006-05-15 15:00 ` Andrew Morton
2006-05-15 15:27 ` Alan Cox
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2006-05-15 15:00 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: bzolnier, linux-ide, linux-kernel
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
>
> Hello.
>
> Andrew Morton wrote:
>
> >> ide_dma_speed() fails to actually honor the IDE drivers' mode support
> >> masks) because of the bogus checks -- thus, selecting the DMA transfer mode
> >> that the driver explicitly refuses to support is possible. Additionally, there
> >> is no check for validity of the UltraDMA mode data in the drive ID, and the
> >> function is misdocumented.
>
> > drivers/ide/ide-lib.c: In function `ide_dma_speed':
> > drivers/ide/ide-lib.c:86: warning: `ultra_mask' might be used uninitialized in this function
>
> > Looks like a real bug to me - it depends up on the values of `mode' and
> > id->field_value.
>
> > Anyway, I'll drop it, please review and fix. I assume that warning was
> > occurring for you as well - please spend more time over these things.
> > Especially when working on IDE, where bugs are slow to show themselves and
> > have particularly bad consequences.
>
> That's what gcc thinks. The code is 100% correct -- it will never reach
> the switch statement with mode > 0 (in which case ultra_mask isn't used) and
> ultra_mask unitialized.
hm, OK.
> I may add an explicit initializer in the declaration if you like...
Opinions vary. I think that's less bad than spitting a warning at all
developers for all time.
But we can redo things like below and save a little code in the process.
Look OK?
--- devel/drivers/ide/ide-lib.c~ide_dma_speed-fixes-warning-fix 2006-05-15 07:56:28.000000000 -0700
+++ devel-akpm/drivers/ide/ide-lib.c 2006-05-15 08:00:12.000000000 -0700
@@ -90,9 +90,9 @@ u8 ide_dma_speed(ide_drive_t *drive, u8
return 0;
/* Capable of UltraDMA modes? */
- if (id->field_valid & 4)
- ultra_mask = id->dma_ultra & hwif->ultra_mask;
- else
+ ultra_mask = id->dma_ultra & hwif->ultra_mask;
+
+ if (!(id->field_valid & 4))
mode = 0; /* fallback to MW/SW DMA if no UltraDMA */
switch (mode) {
_
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ide_dma_speed() fixes
2006-05-15 15:00 ` Andrew Morton
@ 2006-05-15 15:27 ` Alan Cox
0 siblings, 0 replies; 6+ messages in thread
From: Alan Cox @ 2006-05-15 15:27 UTC (permalink / raw)
To: Andrew Morton; +Cc: Sergei Shtylyov, bzolnier, linux-ide, linux-kernel
On Llu, 2006-05-15 at 08:00 -0700, Andrew Morton wrote:
>
> /* Capable of UltraDMA modes? */
> - if (id->field_valid & 4)
> - ultra_mask = id->dma_ultra & hwif->ultra_mask;
> - else
> + ultra_mask = id->dma_ultra & hwif->ultra_mask;
> +
> + if (!(id->field_valid & 4))
> mode = 0; /* fallback to MW/SW DMA if no UltraDMA */
>
Looks fine to me, id-> is always the full 512 bytes of data so its safe
to do that.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-05-15 15:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-05-12 2:36 [PATCH] ide_dma_speed() fixes Sergei Shtylyov
2006-05-14 12:05 ` Andrew Morton
2006-05-15 13:44 ` Sergei Shtylyov
2006-05-15 14:08 ` Alan Cox
2006-05-15 15:00 ` Andrew Morton
2006-05-15 15:27 ` Alan Cox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox