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

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

[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 <fujita.tomonori@lab.ntt.co.jp>
---
 include/linux/device.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 3a38d1f..8046b60 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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;
 
-- 
1.5.2.4

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Greg KH]

On Wed, Sep 26, 2007 at 05:58:01PM +0900, FUJITA Tomonori wrote:
> 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 <fujita.tomonori@lab.ntt.co.jp>
> ---
>  include/linux/device.h |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 3a38d1f..8046b60 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[FUJITA Tomonori]

On Wed, 26 Sep 2007 09:05:58 -0700
Greg KH <gregkh@suse.de> wrote:

> On Wed, Sep 26, 2007 at 05:58:01PM +0900, FUJITA Tomonori wrote:
> > 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 <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  include/linux/device.h |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 3a38d1f..8046b60 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -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?

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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[James Bottomley]

On Wed, 2007-09-26 at 09:05 -0700, Greg KH wrote:
> On Wed, Sep 26, 2007 at 05:58:01PM +0900, FUJITA Tomonori wrote:
> > 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 <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  include/linux/device.h |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 3a38d1f..8046b60 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -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?

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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Matthew Wilcox]

On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> 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.

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.

-- 
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."

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Greg KH]

On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > 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.

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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[James Bottomley]

On Mon, 2007-10-01 at 21:22 -0700, Greg KH wrote:
> On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> > On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > > 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.
> 
> 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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Kay Sievers]

On 10/2/07, James Bottomley <James.Bottomley@steeleye.com> wrote:
> On Mon, 2007-10-01 at 21:22 -0700, Greg KH wrote:
> > On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> > > On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > > > 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.
> >
> > 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?

I guess Greg means:
  struct device {
      ...
      struct device_dma_parameters *dma_parms;
  }

and allocate dma_parms on demand.

Kay

Re: [PATCH 3/5] add sg segment limitation info to device structure

[James Bottomley]

On Tue, 2007-10-02 at 17:02 +0200, Kay Sievers wrote:
> On 10/2/07, James Bottomley <James.Bottomley@steeleye.com> wrote:
> > On Mon, 2007-10-01 at 21:22 -0700, Greg KH wrote:
> > > On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> > > > On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > > > > 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.
> > >
> > > 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?
> 
> 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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Kay Sievers]

On Tue, 2007-10-02 at 10:05 -0500, James Bottomley wrote:
> On Tue, 2007-10-02 at 17:02 +0200, Kay Sievers wrote:
> > On 10/2/07, James Bottomley <James.Bottomley@steeleye.com> wrote:
> > > On Mon, 2007-10-01 at 21:22 -0700, Greg KH wrote:
> > > > On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> > > > > On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > > > > > 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.
> > > >
> > > > 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?
> > 
> > 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?

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

Kay

Re: [PATCH 3/5] add sg segment limitation info to device structure

[James Bottomley]

On Tue, 2007-10-02 at 17:10 +0200, Kay Sievers wrote:
> On Tue, 2007-10-02 at 10:05 -0500, James Bottomley wrote:
> > On Tue, 2007-10-02 at 17:02 +0200, Kay Sievers wrote:
> > > On 10/2/07, James Bottomley <James.Bottomley@steeleye.com> wrote:
> > > > On Mon, 2007-10-01 at 21:22 -0700, Greg KH wrote:
> > > > > On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> > > > > > On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > > > > > > 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.
> > > > >
> > > > > 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?
> > > 
> > > 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?
> 
> 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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Kay Sievers]

On 10/2/07, James Bottomley <James.Bottomley@steeleye.com> wrote:
> On Tue, 2007-10-02 at 17:10 +0200, Kay Sievers wrote:
> > On Tue, 2007-10-02 at 10:05 -0500, James Bottomley wrote:
> > > On Tue, 2007-10-02 at 17:02 +0200, Kay Sievers wrote:
> > > > On 10/2/07, James Bottomley <James.Bottomley@steeleye.com> wrote:
> > > > > On Mon, 2007-10-01 at 21:22 -0700, Greg KH wrote:
> > > > > > On Mon, Oct 01, 2007 at 07:39:02PM -0600, Matthew Wilcox wrote:
> > > > > > > On Mon, Oct 01, 2007 at 07:36:10PM -0400, James Bottomley wrote:
> > > > > > > > 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.
> > > > > >
> > > > > > 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?
> > > >
> > > > 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?
> >
> > 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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Matthew Wilcox]

On Tue, Oct 02, 2007 at 05:23:39PM +0200, Kay Sievers wrote:
> 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.

-- 
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."

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Greg KH]

On Tue, Oct 02, 2007 at 09:25:34AM -0600, Matthew Wilcox wrote:
> On Tue, Oct 02, 2007 at 05:23:39PM +0200, Kay Sievers wrote:
> > 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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[FUJITA Tomonori]

On Tue, 2 Oct 2007 09:44:13 -0700
Greg KH <gregkh@suse.de> wrote:

> On Tue, Oct 02, 2007 at 09:25:34AM -0600, Matthew Wilcox wrote:
> > On Tue, Oct 02, 2007 at 05:23:39PM +0200, Kay Sievers wrote:
> > > 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).

- 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.


diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 37c00f6..c93ebe8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 171ca71..2a18134 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 3a38d1f..3c24932 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 2dc21cb..02854c8 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -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
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 038a0dc..1d630b5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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;}

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Jeff Garzik]

FUJITA Tomonori wrote:
> On Tue, 2 Oct 2007 09:44:13 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
>> On Tue, Oct 02, 2007 at 09:25:34AM -0600, Matthew Wilcox wrote:
>>> On Tue, Oct 02, 2007 at 05:23:39PM +0200, Kay Sievers wrote:
>>>> 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).
> 
> - 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

Re: [PATCH 3/5] add sg segment limitation info to device structure

[Greg KH]

On Wed, Oct 03, 2007 at 11:19:22PM +0900, FUJITA Tomonori wrote:
> On Tue, 2 Oct 2007 09:44:13 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > On Tue, Oct 02, 2007 at 09:25:34AM -0600, Matthew Wilcox wrote:
> > > On Tue, Oct 02, 2007 at 05:23:39PM +0200, Kay Sievers wrote:
> > > > 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).
> 
> - 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.
> 
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 37c00f6..c93ebe8 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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);

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