Jeff Garzik wrote:
Tejun Heo wrote:
Implement new EH action ATA_EH_SPINUP. This will be used by new PM
implementation.
Signed-off-by: Tejun Heo <htejun@xxxxxxxxx>
This patch further reinforces something I've been thinking about, as I
watch the EH grow: these EH actions really should be separated into
small, discrete operations. Then the EH "driver" will push discrete
operations onto an execution stack.
It's always getting tough to follow ata_eh_recover() and ata_eh_reset()
's code through multiple iterations of recovery. This patch
(ata_eh_spinup) also illustrates how difficult it would be if the
ordering of these operations ever needed to change.
So, I'm not NAK'ing the patch, just throwing out a 'caution' sign. :)
I've often thought something along the lines of a list of tasks
("eh_op_list"), for when a port is frozen and doing EH:
ata_eh_freeze()
ata_eh_push(ap, EH_OP_RESET_BUS, ...)
ata_eh_push(ap, EH_OP_SPINUP, ...)
ata_eh_push(ap, EH_OP_CONFIGURE, ...)
ata_eh_push(ap, EH_OP_SET_MODE, ...)
ata_eh_run()
ata_eh_thaw()
would be nice. A discrete operation would handle its own discrete
operation, prepend and append other operations to eh_op_list, or
optionally cause the engine to fail, and return immediately.
At first, I thought many different controllers would use different EH
drive routines, so I put all EH operations in separate functions and
grouped them into several larger steps - currently autopsy, report,
recover and finish.
In earlier implementation, sil24 and ahci actually had to implement
their own drive routine calling those ops and doing private things
in-between, but as EH implementation matures all those are handled by
feeding proper information into the core EH. I'm looking at other
drivers to convert them to new EH but haven't found any driver which the
current standard EH can't handle. Although I haven't looked at all
drivers yet and some of them may indeed need such specialized
implementation.
I think it boils down to the fact that the core EH is more about ATA
devices than it's about controllers. Controllers vary from one another
but ATA/ATAPI devices are the same old ATA/ATAPI devices whatever the
controller is. So, as long as LLD has a way to feed enough information
to core EH about the state of the attached device && core EH's
controller interface (freeze, thaw & resets) fits the controller to
acceptable degree, core EH doesn't have to be different across different
LLDs.
IMHO, it's best to keep single unified implementation until it begins to
pick up many special cases to deal with specific LLDs. I think we're
still in the safe regarding that. If we pass that point, I think we
should start by implementing private drive routines in LLDs before
making core EH programmable. After several such tries, the requirements
for programmable EH would be much clearer and we would be able to do
much better job of implementing something which is flexible enough but
still as simple as possible.
To sum up...
* Until now, unified EH seems good enough. In fact, not a single driver
needs to go further than calling ata_do_eh() w/ proper callbacks.
* I agree that it would be nice to have programmable EH, but let's do it
when it's actually needed. It seems that early implementation of
complex infrastructure seldom ends up in something pleasant to look at.
Thanks.
--
tejun
-
: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
