Re: [PATCH #upstream-fixes] libata: clear link->eh_info.serror from ata_std_postreset()

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> link->eh_info.serror is used to cache SError for controllers which
>> need it cleared from interrupt handler to clear IRQ.  It also should
>> be cleared after reset just like SError itself.
>>
>> Make ata_std_postreset() clear link->eh_info.serror too and update
>> sata_sil such that it doesn't care about bookkeeping the value.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/libata-core.c |    1 +
>>  drivers/ata/sata_sil.c    |   11 +----------
>>  2 files changed, 2 insertions(+), 10 deletions(-)
>>
>> Index: work/drivers/ata/libata-core.c
>> ===================================================================
>> --- work.orig/drivers/ata/libata-core.c
>> +++ work/drivers/ata/libata-core.c
>> @@ -3923,6 +3923,7 @@ void ata_std_postreset(struct ata_link *
>>      /* clear SError */
>>      if (sata_scr_read(link, SCR_ERROR, &serror) == 0)
>>          sata_scr_write(link, SCR_ERROR, serror);
>> +    link->eh_info.serror = 0;
> 
> IMO it would make more sense to record the state of the hardware
> following sata_scr_write() than simply zeroing the cache value.
> 
> Just a gut feeling... it seems like having a manufactured value (zero)
> rather than the last known from-the-hardware value could lead to
> inconsistencies.
> 
> Comments?

link->eh_info.serror is always used as complement to the hardware SError
value.  It's a temporary storage to dump/accumulate SError value when
for whatever reason the hardware can't hold the value.
link->eh_info.serror and the hardware SError value are always OR'd
before being used, so no reason to dup the value.

Thanks.

-- 
tejun