Re: [PATCH 2/4] libata: separate out rw ATA taskfile building into ata_build_tf()

[Tejun Heo <htejun@gmail.com>]

Mark Lord wrote:
> This logic sequence bothers me slightly:
> 
> Tejun Heo wrote:
>> Separate out rw ATA taskfile building from ata_scsi_rw_xlat() into
>> ata_build_tf().  This will be used to improve media error handling.
> ...
>> +    if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
>> +               ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
>> +        /* yay, NCQ */
>> +        if (!lba_48_ok(block, n_block))
>> +            return -ERANGE;
> ...
>> +    } else if (dev->flags & ATA_DFLAG_LBA) {
>> +        tf->flags |= ATA_TFLAG_LBA;
>> +
>> +        if (lba_28_ok(block, n_block)) {
>> +            /* use LBA28 */
>> +            tf->device |= (block >> 24) & 0xf;
>> +        } else if (lba_48_ok(block, n_block)) {
>> +            if (!(dev->flags & ATA_DFLAG_LBA48))
>> +                return -ERANGE;
> ...
> 
> In the first case, if !lba_48_ok(), then we should at least attempt
> to see if the request can be issued via non-NCQ.
> 
> Something like this:
> 
> if (lba_48_ok() && dev->flags & 
> (ATA_DFLAG_PIO|ATA_DFLAG_NCQ_OFF|ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ) {
>   /* yay, NCQ */
>   ...
> } else if (dev->flags & ATA_DFLAG_LBA) {
>   ...
> 
> ..except prettier!  ;)
> 
> In theory, it looks like an impossible sequence anyway..
> if we have the NCQ flag set, then we probably already *know*
> that LBA48 is okay.  But since we're not making that assumption,
> let's give it the benefit of a doubt and try non-NCQ if LBA48 is a no-go.

First of all, that function is moved verbatim from ata_scsi_rw_xlat(), 
and I don't really understand your point.  lba_48_ok() checks whether 
the given block and n_block are inside LBA48 limit.  NCQ commands use 
LBA48 (well, nsect is in the different registers but the limits are the 
same) which is the largest addressing mode currently available.  So, if 
a command cannot be issued using NCQ, it cannot be issued w/ any command 
mode.  ie. if !lba_48_ok(), that command just cannot be issued.

Am I misunderstanding you?

-- 
tejun