Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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@gmail.com>

    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