Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo <htejun@gmail.com>]

Hello, Mark.

Mark Lord wrote:
>> This patch provides two new struct ata_port_operations methods for this,
>> and modifies the code in libata-pmp to use them if provided.
> ...
> 
> Note that, while this does work for sata_mv, I'm still thinking about it.
> 
> I'm not totally clear yet (more reading to do) as to how/when
> the ATA shadow/taskfile registers get updated to reflect those
> for the currently selected pmp..
> 
> It would seem that with other parts of libata-sff directly reading
> from the ctl, status, and altstatus "registers" during polling,
> command setup, and probing, that there might (?) be a loophole
> somewhere in this strategy.

Yeah, this is an interesting problem.  There are basically multiple sets
of TF registers and the SFF way of assuming single set doesn't really
work well and I don't really think adding ->pmp_read/write is the
correct long term solution to the problem.  We need to confine direct TF
register accesses to SFF layer only as controllers which don't implement
SFF interface may or may not emulate TF registers and even when they do,
it sometimes can't really match the SFF behavior.

For command execution, TF write is already performed by qc_prep and
issue and TF read back should be performed by LLD if RESULT_TF is set.
For EH, the reset methods should be responsible whether or how the TF
registers are accessed.

Mark, for your case, I think the correct thing to do is to factor out
the necessary bare-bone part from SFF implementation and use it with
necessary PMP register setup once the necessary change is made to the
core layer.  The controller is NOT a SFF one and I don't think
stretching SFF implementation to fit mv's PMP-aware emulation of it is a
good idea.  Maybe we can fit libata to it but it's likely we'll need to
modify it again to fit different emulation from different vendor.

> Is this scenario going to be possible:  somebody calls sata_pmp_read()
> as part of, say, hotplug polling, and after that operation completes
> we then have code that calls ata_check_status() prior to the next
> tf_load / command issue ?  If so, they'll see the wrong cached shadow
> status register.

I think what should happen is that any of TF registers isn't accessed
behind LLD's back.  Then, you don't have nothing to worry about.  If the
controller emulates some of SFF interface, you can always properly wrap
SFF helpers with necessary setup and teardown added.

> Mmm.. an amazing amount of complexity there for something that
> ought to be very simple.

I wish things are a bit clearer now.  I think the problem here is that
we're assuming SFF TF access on controllers which aren't really SFF.
For sil24 and ahci, the driver emulates it and it isn't too difficult.
The picture gets more interesting for sata_mv as its hardware interface
much closer to SFF than sil24 or ahci and TF registers matter much more.
 For ahci and sil24, LLD can just fool libata core layer which assumes
ubiquitous TF access.  TF registers don't really matter to controller
operation anyway and feeding bogus values work well.  For sata_mv, it's
different.  Those registers are integral part of controller operation
and sata_mv can't really tolerate core layer stepping in w/o notifying LLD.

Thanks.

-- 
tejun