From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: [PATCH #upstream-fixes] libata: clear link->eh_info.serror from ata_std_postreset() Date: Sat, 08 Dec 2007 09:05:05 +0900 Message-ID: <4759DFB1.7000405@gmail.com> References: <4759DB17.9010505@gmail.com> <4759DB75.1010702@gmail.com> <4759DCF9.10407@garzik.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from rv-out-0910.google.com ([209.85.198.190]:32167 "EHLO rv-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752501AbXLHAFM (ORCPT ); Fri, 7 Dec 2007 19:05:12 -0500 Received: by rv-out-0910.google.com with SMTP id k20so872735rvb for ; Fri, 07 Dec 2007 16:05:11 -0800 (PST) In-Reply-To: <4759DCF9.10407@garzik.org> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Jeff Garzik Cc: IDE/ATA development list , mark.paulus@technologist.com, sveint@gmail.com, bug-track@fisher-privat.net 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 >> --- >> 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