Skip to contentSkip to conversation

add dma_max_segment_size option to scsi_host_template

PATCH In review FUJITA Tomonori 4 participants 8 messages Sep 26, 2007 – Sep 29, 2007

Subsystems (3)
SCSI Driver core DMA mapping helpers
lore
Share & tools
lore

FUJITA Tomonori

author
#0 lore raw

PATCH 1/5 add dma_max_segment_size option to scsi_host_template

This patch moves blk_queue_max_segment_size to scsi_alloc_queue from llds. It enables scsi_add_host to tells iommu lld's dma_max_segment_size. If a low-level driver doesn't specify dma_max_segment_size, scsi-ml uses 65536 (MAX_SEGMENT_SIZE). So there are not any functional changes.

Signed-off-by FUJITA Tomonori <[email protected]>
---
 drivers/scsi/hosts.c     |    5 +++++
 drivers/scsi/scsi_lib.c  |    1 +
 include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 0 deletions(-)
drivers/scsi/hosts.c +5 -0
index 96bc312..22877d3 100644
@@ -365,6 +365,11 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	else
 		shost->dma_boundary = 0xffffffff;
 
+	if (sht->dma_max_segment_size)
+		shost->dma_max_segment_size = sht->dma_max_segment_size;
+	else
+		shost->dma_max_segment_size = 65536;
+
 	rval = scsi_setup_command_freelist(shost);
 	if (rval)
 		goto fail_kfree;
drivers/scsi/scsi_lib.c +1 -0
index a417a6f..2ec77a9 100644
@@ -1564,6 +1564,7 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 	blk_queue_max_sectors(q, shost->max_sectors);
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
+	blk_queue_max_segment_size(q, shost->dma_max_segment_size);
 
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
include/scsi/scsi_host.h +6 -0
index 3b8a6a8..1eb8435 100644
@@ -383,6 +383,11 @@ struct scsi_host_template {
 	unsigned long dma_boundary;
 
 	/*
+	 * dma scatter gather segment size limit.
+	 */
+	unsigned int dma_max_segment_size;
+
+	/*
 	 * This specifies "machine infinity" for host templates which don't
 	 * limit the transfer size.  Note this limit represents an absolute
 	 * maximum, and may be over the transfer limits allowed for

@@ -571,6 +576,7 @@ struct Scsi_Host {
 	short unsigned int sg_tablesize;
 	short unsigned int max_sectors;
 	unsigned long dma_boundary;
+	unsigned int dma_max_segment_size;
 	/* 
 	 * Used to assign serial numbers to the cmds.
 	 * Protected by the host lock.
Signature · 1 line
1.5.2.4
#1 lore raw
Quoted 18 lines · FUJITA Tomonori

This patch moves blk_queue_max_segment_size to scsi_alloc_queue from llds. It enables scsi_add_host to tells iommu lld's dma_max_segment_size. If a low-level driver doesn't specify dma_max_segment_size, scsi-ml uses 65536 (MAX_SEGMENT_SIZE). So there are not any functional changes.

Signed-off-by FUJITA Tomonori <[email protected]>
---
 drivers/scsi/hosts.c     |    5 +++++
 drivers/scsi/scsi_lib.c  |    1 +
 include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 0 deletions(-)
drivers/scsi/hosts.c +0 -0
index 96bc312..22877d3 100644

drivers/scsi/hosts.c struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) · line 371

Quoted 7 lines
drivers/scsi/hosts.c +4 -0
@@ -365,6 +365,11 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	else
 		shost->dma_boundary = 0xffffffff;
 
+	if (sht->dma_max_segment_size)
+		shost->dma_max_segment_size = sht->dma_max_segment_size;
+	else
+		shost->dma_max_segment_size = 65536;

Use MAX_SEGMENT_SIZE here, instead of manually entering 64k?

Otherwise everything looks good from a quick look, it's definitely missing functionality that we have wanted for quite some time!

Signature · 1 line
Jens Axboe
#2 lore raw
In reply to FUJITA Tomonori
Quoted 6 lines · FUJITA Tomonori

This patch moves blk_queue_max_segment_size to scsi_alloc_queue from llds. It enables scsi_add_host to tells iommu lld's dma_max_segment_size. If a low-level driver doesn't specify dma_max_segment_size, scsi-ml uses 65536 (MAX_SEGMENT_SIZE). So there are not any functional changes.

Quoted 6 lines
Signed-off-by FUJITA Tomonori <[email protected]>
---
 drivers/scsi/hosts.c     |    5 +++++
 drivers/scsi/scsi_lib.c  |    1 +
 include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 0 deletions(-)

hmmmmm... All the patches look technically correct, but IMO this really should behave more the the dma_mask interface: platform sets a sane dma_mask (usually 0xffffffff), and LLDD calls dma_set_mask() or pci_set_dma_mask().

Thus, IMO an LLDD should call dma_set_max_seg(), and then SCSI midlayer can obtain that value from struct device.

Just like dma_mask, I think we can avoid a scsi_host_template addition.

I echo Jens sentiment, though, in closing: thanks for doing this work, it's been needed for quite a while.

Jeff

author
#3 lore raw
Quoted 14 lines · On Wed, 26 Sep 2007 06:11:45 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 6 lines · FUJITA Tomonori

This patch moves blk_queue_max_segment_size to scsi_alloc_queue from llds. It enables scsi_add_host to tells iommu lld's dma_max_segment_size. If a low-level driver doesn't specify dma_max_segment_size, scsi-ml uses 65536 (MAX_SEGMENT_SIZE). So there are not any functional changes.

Quoted 6 lines
Signed-off-by FUJITA Tomonori <[email protected]>
---
 drivers/scsi/hosts.c     |    5 +++++
 drivers/scsi/scsi_lib.c  |    1 +
 include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 0 deletions(-)
Quoted 7 lines

hmmmmm... All the patches look technically correct, but IMO this really should behave more the the dma_mask interface: platform sets a sane dma_mask (usually 0xffffffff), and LLDD calls dma_set_mask() or pci_set_dma_mask().

Thus, IMO an LLDD should call dma_set_max_seg(), and then SCSI midlayer can obtain that value from struct device.

Yeah, I agreed that max_segment_size should work like dma_mask (that's why I simply put max_segment_size to device structure).

scsi_debug doesn't use dma but calls blk_queue_max_segment_size (I guess that it wants large I/Os). If we can remove it (thanks to chaining sg), scsi-ml gets that value that llds set via dma_set_max_seg and calls blk_queue_max_segment_size.

#4 lore raw
Quoted 16 lines · FUJITA Tomonori
Quoted 13 lines · On Wed, 26 Sep 2007 06:11:45 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 12 lines · FUJITA Tomonori

This patch moves blk_queue_max_segment_size to scsi_alloc_queue from llds. It enables scsi_add_host to tells iommu lld's dma_max_segment_size. If a low-level driver doesn't specify dma_max_segment_size, scsi-ml uses 65536 (MAX_SEGMENT_SIZE). So there are not any functional changes.

Signed-off-by FUJITA Tomonori <[email protected]>
---
 drivers/scsi/hosts.c     |    5 +++++
 drivers/scsi/scsi_lib.c  |    1 +
 include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 0 deletions(-)
Quoted 6 lines
Quoted 3 lines

hmmmmm... All the patches look technically correct, but IMO this really should behave more the the dma_mask interface: platform sets a sane dma_mask (usually 0xffffffff), and LLDD calls dma_set_mask() or pci_set_dma_mask().

Thus, IMO an LLDD should call dma_set_max_seg(), and then SCSI midlayer can obtain that value from struct device.

Yeah, I agreed that max_segment_size should work like dma_mask (that's why I simply put max_segment_size to device structure).

Yep!

Quoted 4 lines

scsi_debug doesn't use dma but calls blk_queue_max_segment_size (I guess that it wants large I/Os). If we can remove it (thanks to chaining sg), scsi-ml gets that value that llds set via dma_set_max_seg and calls blk_queue_max_segment_size.

[/me checks the code] Actually scsi_debug has its own pseudo-bus and struct device, so it sounds like scsi_debug can call dma_set_max_seg() just like any other LLDD?

Maybe dev_set_max_seg() is a better name, if people get really picky (I don't care).

Jeff

author
#5 lore raw
Quoted 31 lines · On Wed, 26 Sep 2007 10:42:06 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 21 lines · FUJITA Tomonori
Quoted 18 lines · On Wed, 26 Sep 2007 06:11:45 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 6 lines · FUJITA Tomonori

This patch moves blk_queue_max_segment_size to scsi_alloc_queue from llds. It enables scsi_add_host to tells iommu lld's dma_max_segment_size. If a low-level driver doesn't specify dma_max_segment_size, scsi-ml uses 65536 (MAX_SEGMENT_SIZE). So there are not any functional changes.

Quoted 6 lines
Signed-off-by FUJITA Tomonori <[email protected]>
---
 drivers/scsi/hosts.c     |    5 +++++
 drivers/scsi/scsi_lib.c  |    1 +
 include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 0 deletions(-)

hmmmmm... All the patches look technically correct, but IMO this really should behave more the the dma_mask interface: platform sets a sane dma_mask (usually 0xffffffff), and LLDD calls dma_set_mask() or pci_set_dma_mask().

Quoted 5 lines
Quoted 2 lines

Thus, IMO an LLDD should call dma_set_max_seg(), and then SCSI midlayer can obtain that value from struct device.

Yeah, I agreed that max_segment_size should work like dma_mask (that's why I simply put max_segment_size to device structure).

Yep!

Quoted 8 lines
Quoted 4 lines

scsi_debug doesn't use dma but calls blk_queue_max_segment_size (I guess that it wants large I/Os). If we can remove it (thanks to chaining sg), scsi-ml gets that value that llds set via dma_set_max_seg and calls blk_queue_max_segment_size.

[/me checks the code] Actually scsi_debug has its own pseudo-bus and struct device, so it sounds like scsi_debug can call dma_set_max_seg() just like any other LLDD?

You are right. scsi_debug's pseudo-bus works.

But probabaly, scsi_debug doesn't need to call
blk_queue_max_segment_size now.

Quoted 2 lines

Maybe dev_set_max_seg() is a better name, if people get really picky (I don't care).

How about dma_set_max_seg_size()?

drivers/scsi/scsi_lib.c +4 -0
index a417a6f..7adadfb 100644
@@ -1554,6 +1554,7 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 					 request_fn_proc *request_fn)
 {
 	struct request_queue *q;
+	struct device *dev = shost->shost_gendev.parent;
 
 	q = blk_init_queue(request_fn, NULL);
 	if (!q)

@@ -1565,6 +1566,9 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 
+	if (dev->max_segment_size)
+		blk_queue_max_segment_size(q, dev->max_segment_size);
+
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	return q;
include/linux/device.h +7 -0
index 3a38d1f..8046b60 100644
@@ -443,6 +443,13 @@ struct device {
 
 	struct dma_coherent_mem	*dma_mem; /* internal for coherent mem
 					     override */
+
+	/*
+	 * a low level driver may set these to teach IOMMU code about
+	 * sg limitations.
+	 */
+	unsigned int max_segment_size;
+
 	/* arch specific additions */
 	struct dev_archdata	archdata;
include/linux/dma-mapping.h +5 -0
index 2dc21cb..30404b8 100644
@@ -31,6 +31,11 @@ static inline int valid_dma_direction(int dma_direction)
 		(dma_direction == DMA_FROM_DEVICE));
 }
 
+static inline void dma_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	dev->max_segment_size = size;
+}
+
 #ifdef CONFIG_HAS_DMA
 #include <asm/dma-mapping.h>
 #else
#6 lore raw
Quoted 4 lines · FUJITA Tomonori

You are right. scsi_debug's pseudo-bus works.

But probabaly, scsi_debug doesn't need to call
blk_queue_max_segment_size now.

Either way works for me. :)

Quoted 50 lines
Quoted 1 line

Maybe dev_set_max_seg() is a better name, if people get really picky (I don't care).

How about dma_set_max_seg_size()?

drivers/scsi/scsi_lib.c +4 -0
index a417a6f..7adadfb 100644
@@ -1554,6 +1554,7 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 					 request_fn_proc *request_fn)
 {
 	struct request_queue *q;
+	struct device *dev = shost->shost_gendev.parent;
 
 	q = blk_init_queue(request_fn, NULL);
 	if (!q)

@@ -1565,6 +1566,9 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 
+	if (dev->max_segment_size)
+		blk_queue_max_segment_size(q, dev->max_segment_size);
+
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	return q;
include/linux/device.h +7 -0
index 3a38d1f..8046b60 100644
@@ -443,6 +443,13 @@ struct device {
 
 	struct dma_coherent_mem	*dma_mem; /* internal for coherent mem
 					     override */
+
+	/*
+	 * a low level driver may set these to teach IOMMU code about
+	 * sg limitations.
+	 */
+	unsigned int max_segment_size;
+
 	/* arch specific additions */
 	struct dev_archdata	archdata;
include/linux/dma-mapping.h +0 -0
index 2dc21cb..30404b8 100644

include/linux/dma-mapping.h static inline int valid_dma_direction(int dma_direction) · line 38

Quoted 8 lines
include/linux/dma-mapping.h +5 -0
@@ -31,6 +31,11 @@ static inline int valid_dma_direction(int dma_direction)
 		(dma_direction == DMA_FROM_DEVICE));
 }
 
+static inline void dma_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	dev->max_segment_size = size;
+}
+

Definitely moving in the right direction, IMO. I would suggest a few minor changes to dma_set_max_seg_size(), to permit platforms to override the default behavior, and to get people into the habit of checking the return value (since it might matter in the future):

1) surround with #ifndef ARCH_HAS_DMA_MAX_SEG

2) change return value to 'int'

3) unconditionally return zero, for now.

Otherwise, approach and code look good to me.

Jeff

#7 lore raw
Quoted 74 lines · On Thu, 27 Sep 2007 02:31:14 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 4 lines · FUJITA Tomonori

You are right. scsi_debug's pseudo-bus works.

But probabaly, scsi_debug doesn't need to call
blk_queue_max_segment_size now.

Either way works for me. :)

Quoted 51 lines
Quoted 2 lines

Maybe dev_set_max_seg() is a better name, if people get really picky (I don't care).

How about dma_set_max_seg_size()?

drivers/scsi/scsi_lib.c +4 -0
index a417a6f..7adadfb 100644
@@ -1554,6 +1554,7 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 					 request_fn_proc *request_fn)
 {
 	struct request_queue *q;
+	struct device *dev = shost->shost_gendev.parent;
 
 	q = blk_init_queue(request_fn, NULL);
 	if (!q)

@@ -1565,6 +1566,9 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 
+	if (dev->max_segment_size)
+		blk_queue_max_segment_size(q, dev->max_segment_size);
+
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	return q;
include/linux/device.h +7 -0
index 3a38d1f..8046b60 100644
@@ -443,6 +443,13 @@ struct device {
 
 	struct dma_coherent_mem	*dma_mem; /* internal for coherent mem
 					     override */
+
+	/*
+	 * a low level driver may set these to teach IOMMU code about
+	 * sg limitations.
+	 */
+	unsigned int max_segment_size;
+
 	/* arch specific additions */
 	struct dev_archdata	archdata;
include/linux/dma-mapping.h +0 -0
index 2dc21cb..30404b8 100644

include/linux/dma-mapping.h static inline int valid_dma_direction(int dma_direction) · line 38

Quoted 8 lines
include/linux/dma-mapping.h +5 -0
@@ -31,6 +31,11 @@ static inline int valid_dma_direction(int dma_direction)
 		(dma_direction == DMA_FROM_DEVICE));
 }
 
+static inline void dma_set_max_seg_size(struct device *dev, unsigned int size)
+{
+	dev->max_segment_size = size;
+}
+

Definitely moving in the right direction, IMO. I would suggest a few minor changes to dma_set_max_seg_size(), to permit platforms to override the default behavior, and to get people into the habit of checking the return value (since it might matter in the future):

Quoted 7 lines

1) surround with #ifndef ARCH_HAS_DMA_MAX_SEG

2) change return value to 'int'

3) unconditionally return zero, for now.

Otherwise, approach and code look good to me.

I will, thanks!

I'll post an updated patchset for -mm kernel soon.