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

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

Hello.

Russell King wrote:

>>>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.

>>>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

>>Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> I wonder why Sergei's acking this patch - do you have the hardware to
> test it on?  Are you somehow involved in this driver?  I think the
> answer to both is most likely no.

    This just signified my positive review (as for any other IDE patch).

> Moreover, I think the patch is quite broken.  If an invalid DMA mode

    In what way I wonder?

> 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.

    Why bother doing this?  The speedproc() method shouldn't even try to 
handle an unsupopoted mode, to begin with.,,

> Moreover, 'on' will be zero, causing icside_set_speed() to return zero

    It means just the opposite for the speedproc() -- i.e. success.

> which means the DMA setting was not successful (iow, use PIO).

    You're mixing with the ide_dma_check() method.

> This causes icside_dma_check() to return -1 to the IDE layer which
> should fail the setting.

    Then the speedproc() method's return value and the ide_dma_check() method 
must be fixed too -- NAK the patch. I haven't noticed so far that this driver 
is more broken than the others, sorry. :-)

> With your patch, you make icside_set_speed() return '1' meaning DMA
> was succesfully configured.  That then causes icside_dma_check() to

    The return value was wrong from the very start -- any non-zero result 
should mean error.

> return success to the IDE layer, so we now allow invalid speeds.

    What it does *now* is returning bad result to ide_tune_dma() and the 
possible userspace callers.

> Ergo, what wasn't broken before is now broken.  So the patch is
> completely wrong and was trying to fix a problem which didn't exist in
> the first place.

    More like incomplete fix to an existing problem.

> BIG NAK.

    Indeed. It needs a bigger hammer. ;-)

MBR, Sergei