Re: [PATCH] libata: write cache and read ahead

[Jeff Garzik <jgarzik@pobox.com>]

Douglas Gilbert wrote:
> With repect to my libata TEST UNIT READY patch there
> is a good chance that can be dropped. The feedback
> from the t10 reflector seems to indicate the current
> definition may be changed, unfortunately the proposed
> changes may require some SAT state information being
> held for each libata device (e.g. pseudo "STOPPED"

Keeping some state is not a big deal.  If it grows beyond a single 
struct member, though, I would probably be inclined to encapsulate the 
state in a separate struct ata_scsi_xlat_state or somesuch.


> state to mimic SBC-2). It also seems some power up
> issues may need to involve the transport layer
> for resolution. For example, SSP's additional sense code
> of LOGICAL UNIT NOT READY, NOTIFY (ENABLE SPINUP)
> REQUIRED may also need to be issued by libata.

libata, in general, is pretty ignorant of PM issues :)


> Another potential issue that I didn't mention was support
> for the "IMMED" bit in commands like START STOP UNIT; i.e.
> issue the corresponding ATA command but return to the caller
> without waiting for the ATA command to finish.

No need to avoid the issue, I know all about it :)

No plans to support stuff such as IMMED; too much pain for little gain.


> Showing my aversion to writing something like:
>    u8 page[sizeof(def_caching_mpage)];
> 
> Using sizeof in an array declaration seems to be a grey area in C,
> but gcc accepts it so perhaps I shouldn't worry.

Your example (quoted) should be fine on all compilers we care about :)


>>>+/* FIXME: we may want to issue two SET FEATURES commands here */
>>>+        if (dra != !(ata_id_rahead_enabled(qc->dev->id))) {
>>
>>
>>bug: need one more '!' AFAICS
> 
> 
> More coffee needed (by you) IMO. The logical inversion is required

Whoops, indeed, sorry about that.



>>>+    case INQUIRY:
>>>+        if (((cmd[1] & 0x3) == 0x1) && (cmd[2] == 0x89)) {
>>
>>
>>what do these magic numbers indicate?
> 
> 
> Place holder for ATA Information VPD page [0x89]. Valid when
> CmdDt=0 and EVPD=1. That page is defined in sat-r05. I'm about to
> decode that page in sg_inq and sdparm. Its payload is the ATA
> IDENTIFY command's response and the ATA device's "signature"
> [whatever that is].

Let's not add such a placeholder.  I'd rather just wait until the real 
thing appears :)

	Jeff