add sg segment limitation info to device structure

PATCH In review FUJITA Tomonori 6 participants 17 messages Sep 26, 2007 – Oct 3, 2007

Subsystems (4)

SCSI PCI Driver core DMA mapping helpers

lore

Share & tools

lore

FUJITA Tomonori

author

2007-09-26 08:58 UTC #0 lore raw

PATCH 3/5 add sg segment limitation info to device structure

iommu code merges sg segments without considering lld's sg segment restrictions. iommu code can't access to the limitations because they are in request_queue. This patch adds max_segment_size to device structure. seg_boundary_mask will be added too later.

Signed-off-by FUJITA Tomonori <[email protected]>

---
 include/linux/device.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

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;

Signature · 1 line

1.5.2.4

Greg KH

2007-09-26 16:13 UTC #1 lore raw

Quoted 15 lines · FUJITA Tomonori

iommu code merges sg segments without considering lld's sg segment restrictions. iommu code can't access to the limitations because they are in request_queue. This patch adds max_segment_size to device structure. seg_boundary_mask will be added too later.
Signed-off-by FUJITA Tomonori <[email protected]>
---
 include/linux/device.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
include/linux/device.h +0 -0
index 3a38d1f..8046b60 100644

include/linux/device.h struct device { · line 451

Quoted 9 lines

include/linux/device.h +6 -0

@@ -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;

Does this really need to be here? Can't it go into the bus specific device that needs this?

thanks,

greg k-h

FUJITA Tomonori

author

2007-09-27 00:38 UTC #2 lore raw

Quoted 17 lines · On Wed, 26 Sep 2007 09:05:58 -0700 Greg KH <[email protected]> wrote:

Quoted 16 lines · FUJITA Tomonori
iommu code merges sg segments without considering lld's sg segment restrictions. iommu code can't access to the limitations because they are in request_queue. This patch adds max_segment_size to device structure. seg_boundary_mask will be added too later.
Signed-off-by FUJITA Tomonori <[email protected]>
---
 include/linux/device.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
include/linux/device.h +0 -0
index 3a38d1f..8046b60 100644
@@ -443,6 +443,13 @@ struct device {
 

Quoted 11 lines

include/linux/device.h struct device { · line 451
Quoted 8 lines
include/linux/device.h +6 -0
 	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;
Does this really need to be here? Can't it go into the bus specific device that needs this?

dma_map_sg() is bus specific? I think that this really need to be. It needs to work like dma_mask in device structure.

James Bottomley

2007-10-01 23:36 UTC #3 lore raw

In reply to Greg KH

Quoted 17 lines · Greg KH

Quoted 16 lines · FUJITA Tomonori
iommu code merges sg segments without considering lld's sg segment restrictions. iommu code can't access to the limitations because they are in request_queue. This patch adds max_segment_size to device structure. seg_boundary_mask will be added too later.
Signed-off-by FUJITA Tomonori <[email protected]>
---
 include/linux/device.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
include/linux/device.h +0 -0
index 3a38d1f..8046b60 100644
@@ -443,6 +443,13 @@ struct device {
 

Quoted 11 lines

include/linux/device.h struct device { · line 451
Quoted 8 lines
include/linux/device.h +6 -0
 	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;
Does this really need to be here? Can't it go into the bus specific device that needs this?

Unfortunately, no. The IOMMU may not be on the bus for the device (on HPPA for instance, the IOMMU sits on the runway bus which is plugged into the parisc bus, which finally plugs into the PCI bus) which means that the iommu itself can only deal with generic devices (because it doesn't want to do massive casing on the possible busses).

One possibility we could do is to add a

struct dma_device {
	struct device dev;
	u64 dma_mask;
	u64 coherent_dma_mask;
	unsigned int max_segment_size;
	/* plus any other DMA parameters */
};

but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.

James

Matthew Wilcox

2007-10-02 01:39 UTC #4 lore raw

Quoted 10 lines · James Bottomley

One possibility we could do is to add a

struct dma_device {
	struct device dev;
	u64 dma_mask;
	u64 coherent_dma_mask;
	unsigned int max_segment_size;
	/* plus any other DMA parameters */
};

Quoted 5 lines

but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.

If we're going to do this, then we should go further and include:
- unsigned int irq
- struct resources (how many?)
- struct list_head dma_pools

dma_device might not be the right name. Really, we're talking about 'internal' device types (EISA, PCI, GSC, Zorro, ...) as opposed to external like usb.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Greg KH

2007-10-02 04:20 UTC #5 lore raw

Quoted 11 lines · Matthew Wilcox

Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
	struct device dev;
	u64 dma_mask;
	u64 coherent_dma_mask;
	unsigned int max_segment_size;
	/* plus any other DMA parameters */
};

Quoted 5 lines

Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.

Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?

thanks,

greg k-h

James Bottomley

2007-10-02 14:45 UTC #6 lore raw

Quoted 18 lines · Greg KH

Quoted 11 lines · Matthew Wilcox
Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
	struct device dev;
	u64 dma_mask;
	u64 coherent_dma_mask;
	unsigned int max_segment_size;
	/* plus any other DMA parameters */
};
Quoted 5 lines
Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.

Quoted 3 lines

Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?

Well, that's just a bit ugly ... I assume you're thinking of adding a struct device_dma_parameters, and then defining the platform device as

struct pci_dev {
	...
	struct device dev;
	struct device_dma_parameters dma_parms;
	...
};

and then setting up the pointer?

James

Kay Sievers

2007-10-02 15:02 UTC #7 lore raw

Quoted 26 lines · James Bottomley

Quoted 18 lines · Greg KH
Quoted 11 lines · Matthew Wilcox
Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
struct device dev;
u64 dma_mask;
u64 coherent_dma_mask;
unsigned int max_segment_size;
/* plus any other DMA parameters */
};
Quoted 5 lines
Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.
Quoted 3 lines
Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?
Well, that's just a bit ugly ... I assume you're thinking of adding a struct device_dma_parameters, and then defining the platform device as

Quoted 8 lines

struct pci_dev {
        ...
        struct device dev;
        struct device_dma_parameters dma_parms;
        ...
};

and then setting up the pointer?

I guess Greg means:

  struct device {
      ...
      struct device_dma_parameters *dma_parms;

}

and allocate dma_parms on demand.

Kay

James Bottomley

2007-10-02 15:05 UTC #8 lore raw

Quoted 36 lines · Kay Sievers

Quoted 26 lines · James Bottomley
Quoted 18 lines · Greg KH
Quoted 11 lines · Matthew Wilcox
Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
struct device dev;
u64 dma_mask;
u64 coherent_dma_mask;
unsigned int max_segment_size;
/* plus any other DMA parameters */
};
Quoted 5 lines
Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.
Quoted 3 lines
Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?
Well, that's just a bit ugly ... I assume you're thinking of adding a struct device_dma_parameters, and then defining the platform device as
Quoted 8 lines
struct pci_dev {
        ...
        struct device dev;
        struct device_dma_parameters dma_parms;
        ...
};
and then setting up the pointer?

Quoted 7 lines

I guess Greg means:
  struct device {
      ...
      struct device_dma_parameters *dma_parms;
}
and allocate dma_parms on demand.

But they're demanded by every bus (with the possible exception of PCMCIA) because they'll be holding the dma mask. Sure, we could do a separate allocation in every bus device creation routine, but isn't that even more complex than sticking them in the global allocation of the bus device because the failure modes are now more complex?

James

Kay Sievers

2007-10-02 15:09 UTC #9 lore raw

Quoted 43 lines · James Bottomley

Quoted 42 lines · Kay Sievers
Quoted 26 lines · James Bottomley
Quoted 18 lines · Greg KH
Quoted 11 lines · Matthew Wilcox
Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
struct device dev;
u64 dma_mask;
u64 coherent_dma_mask;
unsigned int max_segment_size;
/* plus any other DMA parameters */
};
Quoted 5 lines
Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.
Quoted 3 lines
Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?
Well, that's just a bit ugly ... I assume you're thinking of adding a struct device_dma_parameters, and then defining the platform device as
Quoted 8 lines
struct pci_dev {
        ...
        struct device dev;
        struct device_dma_parameters dma_parms;
        ...
};
and then setting up the pointer?
I guess Greg means:
  struct device {
      ...
      struct device_dma_parameters *dma_parms;
}

Quoted 7 lines

Quoted 1 line
and allocate dma_parms on demand.
But they're demanded by every bus (with the possible exception of PCMCIA) because they'll be holding the dma mask. Sure, we could do a separate allocation in every bus device creation routine, but isn't that even more complex than sticking them in the global allocation of the bus device because the failure modes are now more complex?

Hmm, SCSI devices are bus devices too, will they need it?

Kay

James Bottomley

2007-10-02 15:14 UTC #10 lore raw

Quoted 46 lines · Kay Sievers

Quoted 45 lines · James Bottomley
Quoted 43 lines · Kay Sievers
Quoted 26 lines · James Bottomley
Quoted 18 lines · Greg KH
Quoted 11 lines · Matthew Wilcox
Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
struct device dev;
u64 dma_mask;
u64 coherent_dma_mask;
unsigned int max_segment_size;
/* plus any other DMA parameters */
};
Quoted 5 lines
Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.
Quoted 3 lines
Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?
Well, that's just a bit ugly ... I assume you're thinking of adding a struct device_dma_parameters, and then defining the platform device as
Quoted 8 lines
struct pci_dev {
        ...
        struct device dev;
        struct device_dma_parameters dma_parms;
        ...
};
and then setting up the pointer?
I guess Greg means:
  struct device {
      ...
      struct device_dma_parameters *dma_parms;
}
and allocate dma_parms on demand.

Quoted 7 lines

Quoted 5 lines
But they're demanded by every bus (with the possible exception of PCMCIA) because they'll be holding the dma mask. Sure, we could do a separate allocation in every bus device creation routine, but isn't that even more complex than sticking them in the global allocation of the bus device because the failure modes are now more complex?
Hmm, SCSI devices are bus devices too, will they need it?

Actually, I don't think of SCSI devices as bus devices ... but the answer's no, they don't. Only devices on a DMA'able bus (like PCI, parisc, sbus etc.)

James

Kay Sievers

2007-10-02 15:23 UTC #11 lore raw

Quoted 53 lines · James Bottomley

Quoted 52 lines · Kay Sievers
Quoted 50 lines · James Bottomley
Quoted 36 lines · Kay Sievers
Quoted 26 lines · James Bottomley
Quoted 18 lines · Greg KH
Quoted 11 lines · Matthew Wilcox
Quoted 10 lines · James Bottomley
One possibility we could do is to add a
struct dma_device {
struct device dev;
u64 dma_mask;
u64 coherent_dma_mask;
unsigned int max_segment_size;
/* plus any other DMA parameters */
};
Quoted 5 lines
Quoted 5 lines
but then every bus that can do DMA would need to include a struct dma_device instead of the struct device they do now. Then the IOMMU would know it could cast out from struct device to struct dma_device, but this would be a lot of work to thread through the current infrastructure.
Quoted 3 lines
Why not just hang these fields off of a struct device, that way if the device doesn't/can't do dma, it only has the "loss" of a single pointer, not all of these fields?
Well, that's just a bit ugly ... I assume you're thinking of adding a struct device_dma_parameters, and then defining the platform device as
Quoted 8 lines
struct pci_dev {
        ...
        struct device dev;
        struct device_dma_parameters dma_parms;
        ...
};
and then setting up the pointer?
Quoted 7 lines
I guess Greg means:
  struct device {
      ...
      struct device_dma_parameters *dma_parms;
}
and allocate dma_parms on demand.
But they're demanded by every bus (with the possible exception of PCMCIA) because they'll be holding the dma mask. Sure, we could do a separate allocation in every bus device creation routine, but isn't that even more complex than sticking them in the global allocation of the bus device because the failure modes are now more complex?

Quoted 5 lines

Quoted 1 line
Hmm, SCSI devices are bus devices too, will they need it?
Actually, I don't think of SCSI devices as bus devices ... but the answer's no, they don't. Only devices on a DMA'able bus (like PCI, parisc, sbus etc.)

Ah, I see. Seems we have far more "struct device" devices on a system which don't do DMA then. And in some setups also more "struct device" devices belonging to a "struct bus" without doing any DMA.

Just looking at the number of devices, it seems that allocating it dynamically would be the better deal. We allocate the name of every kobject dynamically today, so I guess it's fine to do that with the DMA data too.

Kay

Matthew Wilcox

2007-10-02 15:25 UTC #12 lore raw

Quoted 4 lines · Kay Sievers

Just looking at the number of devices, it seems that allocating it dynamically would be the better deal. We allocate the name of every kobject dynamically today, so I guess it's fine to do that with the DMA data too.

But we don't need to allocate it dynamically. We can embed it in the pci_dev, eisa_dev, zorro_dev, mca_dev and parisc_device.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Greg KH

2007-10-02 16:49 UTC #13 lore raw

Quoted 8 lines · Matthew Wilcox

Quoted 4 lines · Kay Sievers
Just looking at the number of devices, it seems that allocating it dynamically would be the better deal. We allocate the name of every kobject dynamically today, so I guess it's fine to do that with the DMA data too.
But we don't need to allocate it dynamically. We can embed it in the pci_dev, eisa_dev, zorro_dev, mca_dev and parisc_device.

But then you run into the issue that James pointed out originally.

Anyway, I don't care which, let's see some patches :)

thanks,

greg k-h

FUJITA Tomonori

author

2007-10-03 14:20 UTC #14 lore raw

Quoted 7 lines · On Tue, 2 Oct 2007 09:44:13 -0700 Greg KH <[email protected]> wrote:

Quoted 6 lines · Matthew Wilcox
Quoted 4 lines · Kay Sievers
Just looking at the number of devices, it seems that allocating it dynamically would be the better deal. We allocate the name of every kobject dynamically today, so I guess it's fine to do that with the DMA data too.

Quoted 6 lines

Quoted 2 lines
But we don't need to allocate it dynamically. We can embed it in the pci_dev, eisa_dev, zorro_dev, mca_dev and parisc_device.
But then you run into the issue that James pointed out originally.
Anyway, I don't care which, let's see some patches :)

How about this (based on James' proposal)?

- Currently, there are only max_segment_size and segment_boundary_mask in struct device_dma_parameters (I'll add segment_boundary_mask support later after I finish the iommu part). We'll move more dma stuff in struct device (like dma_mask) to struct device_dma_parameters later (needs some cleanups before that).

- New accessors for the dma parameters are added. So we can easily change where to place struct device_dma_parameters in the future.

- the default max_segment_size is set to 64K, same to the block layer's default value.

drivers/pci/pci.c +8 -0

index 37c00f6..c93ebe8 100644
@@ -1392,6 +1392,13 @@ pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask)
 }
 #endif
 
+#ifndef HAVE_ARCH_PCI_SET_DMA_MAX_SEGMENT_SIZE
+int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size)
+{
+	return dma_set_max_seg_size(&dev->dev, size);
+}
+#endif
+
 /**
  * pcix_get_max_mmrbc - get PCI-X maximum designed memory read byte count
  * @dev: PCI device to query

@@ -1624,6 +1631,7 @@ EXPORT_SYMBOL(pci_clear_mwi);
 EXPORT_SYMBOL_GPL(pci_intx);
 EXPORT_SYMBOL(pci_set_dma_mask);
 EXPORT_SYMBOL(pci_set_consistent_dma_mask);
+EXPORT_SYMBOL(pci_set_dma_max_seg_size);
 EXPORT_SYMBOL(pci_assign_resource);
 EXPORT_SYMBOL(pci_find_parent_resource);
 EXPORT_SYMBOL(pci_select_bars);

drivers/pci/probe.c +3 -0

index 171ca71..2a18134 100644
@@ -948,8 +948,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 
 	set_dev_node(&dev->dev, pcibus_to_node(bus));
 	dev->dev.dma_mask = &dev->dma_mask;
+	dev->dev.dma_parms = &dev->dma_parms;
 	dev->dev.coherent_dma_mask = 0xffffffffull;
 
+	pci_set_dma_max_seg_size(dev, 65536);
+
 	/* Fix up broken headers */
 	pci_fixup_device(pci_fixup_header, dev);

include/linux/device.h +11 -0

index 3a38d1f..3c24932 100644
@@ -404,6 +404,15 @@ extern int devres_release_group(struct device *dev, void *id);
 extern void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp);
 extern void devm_kfree(struct device *dev, void *p);
 
+struct device_dma_parameters {
+	/*
+	 * a low level driver may set these to teach IOMMU code about
+	 * sg limitations.
+	 */
+	unsigned int max_segment_size;
+	unsigned long segment_boundary_mask;
+};
+
 struct device {
 	struct klist		klist_children;
 	struct klist_node	knode_parent;		/* node in sibling list */

@@ -439,6 +448,8 @@ struct device {
 					     64 bit addresses for consistent
 					     allocations such descriptors. */
 
+	struct device_dma_parameters *dma_parms;
+
 	struct list_head	dma_pools;	/* dma pools (if dma'ble) */
 
 	struct dma_coherent_mem	*dma_mem; /* internal for coherent mem

include/linux/dma-mapping.h +15 -0

index 2dc21cb..02854c8 100644
@@ -43,6 +43,21 @@ static inline int valid_dma_direction(int dma_direction)
 
 extern u64 dma_get_required_mask(struct device *dev);
 
+static inline unsigned int dma_get_max_seg_size(struct device *dev)
+{
+	return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536;
+}
+
+static inline unsigned int dma_set_max_seg_size(struct device *dev,
+						unsigned int size)
+{
+	if (dev->dma_parms) {
+		dev->dma_parms->max_segment_size = size;
+		return 0;
+	} else
+		return -EIO;
+}
+
 /* flags for the coherent memory api */
 #define	DMA_MEMORY_MAP			0x01
 #define DMA_MEMORY_IO			0x02

include/linux/pci.h +4 -0

index 038a0dc..1d630b5 100644
@@ -151,6 +151,8 @@ struct pci_dev {
 					   this if your device has broken DMA
 					   or supports 64-bit transfers.  */
 
+	struct device_dma_parameters dma_parms;
+
 	pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
 					   this is D0-D3, D0 being fully functional,
 					   and D3 being off. */

@@ -554,6 +556,7 @@ void pci_intx(struct pci_dev *dev, int enable);
 void pci_msi_off(struct pci_dev *dev);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
+int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size);
 int pcix_get_max_mmrbc(struct pci_dev *dev);
 int pcix_get_mmrbc(struct pci_dev *dev);
 int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc);

@@ -739,6 +742,7 @@ static inline void pci_set_master(struct pci_dev *dev) { }
 static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
 static inline void pci_disable_device(struct pci_dev *dev) { }
 static inline int pci_set_dma_mask(struct pci_dev *dev, u64 mask) { return -EIO; }
+static inline int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size) { return -EIO; }
 static inline int pci_assign_resource(struct pci_dev *dev, int i) { return -EBUSY;}
 static inline int __pci_register_driver(struct pci_driver *drv, struct module *owner) { return 0;}
 static inline int pci_register_driver(struct pci_driver *drv) { return 0;}

Jeff Garzik

2007-10-03 17:59 UTC #15 lore raw

Quoted 23 lines · FUJITA Tomonori

Quoted 11 lines · On Tue, 2 Oct 2007 09:44:13 -0700 Greg KH <[email protected]> wrote:
Quoted 7 lines · Matthew Wilcox
Quoted 4 lines · Kay Sievers
Just looking at the number of devices, it seems that allocating it dynamically would be the better deal. We allocate the name of every kobject dynamically today, so I guess it's fine to do that with the DMA data too.
But we don't need to allocate it dynamically. We can embed it in the pci_dev, eisa_dev, zorro_dev, mca_dev and parisc_device.
But then you run into the issue that James pointed out originally.
Anyway, I don't care which, let's see some patches :)
How about this (based on James' proposal)?
- Currently, there are only max_segment_size and segment_boundary_mask in struct device_dma_parameters (I'll add segment_boundary_mask support later after I finish the iommu part). We'll move more dma stuff in struct device (like dma_mask) to struct device_dma_parameters later (needs some cleanups before that).

Quoted 5 lines

- New accessors for the dma parameters are added. So we can easily change where to place struct device_dma_parameters in the future.
- the default max_segment_size is set to 64K, same to the block layer's default value.

ACK

Greg KH

2007-10-03 21:50 UTC #16 lore raw

In reply to FUJITA Tomonori

Quoted 51 lines · FUJITA Tomonori

Quoted 7 lines · On Tue, 2 Oct 2007 09:44:13 -0700 Greg KH <[email protected]> wrote:
Quoted 6 lines · Matthew Wilcox
Quoted 4 lines · Kay Sievers
Just looking at the number of devices, it seems that allocating it dynamically would be the better deal. We allocate the name of every kobject dynamically today, so I guess it's fine to do that with the DMA data too.
Quoted 6 lines
Quoted 2 lines
But we don't need to allocate it dynamically. We can embed it in the pci_dev, eisa_dev, zorro_dev, mca_dev and parisc_device.
But then you run into the issue that James pointed out originally.
Anyway, I don't care which, let's see some patches :)
How about this (based on James' proposal)?
- Currently, there are only max_segment_size and segment_boundary_mask in struct device_dma_parameters (I'll add segment_boundary_mask support later after I finish the iommu part). We'll move more dma stuff in struct device (like dma_mask) to struct device_dma_parameters later (needs some cleanups before that).
- New accessors for the dma parameters are added. So we can easily change where to place struct device_dma_parameters in the future.
- the default max_segment_size is set to 64K, same to the block layer's default value.
drivers/pci/pci.c +7 -0
index 37c00f6..c93ebe8 100644
@@ -1392,6 +1392,13 @@ pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask)
 }
 #endif
 
+#ifndef HAVE_ARCH_PCI_SET_DMA_MAX_SEGMENT_SIZE
+int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size)
+{
+	return dma_set_max_seg_size(&dev->dev, size);
+}
+#endif
+
 /**
  * pcix_get_max_mmrbc - get PCI-X maximum designed memory read byte count
  * @dev: PCI device to query

drivers/pci/pci.c EXPORT_SYMBOL(pci_clear_mwi); · line 1637

Quoted 7 lines

drivers/pci/pci.c +1 -0

@@ -1624,6 +1631,7 @@ EXPORT_SYMBOL(pci_clear_mwi);
 EXPORT_SYMBOL_GPL(pci_intx);
 EXPORT_SYMBOL(pci_set_dma_mask);
 EXPORT_SYMBOL(pci_set_consistent_dma_mask);
+EXPORT_SYMBOL(pci_set_dma_max_seg_size);
 EXPORT_SYMBOL(pci_assign_resource);
 EXPORT_SYMBOL(pci_find_parent_resource);
 EXPORT_SYMBOL(pci_select_bars);

Export the symbol right next to the function, that way the #ifdef doesn't have to be duplicated.

other than that minor problem, this looks good to me. James, any objection to this?

thanks,

greg k-h