Hello.
Bartlomiej Zolnierkiewicz wrote:
Moreover, I think the patch is quite broken. If an invalid DMA mode
is passed, currently the driver sets the cycle time to 480ns (stored
in drive_data) since both cycle_time and use_dma_info will be zero.
Moreover, 'on' will be zero, causing icside_set_speed() to return zero
which means the DMA setting was not successful (iow, use PIO).
Thanks for spotting this. The patch in the current form is indeed broken
as I haven't noticed that icside_set_speed() returns zero on failure.
You're not the only one.
However all other implementations of ->speedproc return zero on success
and non-zero on failure. Currently it doesn't matter for icside host
driver and isn't a bug per se since:
- ide_set_xfer_rate() return value is ignored by all IDE core users
Indeed. So my accusation of breaking the usermode interface (and UltraDMA
downgrade) was too rash. :-<
- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()
Yeah, that was rash too. :-<
but sooner or later we will need to fix anyway - so lets do it now.
It also seems to cleanup icside_dma_check() a bit since return value
invertion is no longer needed.
[PATCH] icside: fix ->speedproc to return on unsupported modes (take 2)
* All other implementations of ->speedproc return zero on success
and non-zero on failure. Currently it doesn't matter for icside host
driver and isn't a bug per se since:
- ide_set_xfer_rate() return value is ignored by all IDE core users
- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()
but sooner or later we will need to fix anyway - so lets do it now.
* icside_set_speed() happily accepts unsupported transfer modes which
results in drive->drive_data being set to the maximum value (480)
and drive->current_speed being set to the unsupported transfer mode.
I don't understand why it needs to bother setting drive->current_speed at
all to begin with.
Fix it.
v2:
- The initial version of the patch was broken because it didn't take into
the account (the different from usual) return values of icside_set_speed().
(Noticed by Russell).
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
Looks like the patch still needs more polishing:
Index: b/drivers/ide/arm/icside.c
===================================================================
--- a/drivers/ide/arm/icside.c
+++ b/drivers/ide/arm/icside.c
@@ -250,7 +250,7 @@ static void icside_build_sglist(ide_driv
*/
static int icside_set_speed(ide_drive_t *drive, const u8 xfer_mode)
{
- int on = 0, cycle_time = 0, use_dma_info = 0;
+ int on = -1, cycle_time = 0, use_dma_info = 0;
We don't need to pre-initialize cycle_time anymore.
switch (xfer_mode) {
case XFER_MW_DMA_2:
@@ -272,6 +272,8 @@ static int icside_set_speed(ide_drive_t
case XFER_SW_DMA_0:
cycle_time = 480;
break;
+ default:
+ return 1;
}
/*
@@ -284,7 +286,7 @@ static int icside_set_speed(ide_drive_t
drive->drive_data = cycle_time;
if (cycle_time && ide_config_drive_speed(drive, xfer_mode) == 0)
Now cycle_time can't be 0 here, so the first condition may be dropped.
- on = 1;
+ on = 0;
else
drive->drive_data = 480;
I'm not sure that this really makes sense now.
MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
