Re: [PATCH 05/10] libata: implement new EH action ATA_EH_SPINUP

[Tejun Heo <htejun@gmail.com>]

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@gmail.com>
> 
> 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