Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
On Wed, 2008-01-09 at 14:13 +0900, Tejun Heo wrote: James Bottomley wrote: Also, DMA alignment at block layer isn't enough for ATA. ATA needs drain buffers for ATAPI commands with variable length response. :-( OK, where is this in the libata code? The dma_pad size is only 4 bytes, so this drain, I assume is only a word long? Given the word alignment requirements of ATA doesn't this still mean it's only draining up to the word boundary anyway (so the code is still correct)? Patch is acked but not merged yet. http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=commit;h=6cd22a0febc74bebe52d58eb22271b8770892a2d The full function can be read from the following. It's ata_sg_setup_extra(). http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=drivers/ata/libata-core.c;h=d763c072e6cefc724ea24cb68a7adf47b340f054;hb=6cd22a0febc74bebe52d58eb22271b8770892a2d OK, but that patch was sent to the mailing list on 4 Jan ... five days after this one. It's a little hard to take unposted patches into account ... It's a fairly comprehensive merge clash ... where is this drain patch on the upstream track? because currently ipr and aic94xx panic the system without the dma padding removal (I just figured -rc6 was a bit late for major surgery like this, but I was planning a backport if it stood up in 2.6.24). James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
James Bottomley wrote: ATA requires that all DMA transfers begin and end on word boundaries. Because of this, a large amount of machinery grew up in ide to adjust scatterlists on this basis. However, as of 2.5, the block layer has a dma_alignment variable which ensures both the beginning and length of a DMA transfer are aligned on the dma_alignment boundary. Although the block layer does adjust the beginning of the transfer to ensure this happens, it doesn't actually adjust the length, it merely makes sure that space is allocated for transfers beyond the declared length. The upshot of this is that scatterlists may be padded to any size between the actual length and the length adjusted to the dma_alignment safely knowing that memory is allocated in this region. Right at the moment, SCSI takes the default dma_aligment which is on a 512 byte boundary. Note that this aligment only applies to transfers coming in from user space. However, since all kernel allocations are automatically aligned on a minimum of 32 byte boundaries, it is safe to adjust them in this manner as well. Signed-off-by: James Bottomley [EMAIL PROTECTED] --- This also fixes a current panic in libsas with SATAPI devices. I was trying to trace a bad interaction between the libata padding and the new sglist code which was the root cause, and ended up concluding that the whole libata padding thing was redundant. In a future improvement, I think we can relax SCSI dma_alignemnt (it's causing almost every user space command to be copied instead of directly passed through) and allow devices and transports to build up their own requirements instead. Great but this intersects with currently queued and pending changes for libata-dev#upstream and needs to be merged. Also, DMA alignment at block layer isn't enough for ATA. ATA needs drain buffers for ATAPI commands with variable length response. :-( -- tejun - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
On Wed, 2008-01-09 at 11:10 +0900, Tejun Heo wrote: James Bottomley wrote: ATA requires that all DMA transfers begin and end on word boundaries. Because of this, a large amount of machinery grew up in ide to adjust scatterlists on this basis. However, as of 2.5, the block layer has a dma_alignment variable which ensures both the beginning and length of a DMA transfer are aligned on the dma_alignment boundary. Although the block layer does adjust the beginning of the transfer to ensure this happens, it doesn't actually adjust the length, it merely makes sure that space is allocated for transfers beyond the declared length. The upshot of this is that scatterlists may be padded to any size between the actual length and the length adjusted to the dma_alignment safely knowing that memory is allocated in this region. Right at the moment, SCSI takes the default dma_aligment which is on a 512 byte boundary. Note that this aligment only applies to transfers coming in from user space. However, since all kernel allocations are automatically aligned on a minimum of 32 byte boundaries, it is safe to adjust them in this manner as well. Signed-off-by: James Bottomley [EMAIL PROTECTED] --- This also fixes a current panic in libsas with SATAPI devices. I was trying to trace a bad interaction between the libata padding and the new sglist code which was the root cause, and ended up concluding that the whole libata padding thing was redundant. In a future improvement, I think we can relax SCSI dma_alignemnt (it's causing almost every user space command to be copied instead of directly passed through) and allow devices and transports to build up their own requirements instead. Great but this intersects with currently queued and pending changes for libata-dev#upstream and needs to be merged. That's OK, I can do the merge .. that's what git is for. Also, DMA alignment at block layer isn't enough for ATA. ATA needs drain buffers for ATAPI commands with variable length response. :-( OK, where is this in the libata code? The dma_pad size is only 4 bytes, so this drain, I assume is only a word long? Given the word alignment requirements of ATA doesn't this still mean it's only draining up to the word boundary anyway (so the code is still correct)? James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
James Bottomley wrote: Also, DMA alignment at block layer isn't enough for ATA. ATA needs drain buffers for ATAPI commands with variable length response. :-( OK, where is this in the libata code? The dma_pad size is only 4 bytes, so this drain, I assume is only a word long? Given the word alignment requirements of ATA doesn't this still mean it's only draining up to the word boundary anyway (so the code is still correct)? Patch is acked but not merged yet. http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=commit;h=6cd22a0febc74bebe52d58eb22271b8770892a2d The full function can be read from the following. It's ata_sg_setup_extra(). http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=drivers/ata/libata-core.c;h=d763c072e6cefc724ea24cb68a7adf47b340f054;hb=6cd22a0febc74bebe52d58eb22271b8770892a2d -- tejun - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
On Mon, 31 Dec 2007 15:56:08 -0600
James Bottomley [EMAIL PROTECTED] wrote:
ATA requires that all DMA transfers begin and end on word boundaries.
Because of this, a large amount of machinery grew up in ide to adjust
scatterlists on this basis. However, as of 2.5, the block layer has a
dma_alignment variable which ensures both the beginning and length of a
DMA transfer are aligned on the dma_alignment boundary. Although the
block layer does adjust the beginning of the transfer to ensure this
happens, it doesn't actually adjust the length, it merely makes sure
that space is allocated for transfers beyond the declared length. The
upshot of this is that scatterlists may be padded to any size between
the actual length and the length adjusted to the dma_alignment safely
knowing that memory is allocated in this region.
Great!
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 124033c..2f40d57 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -282,7 +282,7 @@ enum {
/* size of buffer to pad xfers ending on unaligned boundaries */
ATA_DMA_PAD_SZ = 4,
- ATA_DMA_PAD_BUF_SZ = ATA_DMA_PAD_SZ * ATA_MAX_QUEUE,
+ ATA_DMA_PAD_MASK= ATA_DMA_PAD_SZ - 1,
/* ering size */
ATA_ERING_SIZE = 32,
@@ -446,12 +446,9 @@ struct ata_queued_cmd {
unsigned long flags; /* ATA_QCFLAG_xxx */
unsigned inttag;
unsigned intn_elem;
- unsigned intn_iter;
- unsigned intorig_n_elem;
int dma_dir;
- unsigned intpad_len;
unsigned intsect_size;
unsigned intnbytes;
@@ -461,7 +458,6 @@ struct ata_queued_cmd {
unsigned intcursg_ofs;
struct scatterlist sgent;
- struct scatterlist pad_sgent;
void*buf_virt;
/* DO NOT iterate over __sg manually, use ata_for_each_sg() */
@@ -606,9 +602,6 @@ struct ata_port {
struct ata_prd *prd;/* our SG list */
dma_addr_t prd_dma; /* and its DMA mapping */
- void*pad; /* array of DMA pad buffers */
- dma_addr_t pad_dma;
-
struct ata_ioports ioaddr; /* ATA cmd/ctl/dma register blocks */
u8 ctl;/* cache of ATA control register */
@@ -1080,24 +1073,15 @@ extern void ata_port_pbar_desc(struct ata_port *ap,
int bar, ssize_t offset,
static inline struct scatterlist *
ata_qc_first_sg(struct ata_queued_cmd *qc)
{
- qc-n_iter = 0;
if (qc-n_elem)
return qc-__sg;
- if (qc-pad_len)
- return qc-pad_sgent;
return NULL;
}
static inline struct scatterlist *
ata_qc_next_sg(struct scatterlist *sg, struct ata_queued_cmd *qc)
{
- if (sg == qc-pad_sgent)
- return NULL;
- if (++qc-n_iter qc-n_elem)
- return sg_next(sg);
- if (qc-pad_len)
- return qc-pad_sgent;
- return NULL;
+ return sg_next(sg);
}
#define ata_for_each_sg(sg, qc) \
How about removing ata_qc_first_sg and ata_qc_next_sg completely?
Now we can just replace ata_qc_next_sg with sg_next. qc-__sg seems to
be always initialized to NULL so we can remove ata_qc_first_sg too.
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4f6404c..2774882 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1054,25 +1054,8 @@ extern void ata_port_pbar_desc(struct ata_port *ap, int
bar, ssize_t offset,
const char *name);
#endif
-/*
- * qc helpers
- */
-static inline struct scatterlist *
-ata_qc_first_sg(struct ata_queued_cmd *qc)
-{
- if (qc-n_elem)
- return qc-__sg;
- return NULL;
-}
-
-static inline struct scatterlist *
-ata_qc_next_sg(struct scatterlist *sg, struct ata_queued_cmd *qc)
-{
- return sg_next(sg);
-}
-
#define ata_for_each_sg(sg, qc) \
- for (sg = ata_qc_first_sg(qc); sg; sg = ata_qc_next_sg(sg, qc))
+ for (sg = qc-__sg; sg; sg = sg_next(sg))
static inline unsigned int ata_tag_valid(unsigned int tag)
{
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
On Thu, 2008-01-03 at 16:58 +0900, FUJITA Tomonori wrote:
On Mon, 31 Dec 2007 15:56:08 -0600
James Bottomley [EMAIL PROTECTED] wrote:
@@ -1080,24 +1073,15 @@ extern void ata_port_pbar_desc(struct ata_port *ap,
int bar, ssize_t offset,
static inline struct scatterlist *
ata_qc_first_sg(struct ata_queued_cmd *qc)
{
- qc-n_iter = 0;
if (qc-n_elem)
return qc-__sg;
- if (qc-pad_len)
- return qc-pad_sgent;
return NULL;
}
static inline struct scatterlist *
ata_qc_next_sg(struct scatterlist *sg, struct ata_queued_cmd *qc)
{
- if (sg == qc-pad_sgent)
- return NULL;
- if (++qc-n_iter qc-n_elem)
- return sg_next(sg);
- if (qc-pad_len)
- return qc-pad_sgent;
- return NULL;
+ return sg_next(sg);
}
#define ata_for_each_sg(sg, qc) \
How about removing ata_qc_first_sg and ata_qc_next_sg completely?
Now we can just replace ata_qc_next_sg with sg_next. qc-__sg seems to
be always initialized to NULL so we can remove ata_qc_first_sg too.
Sure ... I assumed (without actually looking) that the inlines were
there because they were an API used throughout the drivers. Actually,
grep tells me they're only used in the ata_for_each_sg macro, so this
patch looks good.
Thanks,
James
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4f6404c..2774882 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1054,25 +1054,8 @@ extern void ata_port_pbar_desc(struct ata_port *ap,
int bar, ssize_t offset,
const char *name);
#endif
-/*
- * qc helpers
- */
-static inline struct scatterlist *
-ata_qc_first_sg(struct ata_queued_cmd *qc)
-{
- if (qc-n_elem)
- return qc-__sg;
- return NULL;
-}
-
-static inline struct scatterlist *
-ata_qc_next_sg(struct scatterlist *sg, struct ata_queued_cmd *qc)
-{
- return sg_next(sg);
-}
-
#define ata_for_each_sg(sg, qc) \
- for (sg = ata_qc_first_sg(qc); sg; sg = ata_qc_next_sg(sg, qc))
+ for (sg = qc-__sg; sg; sg = sg_next(sg))
static inline unsigned int ata_tag_valid(unsigned int tag)
{
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
ATA requires that all DMA transfers begin and end on word boundaries.
Because of this, a large amount of machinery grew up in ide to adjust
scatterlists on this basis. However, as of 2.5, the block layer has a
dma_alignment variable which ensures both the beginning and length of a
DMA transfer are aligned on the dma_alignment boundary. Although the
block layer does adjust the beginning of the transfer to ensure this
happens, it doesn't actually adjust the length, it merely makes sure
that space is allocated for transfers beyond the declared length. The
upshot of this is that scatterlists may be padded to any size between
the actual length and the length adjusted to the dma_alignment safely
knowing that memory is allocated in this region.
Right at the moment, SCSI takes the default dma_aligment which is on a
512 byte boundary. Note that this aligment only applies to transfers
coming in from user space. However, since all kernel allocations are
automatically aligned on a minimum of 32 byte boundaries, it is safe to
adjust them in this manner as well.
Signed-off-by: James Bottomley [EMAIL PROTECTED]
---
This also fixes a current panic in libsas with SATAPI devices. I was
trying to trace a bad interaction between the libata padding and the new
sglist code which was the root cause, and ended up concluding that the
whole libata padding thing was redundant.
In a future improvement, I think we can relax SCSI dma_alignemnt (it's
causing almost every user space command to be copied instead of directly
passed through) and allow devices and transports to build up their own
requirements instead.
James
drivers/ata/ahci.c|5 -
drivers/ata/libata-core.c | 126 +++---
drivers/ata/libata-scsi.c | 18 +-
drivers/ata/pata_icside.c |8 --
drivers/ata/sata_fsl.c| 17 -
drivers/ata/sata_mv.c |5 -
drivers/ata/sata_nv.c |2
drivers/ata/sata_promise.c|2
drivers/ata/sata_qstor.c |2
drivers/ata/sata_sil24.c |5 -
drivers/scsi/ipr.c|4 -
drivers/scsi/libsas/sas_ata.c |4 -
include/linux/libata.h| 35 ---
13 files changed, 25 insertions(+), 208 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 54f38c2..4dd318d 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1965,16 +1965,11 @@ static int ahci_port_start(struct ata_port *ap)
struct ahci_port_priv *pp;
void *mem;
dma_addr_t mem_dma;
- int rc;
pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
if (!pp)
return -ENOMEM;
- rc = ata_pad_alloc(ap, dev);
- if (rc)
- return rc;
-
mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, mem_dma,
GFP_KERNEL);
if (!mem)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4753a18..97db82f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -60,6 +60,8 @@
#include linux/io.h
#include scsi/scsi.h
#include scsi/scsi_cmnd.h
+#include scsi/scsi_device.h
+#include scsi/scsi_dbg.h
#include scsi/scsi_host.h
#include linux/libata.h
#include asm/semaphore.h
@@ -4464,7 +4466,6 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
struct ata_port *ap = qc-ap;
struct scatterlist *sg = qc-__sg;
int dir = qc-dma_dir;
- void *pad_buf = NULL;
WARN_ON(!(qc-flags ATA_QCFLAG_DMAMAP));
WARN_ON(sg == NULL);
@@ -4474,34 +4475,14 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
VPRINTK(unmapping %u sg elements\n, qc-n_elem);
- /* if we padded the buffer out to 32-bit bound, and data
-* xfer direction is from-device, we must copy from the
-* pad buffer back into the supplied buffer
-*/
- if (qc-pad_len !(qc-tf.flags ATA_TFLAG_WRITE))
- pad_buf = ap-pad + (qc-tag * ATA_DMA_PAD_SZ);
-
if (qc-flags ATA_QCFLAG_SG) {
if (qc-n_elem)
dma_unmap_sg(ap-dev, sg, qc-n_elem, dir);
- /* restore last sg */
- sg_last(sg, qc-orig_n_elem)-length += qc-pad_len;
- if (pad_buf) {
- struct scatterlist *psg = qc-pad_sgent;
- void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
- memcpy(addr + psg-offset, pad_buf, qc-pad_len);
- kunmap_atomic(addr, KM_IRQ0);
- }
} else {
if (qc-n_elem)
dma_unmap_single(ap-dev,
sg_dma_address(sg[0]), sg_dma_len(sg[0]),
dir);
- /* restore sg */
- sg-length += qc-pad_len;
- if (pad_buf)
- memcpy(qc-buf_virt + sg-length - qc-pad_len,
- pad_buf, qc-pad_len);
Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer
James Bottomley wrote: ATA requires that all DMA transfers begin and end on word boundaries. Because of this, a large amount of machinery grew up in ide to adjust scatterlists on this basis. However, as of 2.5, the block layer has a dma_alignment variable which ensures both the beginning and length of a DMA transfer are aligned on the dma_alignment boundary. Although the block layer does adjust the beginning of the transfer to ensure this happens, it doesn't actually adjust the length, it merely makes sure that space is allocated for transfers beyond the declared length. The upshot of this is that scatterlists may be padded to any size between the actual length and the length adjusted to the dma_alignment safely knowing that memory is allocated in this region. Right at the moment, SCSI takes the default dma_aligment which is on a 512 byte boundary. Note that this aligment only applies to transfers coming in from user space. However, since all kernel allocations are automatically aligned on a minimum of 32 byte boundaries, it is safe to adjust them in this manner as well. Signed-off-by: James Bottomley [EMAIL PROTECTED] --- This also fixes a current panic in libsas with SATAPI devices. I was trying to trace a bad interaction between the libata padding and the new sglist code which was the root cause, and ended up concluding that the whole libata padding thing was redundant. In a future improvement, I think we can relax SCSI dma_alignemnt (it's causing almost every user space command to be copied instead of directly passed through) and allow devices and transports to build up their own requirements instead. Initial read: ACK, with thanks. I'll queue this into libata#upstream, unless you have other suggestions. This is definitely the sort of thing that low-level drivers (and their helper libs, e.g. libata) should not need to do. Jeff - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
