LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Tejun Heo <htejun@gmail.com>
To: Mikael Pettersson <mikpe@it.uu.se>
Cc: Christian Mueller <Christian.Mueller@wdc.com>,
	Bruce Allen <ballen@gravity.phys.uwm.edu>,
	Smartmontools Mailing List 
	<smartmontools-support@lists.sourceforge.net>,
	LKML <linux-kernel@vger.kernel.org>,
	IDE/ATA development list <linux-ide@vger.kernel.org>
Subject: Re: [smartmontools-support] inactive SATA drives won't stay in standby or sleep, PATA models did. (fwd)
Date: Tue, 21 Oct 2008 17:55:31 +0900	[thread overview]
Message-ID: <48FD9903.2090400@gmail.com> (raw)
In-Reply-To: <18685.35827.914278.413151@harpo.it.uu.se>

Hello, Mikael.
> I did a round of regression tests which identified another
> related but different problem.
> 
> In kernels 2.6.24 and 2.6.25 sata_promise would actually recover
> from the timeouts, while in kernels 2.6.26 and 2.6.27 it would not.
> Before 2.6.26 sata_promise explicitly used sata_std_hardreset, but
> in the "make reset related methods proper port operations" commit
> (a1efdaba2dbd6fb89e23a87b66d3f4dd92c9f5af), Tejun changed sata_promise
> to use the hardreset it now inherits from ata_sff_port_ops, namely
> sata_sff_hardreset. This change looks accidental. The main difference
> between these two procedures is that the sff version will poll the
> legacy status register until the port becomes ready.

Hmm... it's quite likely that I've introduced the regression but that
commit ain't it (I actually wrote a script to verify the inheritance
change doesn't actually change the function table).  What used to be
sata_std_hardreset() is now sata_sff_hardreset().  The change was made
while separating out SFF as [S]ATA as per the standard doesn't have
any way to wait for device readiness.  The TF-polling is SFF specific
and thus moved out to sata_sff_hardreset().

So, in both 2.6.24 and 2.6.25, sata_promise did wait for device
readiness after hardreset as does 2.6.26 or any later version.

> Changing sata_promise to use sata_std_hardreset in 2.6.26/.27
> makes the EH after the timeouts much more reliable. Not as
> tidy as with the previous ->prereset fix, but still working.

The only behavior change between 2.6.25 and 2.6.26 as far as
sata_promise is concerned is that hardrset is preferred over
softreset.  Here's what I think is going on.

Previously, after a timeout, libata-eh will invoke softreset and if
that works all should have been fine whether hardreset actually worked
or not.  Now, after something goes wrong, libata EH calls hardreset
and as hardreset doesn't work properly without the controller reset so
it fails.  So, the libata core layer change exposed a bug in
hardreset, which was one of the reasons why the change was made -
hardreset being the last line of defense, using it occasionally
doesn't make sense test-coverage-wise.

I agree the core layer changes can be quite confusing but they were
necessary to keep the core layer scalable.  libata now has oodles of
different types of low level drivers and things were and still are
getting quite treacherous for drivers living on the edge.

Anyways, so, please fix hardreset.  If it can't wait for device
readiness reliably, the right thing to do is to use
sata_std_hardreset() which will trigger follow-up softreset to wait
for device readiness and classify devices but if adding the controller
reset makes the hardreset more reliable, please do so.

Thanks.

-- 
tejun

  reply	other threads:[~2008-10-21  8:57 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <48EBFB63.20506@gmail.com>
2008-10-08  0:20 ` Christian Mueller
2008-10-08  0:28   ` Tejun Heo
2008-10-12 14:55     ` Mikael Pettersson
2008-10-13  5:16       ` Tejun Heo
2008-10-13  7:03         ` Mikael Pettersson
2008-10-13  7:08           ` Tejun Heo
2008-10-19 23:40         ` Mikael Pettersson
2008-10-21  4:18           ` Tejun Heo
2008-10-21  7:56             ` Mikael Pettersson
2008-10-21  9:02               ` Tejun Heo
2008-10-21  9:30                 ` Mikael Pettersson
2008-10-21  7:59           ` Mikael Pettersson
2008-10-21  8:55             ` Tejun Heo [this message]
     [not found] <Pine.LNX.4.64.0809142159550.18213@gc.phys.uwm.edu>
     [not found] ` <48E1B8F8.3090205@gmail.com>
     [not found]   ` <48E26BDA.8080804@tlinx.org>
     [not found]     ` <48E26E61.2010705@gmail.com>
     [not found]       ` <48E34BC8.3050009@tlinx.org>
     [not found]         ` <48E6DE07.70706@gmail.com>
2008-10-07  0:37           ` Linda Walsh
2008-10-07  1:08             ` Tejun Heo
2008-10-07  1:36               ` Linda Walsh
2008-10-07  1:42                 ` Tejun Heo
2008-10-07 10:13                   ` Linda Walsh
2008-10-07 22:27                   ` Linda Walsh
2008-10-07 23:59                     ` Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=48FD9903.2090400@gmail.com \
    --to=htejun@gmail.com \
    --cc=Christian.Mueller@wdc.com \
    --cc=ballen@gravity.phys.uwm.edu \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikpe@it.uu.se \
    --cc=smartmontools-support@lists.sourceforge.net \
    --subject='Re: [smartmontools-support] inactive SATA drives won'\''t stay in standby or sleep, PATA models did. (fwd)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).