Re: [PATCH] make ata_exec_internal_sg honor DMADIR

[Tejun Heo <tj@kernel.org>]

Hello,

On Sun, May 19, 2013 at 03:31:22PM +0200, Vincent Pelletier wrote:
> Now that I got it working, I'm thining it would be better to automatically 
> fallback to enabling DMADIR per-device level during initialisation (just like 
> current fallbacks to 1.5Gbps and UDMA/33:PIO3, as visible in 1/2 commit 
> message).
> I believe it will slow down boot when such device is plugged in, though, any 
> idea on how this can be avoided ?

I don't really wanna go that way.  Those bridges always have been
something fringe and broken in ways which aren't fundamentally
fixable.  Fixing one would break another without anyway to properly
detect them.

So, I'm okay with having a knob for cases where the user knows what to
do but I don't think even that is something of much importance, and
I'm definitely not gonna do anything which may affect !bridge case
adversely.  Those bridges have always been a second-class citizen and
their importance has waned a lot.

> > While better, please go into more details.  The problem here is that
> > the bridge requires DMADIR and while libata makes use of DMADIR for
> > regular commands, it doesn't do that for internal commands which are
> > used during EH, right?
> 
> Just to be clear about EH: the timeout happens during device  initialisation 
> (both at boot or on hotplug), not during error handling (or, maybe it happens 
> in both places, but for the same reason).
> I'm not comfortable with calling device discovery/initialisation as "error 
> handling", but maybe it's unambiguous when used to libata.

EH stands for exception handling and discovery / init definitely are
part of exception handling in libata speak.

> Subject: [1/2] libata: make ata_exec_internal_sg honor DMADIR
...
> ---
>  drivers/ata/libata-core.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 63c743b..d121db7 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1600,8 +1600,13 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
>  
>  	/* prepare & issue qc */
>  	qc->tf = *tf;
> -	if (cdb)
> +	if (cdb) {
>  		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
> +		if ((dev->flags & ATA_DFLAG_DMADIR) &&
> +		    (dma_dir == DMA_FROM_DEVICE))
> +			/* some SATA bridges need us to indicate data xfer direction */
> +			qc->tf.feature |= ATAPI_DMADIR;
> +	}

So, nope, I really don't want this.

> Subject: [2/2] [RFC] libata: Add a per-device sysfs control for atapi_dmadir
...
> +atapi_dmadir
> +
> +	Bitmask enabling dmadir for corresponding device if ATAPI.
> +	1:	Enable dmadir for port's device 0
> +	2:	Enable dmadir for port's device 1
> +	(etc)
> +	See also libata's atapi_dmadir module parameter.

Shouldn't this be a device property?

Thanks.

-- 
tejun