[PATCH] libata: make ata_sg_setup_one() trim zero length sg

[PATCH] libata: make ata_sg_setup_one() trim zero length sg

[Tejun Heo]

This patch makes ata_sg_setup_one() trim sg entry (thus making
qc->n_elem zero) if padding results in zero length sg entry.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

Jeff, currently, in both sg_setup() and sg_setup_one() cases, zero
length sg entry can reach low level driver fill_sg() function which
could possibly cause weird problems.  This and a following patch kill
such cases.

 libata-core.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2583,7 +2583,7 @@ static void ata_sg_clean(struct ata_queu
 	WARN_ON(sg == NULL);
 
 	if (qc->flags & ATA_QCFLAG_SINGLE)
-		WARN_ON(qc->n_elem != 1);
+		WARN_ON(qc->n_elem > 1);
 
 	VPRINTK("unmapping %u sg elements\n", qc->n_elem);
 
@@ -2606,7 +2606,7 @@ static void ata_sg_clean(struct ata_queu
 			kunmap_atomic(addr, KM_IRQ0);
 		}
 	} else {
-		if (sg_dma_len(&sg[0]) > 0)
+		if (qc->n_elem)
 			dma_unmap_single(ap->host_set->dev,
 				sg_dma_address(&sg[0]), sg_dma_len(&sg[0]),
 				dir);
@@ -2784,6 +2784,7 @@ static int ata_sg_setup_one(struct ata_q
 	int dir = qc->dma_dir;
 	struct scatterlist *sg = qc->__sg;
 	dma_addr_t dma_address;
+	int trim_sg = 0;
 
 	/* we must lengthen transfers to end on a 32-bit boundary */
 	qc->pad_len = sg->length & 3;
@@ -2803,13 +2804,15 @@ static int ata_sg_setup_one(struct ata_q
 		sg_dma_len(psg) = ATA_DMA_PAD_SZ;
 		/* trim sg */
 		sg->length -= qc->pad_len;
+		if (sg->length == 0)
+			trim_sg = 1;
 
 		DPRINTK("padding done, sg->length=%u pad_len=%u\n",
 			sg->length, qc->pad_len);
 	}
 
-	if (!sg->length) {
-		sg_dma_address(sg) = 0;
+	if (trim_sg) {
+		qc->n_elem--;
 		goto skip_map;
 	}
 
@@ -2822,9 +2825,9 @@ static int ata_sg_setup_one(struct ata_q
 	}
 
 	sg_dma_address(sg) = dma_address;
-skip_map:
 	sg_dma_len(sg) = sg->length;
 
+skip_map:
 	DPRINTK("mapped buffer of %d bytes for %s\n", sg_dma_len(sg),
 		qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 

Re: [PATCH] libata: make ata_sg_setup_one() trim zero length sg

[Jeff Garzik]

Tejun Heo wrote:
> This patch makes ata_sg_setup_one() trim sg entry (thus making
> qc->n_elem zero) if padding results in zero length sg entry.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
> Jeff, currently, in both sg_setup() and sg_setup_one() cases, zero
> length sg entry can reach low level driver fill_sg() function which
> could possibly cause weird problems.  This and a following patch kill
> such cases.

Can you redo this series against vanilla upstream (and/or #upstream-fixes)?

	Jeff

Re: [PATCH] libata: make ata_sg_setup_one() trim zero length sg

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch makes ata_sg_setup_one() trim sg entry (thus making
>> qc->n_elem zero) if padding results in zero length sg entry.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>> Jeff, currently, in both sg_setup() and sg_setup_one() cases, zero
>> length sg entry can reach low level driver fill_sg() function which
>> could possibly cause weird problems.  This and a following patch kill
>> such cases.
> 
> 
> Can you redo this series against vanilla upstream (and/or #upstream-fixes)?
> 

Hello, Jeff.

Actually, this series is already against the vanilla upstream[1].  The 
order is...

libata: fix WARN_ON() condition in *_fill_sg() [2]
libata: make ata_sg_setup_one() trim zero length sg [3]
libata: fix qc->n_elem == 0 case handling in ata_qc_next_sg [4]

-- 
tejun

[1] f1b318793dcd2d9ff6b5ac06e7762098fa079cee
[2] http://article.gmane.org/gmane.linux.ide/8090
[3] http://article.gmane.org/gmane.linux.ide/8091
[4] http://article.gmane.org/gmane.linux.ide/8092

Re: [PATCH] libata: make ata_sg_setup_one() trim zero length sg

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>> This patch makes ata_sg_setup_one() trim sg entry (thus making
>>> qc->n_elem zero) if padding results in zero length sg entry.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>
>>> ---
>>>
>>> Jeff, currently, in both sg_setup() and sg_setup_one() cases, zero
>>> length sg entry can reach low level driver fill_sg() function which
>>> could possibly cause weird problems.  This and a following patch kill
>>> such cases.
>>
>>
>>
>> Can you redo this series against vanilla upstream (and/or 
>> #upstream-fixes)?
>>
> 
> Hello, Jeff.
> 
> Actually, this series is already against the vanilla upstream[1].  The 
> order is...
> 
> libata: fix WARN_ON() condition in *_fill_sg() [2]
> libata: make ata_sg_setup_one() trim zero length sg [3]
> libata: fix qc->n_elem == 0 case handling in ata_qc_next_sg [4]

Sorry, I should have been more specific.

These changes should go into 2.6.16-rc, and should be based off the 
vanilla Linus tree.  Too many "vanilla upstreams" running around...

	Jeff

[PATCH 1/3] libata: fix WARN_ON() condition in *_fill_sg()

[Tejun Heo]

For ATAPI commands, padding can reduce qc->n_elem by one and thus to
zero making assert(qc->n_elem > 0)'s in ata_fill_sg() and qs_fill_sg()
fail for legal commands.  This patch fixes the assert()'s to take
qc->pad_len into account.

Although the condition check seems a bit excessive, as this part of
code isn't still stable yet, I think it's worth to keep those.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    2 +-
 drivers/scsi/sata_qstor.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

cc1feb52b7a9a10c2438759d39e8649834e668fb
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 7ddd5a6..bbac87a 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2570,7 +2570,7 @@ static void ata_fill_sg(struct ata_queue
 	unsigned int idx;
 
 	assert(qc->__sg != NULL);
-	assert(qc->n_elem > 0);
+	assert(qc->n_elem > 0 || qc->pad_len > 0);
 
 	idx = 0;
 	ata_for_each_sg(sg, qc) {
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
index de05e28..80480f0 100644
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -277,7 +277,7 @@ static unsigned int qs_fill_sg(struct at
 	u8 *prd = pp->pkt + QS_CPB_BYTES;
 
 	assert(qc->__sg != NULL);
-	assert(qc->n_elem > 0);
+	assert(qc->n_elem > 0 || qc->pad_len > 0);
 
 	nelem = 0;
 	ata_for_each_sg(sg, qc) {
-- 
1.1.5

Re: [PATCH 1/3] libata: fix WARN_ON() condition in *_fill_sg()

[Jeff Garzik]

Tejun Heo wrote:
> For ATAPI commands, padding can reduce qc->n_elem by one and thus to
> zero making assert(qc->n_elem > 0)'s in ata_fill_sg() and qs_fill_sg()
> fail for legal commands.  This patch fixes the assert()'s to take
> qc->pad_len into account.
> 
> Although the condition check seems a bit excessive, as this part of
> code isn't still stable yet, I think it's worth to keep those.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-3

[PATCHSET] libata: sg corner case fixes against Linus tree

[Tejun Heo]

Hello, Jeff.

This is the three sg corner case fix patches[1] regenerated against the
current Linus #master[2].  The assert()->WARN_ON() patch in libata-dev
#upstream will collide with this one.  It should be easy to resolve
though.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/8090
    http://article.gmane.org/gmane.linux.ide/8091
    http://article.gmane.org/gmane.linux.ide/8092
[2] bd71c2b17468a2531fb4c81ec1d73520845e97e1

[PATCH 3/3] libata: make ata_sg_setup_one() trim zero length sg

[Tejun Heo]

This patch makes ata_sg_setup_one() trim sg entry (thus making
qc->n_elem zero) if padding results in zero length sg entry.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

600e098c7092d0c0ba4c316e2cec2d1c8edc495c
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index bbac87a..5f1d758 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2514,7 +2514,7 @@ static void ata_sg_clean(struct ata_queu
 	assert(sg != NULL);
 
 	if (qc->flags & ATA_QCFLAG_SINGLE)
-		assert(qc->n_elem == 1);
+		assert(qc->n_elem <= 1);
 
 	VPRINTK("unmapping %u sg elements\n", qc->n_elem);
 
@@ -2537,7 +2537,7 @@ static void ata_sg_clean(struct ata_queu
 			kunmap_atomic(addr, KM_IRQ0);
 		}
 	} else {
-		if (sg_dma_len(&sg[0]) > 0)
+		if (qc->n_elem)
 			dma_unmap_single(ap->host_set->dev,
 				sg_dma_address(&sg[0]), sg_dma_len(&sg[0]),
 				dir);
@@ -2715,6 +2715,7 @@ static int ata_sg_setup_one(struct ata_q
 	int dir = qc->dma_dir;
 	struct scatterlist *sg = qc->__sg;
 	dma_addr_t dma_address;
+	int trim_sg = 0;
 
 	/* we must lengthen transfers to end on a 32-bit boundary */
 	qc->pad_len = sg->length & 3;
@@ -2734,13 +2735,15 @@ static int ata_sg_setup_one(struct ata_q
 		sg_dma_len(psg) = ATA_DMA_PAD_SZ;
 		/* trim sg */
 		sg->length -= qc->pad_len;
+		if (sg->length == 0)
+			trim_sg = 1;
 
 		DPRINTK("padding done, sg->length=%u pad_len=%u\n",
 			sg->length, qc->pad_len);
 	}
 
-	if (!sg->length) {
-		sg_dma_address(sg) = 0;
+	if (trim_sg) {
+		qc->n_elem--;
 		goto skip_map;
 	}
 
@@ -2753,9 +2756,9 @@ static int ata_sg_setup_one(struct ata_q
 	}
 
 	sg_dma_address(sg) = dma_address;
-skip_map:
 	sg_dma_len(sg) = sg->length;
 
+skip_map:
 	DPRINTK("mapped buffer of %d bytes for %s\n", sg_dma_len(sg),
 		qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
-- 
1.1.5

[PATCH 2/3] libata: fix qc->n_elem == 0 case handling in ata_qc_next_sg

[Tejun Heo]

This patch makes ata_for_each_sg() start with pad_sgent when
qc->n_elem is zero.  Previously, ata_for_each_sg() unconditionally
started with qc->__sg, handling the first sg to fill_sg() routines
even when the entry was invalid.  And while at it, unwind ?: in
ata_qc_next_sg() into if statement.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 include/linux/libata.h |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

940e8d008d8e53b532de8de994477baf78e7007a
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9e5db29..c91be5e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -557,17 +557,29 @@ ata_sg_is_last(struct scatterlist *sg, s
 }
 
 static inline struct scatterlist *
+ata_qc_first_sg(struct ata_queued_cmd *qc)
+{
+	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 (++sg - qc->__sg < qc->n_elem)
 		return sg;
-	return qc->pad_len ? &qc->pad_sgent : NULL;
+	if (qc->pad_len)
+		return &qc->pad_sgent;
+	return NULL;
 }
 
 #define ata_for_each_sg(sg, qc) \
-	for (sg = qc->__sg; sg; sg = ata_qc_next_sg(sg, qc))
+	for (sg = ata_qc_first_sg(qc); sg; sg = ata_qc_next_sg(sg, qc))
 
 static inline unsigned int ata_tag_valid(unsigned int tag)
 {
-- 
1.1.5