linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tejun Heo <htejun@gmail.com>
To: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Jeff Garzik <jeff@garzik.org>,
	linux-scsi <linux-scsi@vger.kernel.org>,
	linux-ide <linux-ide@vger.kernel.org>,
	Jens Axboe <Jens.Axboe@oracle.com>,
	FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: Re: [PATCH RESEND number 2] libata: eliminate the home grown dma padding in favour of that provided by the block layer
Date: Mon, 04 Feb 2008 10:21:38 +0900	[thread overview]
Message-ID: <47A668A2.6030106@gmail.com> (raw)
In-Reply-To: <1202058736.3318.75.camel@localhost.localdomain>

James Bottomley wrote:
> I'm only really going by what Tejun says about AHCI.  The problem as I
> understand it is data overrun on PIO mode commands.  AHCI apparently
> (like aic94xx) processes these internally and doesn't actually use the
> libata pio handlers, so it just uses an internal buffer to receive the
> PIO and DMA it into memory.  However, Tejun says (and he'll correct me
> if I'm paraphrasing wrongly, since this was on IRC a while ago) that jmb
> ahci and sata_sil24 both error out in different (but fairly nasty) ways
> if they get extra PIO data that there's no place in the SG list to
> deposit.  This seems to be why he wants to introduce a DMA drain in
> addition to the existing PIO drain.  I'd certainly characterise this
> behaviour as "broken" ... especially as not all AHCI implementations
> apparently have the bug ... some do exactly the right thing on PIO
> overruns and don't need the drain element.

JMB ahci triggers internal error (or was it timeout?  my memory is a bit
blurry now) on overflow.  ICHx ahci and sata_sil24 corrupt data by
offsetting the last FIS containing odd bytes by a byte if data buffer is
not aligned to 4bytes under certain conditions.

[and some explanations on why the aligning and draining stuff]

Also note that ignoring overflow and/or appending draining buffer
shouldn't be applied to READ/WRITE.  Over/underflow should just cause
HSM violation on RWs and friends.  We can do this for each driver or
rather each controller by enabling OFS (ahci), DRD (sil24) but the catch
is that it's pretty darn difficult to verify it actually works.  It not
only depends on specific controller but also on which ATAPI device is
attached and how it behaves depending on chunk size, transfer size in
CDB and command protocol including where it splits data FISes.

For example, IIRC, the above offset-by-one condition occurs on ICHx
ahci's (I've tested 7, 8 and 9), under PIO protocol, when the device
determines to use a separate data FIS for the last three bytes and I
don't know why it doesn't happen for DMA.  It's probably because the
ATAPI devices I have don't split FIS there but who knows?

So, I think we're far better off implementing a generic mechanism at
some higher layer.  libata core was okay.  Block layer is much better.
The overhead is insignificant and the aligning and draining aren't
needed for hot path commands anyway.

Thanks.

-- 
tejun

  reply	other threads:[~2008-02-04  1:21 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-12-31 21:56 [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer James Bottomley
2007-12-31 22:56 ` Jeff Garzik
2008-01-03  7:58 ` FUJITA Tomonori
2008-01-03 15:12   ` James Bottomley
2008-01-09  2:10 ` Tejun Heo
2008-01-09  4:24   ` James Bottomley
2008-01-09  5:13     ` Tejun Heo
2008-01-09 15:13       ` James Bottomley
2008-01-18 23:14 ` [PATCH RESEND] " James Bottomley
2008-02-01 19:40   ` [PATCH RESEND number 2] " James Bottomley
2008-02-01 20:02     ` Jeff Garzik
2008-02-01 21:09       ` James Bottomley
2008-02-03  3:04         ` Tejun Heo
2008-02-03  4:32           ` James Bottomley
2008-02-03  7:37             ` Tejun Heo
2008-02-03 14:38               ` James Bottomley
2008-02-03 15:14                 ` Tejun Heo
2008-02-03 16:12                   ` James Bottomley
2008-02-03 16:38                     ` Jeff Garzik
2008-02-03 17:12                       ` James Bottomley
2008-02-04  1:21                         ` Tejun Heo [this message]
2008-02-04  1:28                     ` Tejun Heo
2008-02-04  9:25                       ` Tejun Heo
2008-02-04 14:43                         ` Tejun Heo
2008-02-04 16:23                           ` James Bottomley
2008-02-05  0:06                             ` Tejun Heo
2008-02-05  0:32                               ` James Bottomley
2008-02-05  0:43                                 ` Tejun Heo
2008-02-05  0:53                                   ` James Bottomley
2008-02-05  1:07                                     ` Tejun Heo
2008-02-05  5:03                                       ` James Bottomley
2008-02-05  5:22                                         ` Tejun Heo
2008-02-04 15:43                         ` James Bottomley

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=47A668A2.6030106@gmail.com \
    --to=htejun@gmail.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=Jens.Axboe@oracle.com \
    --cc=fujita.tomonori@lab.ntt.co.jp \
    --cc=jeff@garzik.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    /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).