Re: [PATCH] libata: eliminate the home grown dma padding in favour of that provided by the block layer

FUJITA Tomonori
2008-01-03T08:02:27Z

On Mon, 31 Dec 2007 15:56:08 -0600
James Bottomley <James.Bottomley@HansenPartnership.com> 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 int tag;
> unsigned int n_elem;
> - unsigned int n_iter;
> - unsigned int orig_n_elem;
>
> int dma_dir;
>
> - unsigned int pad_len;
> unsigned int sect_size;
>
> unsigned int nbytes;
> @@ -461,7 +458,6 @@ struct ata_queued_cmd {
> unsigned int cursg_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)
{