On Thursday 12 July 2007, Sergei Shtylyov wrote:
> 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.
weeeeee, take 3
[PATCH] icside: fix ->speedproc to return on unsupported modes (take 3)
* 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.
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).
v3:
* Remove no longer needed initialization/checking of cycle_time
(Noticed by Sergei).
* No need to set drive->drive_data if DMA is not going to be used
(Noticed by Sergei).
* Remove incorrect setting of drive->current_speed
(Noticed by Sergei).
* Move ide_config_drive_speed() at the end of icside_set_speed().
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
Cc: Russell King <rmk@xxxxxxxxxxxxxxxx>
Cc: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
---
drivers/ide/arm/icside.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
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 cycle_time, use_dma_info = 0;
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;
}
/*
@@ -283,17 +285,10 @@ static int icside_set_speed(ide_drive_t
drive->drive_data = cycle_time;
- if (cycle_time && ide_config_drive_speed(drive, xfer_mode) == 0)
- on = 1;
- else
- drive->drive_data = 480;
-
printk("%s: %s selected (peak %dMB/s)\n", drive->name,
ide_xfer_verbose(xfer_mode), 2000 / drive->drive_data);
- drive->current_speed = xfer_mode;
-
- return on;
+ return ide_config_drive_speed(drive, xfer_mode);
}
static void icside_dma_host_off(ide_drive_t *drive)
@@ -321,7 +316,6 @@ static int icside_dma_check(ide_drive_t
struct hd_driveid *id = drive->id;
ide_hwif_t *hwif = HWIF(drive);
int xfer_mode = XFER_PIO_2;
- int on;
if (!(id->capability & 1) || !hwif->autodma)
goto out;
@@ -350,9 +344,7 @@ static int icside_dma_check(ide_drive_t
}
out:
- on = icside_set_speed(drive, xfer_mode);
-
- return on ? 0 : -1;
+ return icside_set_speed(drive, xfer_mode);
}
static int icside_dma_end(ide_drive_t *drive)
-
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
