Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put_data methods

[Richard Zidlicky <rz@linux-m68k.org>]

On Mon, Apr 07, 2008 at 09:13:42PM +0200, Bartlomiej Zolnierkiewicz wrote:

> 
> v2:
> * Add 'struct request *rq' argument to ->ata_{in,out}put_data methods
>   and don't byte-swap disk fs requests (we shouldn't un-swap fs requests
>   because fs itself is stored byte-swapped on the disk) - this is how
>   things were done before the patch (ideally device mapper should be
>   used instead but it would break existing setups and would have some
>   performance impact).

I like the part about checking 'struct request *rq', will make a  clean
distinction between ide command data and ide disk/medium data which is badly
needed.

However, not only FS data is byteswapped, complete disk including partition
table and everything else is. Will "rq->cmd_type == REQ_TYPE_FS" also catch
all these cases?



> Index: b/drivers/ide/legacy/falconide.c
> ===================================================================
> --- a/drivers/ide/legacy/falconide.c
> +++ b/drivers/ide/legacy/falconide.c
> @@ -44,6 +44,36 @@
>  int falconide_intr_lock;
>  EXPORT_SYMBOL(falconide_intr_lock);
>  
> +static void falconide_atapi_input_bytes(ide_drive_t *drive, void *buf,
> +					unsigned int len)
> +{
> +	insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
> +}
> +
> +static void falconide_atapi_output_bytes(ide_drive_t *drive, void *buf,
> +					 unsigned int len)
> +{
> +	outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
> +}
> +
> +static void falconide_ata_input_data(ide_drive_t *drive, struct request *rq,
> +				     void *buf, unsigned int wcount)
> +{
> +	if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
> +		return insw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
> +
> +	falconide_atapi_input_bytes(drive, buf, wcount * 4);
> +}

this will still do double swapping for disk access, although this is very
easier to fix once the distinction between ide and disk data works fine.

So IMHO despite the little concerns this is the way to go.

Richard