On Wed, Jul 11, 2007 at 11:07:51PM +0400, Sergei Shtylyov wrote: > Hello. > > Bartlomiej Zolnierkiewicz 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@xxxxxxxxx> > > Acked-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> 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. 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). This causes icside_dma_check() to return -1 to the IDE layer which should fail the setting. With your patch, you make icside_set_speed() return '1' meaning DMA was succesfully configured. That then causes icside_dma_check() to return success to the IDE layer, so we now allow invalid speeds. 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. BIG NAK. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - 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
