From: Tejun Heo <htejun@gmail.com>
To: Gwendal Grignou <gwendal@google.com>
Cc: mihrcke@gmail.com, linux-ide@vger.kernel.org, derry@high-rely.com
Subject: Re: [PATCH] libata: Allow SOFT_RESET for Sil3726
Date: Thu, 6 Oct 2011 15:10:26 -0700 [thread overview]
Message-ID: <20111006221026.GA7085@google.com> (raw)
In-Reply-To: <CAMHSBOXhmhaRQgA104U+yajDLwyjVAeMqYUvUZFvZNroL0urug@mail.gmail.com>
Hello,
On Thu, Oct 06, 2011 at 01:44:27PM -0700, Gwendal Grignou wrote:
> Yes. Until the device sends the async D2H Reg FIS indicating the drive
> spun up, the MPM does not accept the SoftRest FIS from the controller.
> On most controller, that fine, the controller state machine keeps
> retrying, but on Sil3132 it stops after a second or so and send an
> error back to the driver.
I see.
> > So, now libata waits for full reset period before proceeding to reset
> > PMP. Hmmm... yeah, it makes sense. Unfortunately, the only way to
> > achieve spinup wait in this case is waiting blindly and libata's reset
> > timeouts are configured to accomodate drive spinup times. PMP SCR
> > failure kinda destroys those blind wait periods.
>
> Yes, I totally agree this blind wait is not clean. Normally we would
> wait until an event occurs [async FIS] and have the timeout just for
> the error case.
> Here we wait [10s] because we think the device is spinning up.
Yes but that's exactly how the reset timeouts are set up. They're
supposed to provide reasonable spinup timeouts when the proper wait
mechanisms can't do so and here it becomes a problem because the blind
timeouts are circumvented by SCR read failure handling.
> > I'm not too sure about ATA_LFLAG_WAIT_SRST. I don't think making the
> > new behavior default would hurt.
>
> I see your point. But if there is no PMP, Sil3132 is behaving, there
> is no need of this logic.
Yes, sure, the behavior is necessary iff PMP is attached as that's
only time SCR read failure can occur anyway and I think it would
generally be a good idea to always enforce the blind timeouts if PMP
is attached, so no need for ATA_LFLAG_WAIT_SRST.
> > Can you please post before & after logs?
> There are 2 problem with the current solution:
> - by not waiting for device spin up, we basically disabled staggered
> spinup: we send hard reset to all port very fast.
> That may put burden on enclosure with weak power supplies.
> - as Derry found out, disk which are slow to spin up can be ignored by
> the kernel.
Looks pretty good to me. Nice spotting. Thanks a lot for tracking it
down and coming up with good solution. :)
--
tejun
next prev parent reply other threads:[~2011-10-06 22:10 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <359604ECF8F440408B9634E6146249B42923805F@mail.scl.local>
[not found] ` <20110628095723.GC3386@htj.dyndns.org>
2011-06-30 17:53 ` Problem w/ hotplug on sata_sil24 w/ PMP (sil3726) Derry Bryson
2011-07-12 15:01 ` tj
2011-07-12 19:21 ` Derry Bryson
2011-07-13 13:34 ` tj
2011-07-13 14:39 ` tj
2011-07-13 16:23 ` Derry Bryson
2011-07-14 7:14 ` tj
2011-07-14 17:37 ` Derry Bryson
2011-07-21 9:00 ` tj
2011-07-21 16:22 ` Derry Bryson
2011-07-22 9:50 ` tj
2011-07-22 17:29 ` Derry Bryson
2011-07-22 19:34 ` Derry Bryson
2011-07-30 12:54 ` tj
2011-09-30 21:54 ` Mike I
2011-10-06 5:48 ` Gwendal Grignou
2011-10-06 6:03 ` [PATCH] libata: Allow SOFT_RESET for Sil3726 Gwendal Grignou
2011-10-06 8:03 ` Tejun Heo
2011-10-06 20:44 ` Gwendal Grignou
2011-10-06 22:10 ` Tejun Heo [this message]
2011-10-12 14:03 ` Mark Lord
2011-10-20 0:02 ` [PATCH] [libata]Issue SRST to Sil3726 PMP Gwendal Grignou
2011-10-20 0:02 ` Gwendal Grignou
2011-10-20 0:07 ` Tejun Heo
2011-10-20 0:28 ` Gwendal Grignou
2011-10-20 0:17 ` Gwendal Grignou
2011-10-20 0:17 ` Gwendal Grignou
2011-10-20 0:35 ` [PATCH] [libata]Send SRST to disks behind " Gwendal Grignou
2011-10-20 0:35 ` [PATCH] [libata]Issue SRST to Disks behind Sil3726 Gwendal Grignou
2011-10-20 0:37 ` Tejun Heo
2011-10-06 10:42 ` [PATCH] libata: Allow SOFT_RESET for Sil3726 Sergei Shtylyov
[not found] ` <CAMHSBOV8rSW3jumpx8URZrgo8w1He9zruP7yAOw7uhQOaAj3Xg@mail.gmail.com>
2011-10-06 6:07 ` Problem w/ hotplug on sata_sil24 w/ PMP (sil3726) Gwendal Grignou
2011-10-08 18:25 ` Michael Ihrcke
2011-10-12 2:06 ` Michael Ihrcke
2011-10-13 2:09 ` Mike I
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=20111006221026.GA7085@google.com \
--to=htejun@gmail.com \
--cc=derry@high-rely.com \
--cc=gwendal@google.com \
--cc=linux-ide@vger.kernel.org \
--cc=mihrcke@gmail.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).