[PATCH] RFC: kills consistent_dma_mask

[PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

Hi,

What do you think about this patch? It kills all references to
consistent_dma_mask in 2.6.0-test3 tree.

The consistent_dma_mask (and set_... function) is presumably a hack
which is currently not used by anything (at least in the official tree).
It's unneeded (it can be easily done in a driver, should a need arrive,
without polluting the PCI subsystem) and is not supported by "DMA" API.
It isn't even implemented on most platforms - only x86_64 and ia64 have
support for it, while on the remaining archs using it according to the
docs (with non-default value) could mean Oops or something like that.

This patch doesn't actually change any current kernel behaviour.
-- 
Krzysztof Halasa
Network Administrator

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: kill-consistent-mask.patch --]
[-- Type: text/x-patch, Size: 8277 bytes --]

diff -ur linux-2.6.orig/Documentation/DMA-mapping.txt linux/Documentation/DMA-mapping.txt
--- linux-2.6.orig/Documentation/DMA-mapping.txt	2003-08-09 22:12:26.000000000 +0200
+++ linux/Documentation/DMA-mapping.txt	2003-08-17 23:55:13.000000000 +0200
@@ -87,10 +87,7 @@
 PCI-X specification requires PCI-X devices to support 64-bit
 addressing (DAC) for all transactions. And at least one platform (SGI
 SN2) requires 64-bit consistent allocations to operate correctly when
-the IO bus is in PCI-X mode. Therefore, like with pci_set_dma_mask(),
-it's good practice to call pci_set_consistent_dma_mask() to set the
-appropriate mask even if your device only supports 32-bit DMA
-(default) and especially if it's a PCI-X device.
+the IO bus is in PCI-X mode.
 
 For correct operation, you must interrogate the PCI layer in your
 device probe routine to see if the PCI controller on the machine can
@@ -103,11 +100,6 @@
 
 	int pci_set_dma_mask(struct pci_dev *pdev, u64 device_mask);
 
-The query for consistent allocations is performed via a a call to
-pci_set_consistent_dma_mask():
-
-	int pci_set_consistent_dma_mask(struct pci_dev *pdev, u64 device_mask);
-
 Here, pdev is a pointer to the PCI device struct of your device, and
 device_mask is a bit mask describing which bits of a PCI address your
 device supports.  It returns zero if your card can perform DMA
@@ -161,30 +153,6 @@
 		goto ignore_this_device;
 	}
 
-If a card is capable of using 64-bit consistent allocations as well,
-the case would look like this:
-
-	int using_dac, consistent_using_dac;
-
-	if (!pci_set_dma_mask(pdev, 0xffffffffffffffff)) {
-		using_dac = 1;
-	   	consistent_using_dac = 1;
-		pci_set_consistent_dma_mask(pdev, 0xffffffffffffffff)
-	} else if (!pci_set_dma_mask(pdev, 0xffffffff)) {
-		using_dac = 0;
-		consistent_using_dac = 0;
-		pci_set_consistent_dma_mask(pdev, 0xffffffff)
-	} else {
-		printk(KERN_WARNING
-		       "mydev: No suitable DMA available.\n");
-		goto ignore_this_device;
-	}
-
-pci_set_consistent_dma_mask() will always be able to set the same or a
-smaller mask as pci_set_dma_mask(). However for the rare case that a
-device driver only uses consistent allocations, one would have to
-check the return value from pci_set_consistent_dma_mask().
-
 If your 64-bit device is going to be an enormous consumer of DMA
 mappings, this can be problematic since the DMA mappings are a
 finite resource on many platforms.  Please see the "DAC Addressing
@@ -255,8 +223,7 @@
 
   The current default is to return consistent memory in the low 32
   bits of the PCI bus space.  However, for future compatibility you
-  should set the consistent mask even if this default is fine for your
-  driver.
+  should set the mask even if this default is fine for your driver.
 
   Good examples of what to use consistent mappings for are:
 
@@ -326,15 +293,6 @@
 driver needs regions sized smaller than a page, you may prefer using
 the pci_pool interface, described below.
 
-The consistent DMA mapping interfaces, for non-NULL dev, will by
-default return a DMA address which is SAC (Single Address Cycle)
-addressable.  Even if the device indicates (via PCI dma mask) that it
-may address the upper 32-bits and thus perform DAC cycles, consistent
-allocation will only return > 32-bit PCI addresses for DMA if the
-consistent dma mask has been explicitly changed via
-pci_set_consistent_dma_mask().  This is true of the pci_pool interface
-as well.
-
 pci_alloc_consistent returns two values: the virtual address which you
 can use to access it from the CPU and dma_handle which you pass to the
 card.
diff -ur linux-2.6.orig/arch/ia64/sn/io/machvec/pci_dma.c linux/arch/ia64/sn/io/machvec/pci_dma.c
--- linux-2.6.orig/arch/ia64/sn/io/machvec/pci_dma.c	2003-07-11 21:30:44.000000000 +0200
+++ linux/arch/ia64/sn/io/machvec/pci_dma.c	2003-08-18 00:00:32.000000000 +0200
@@ -190,7 +190,7 @@
 	 * automatically allocated a 64Bits DMA Address.  Error out if the 
 	 * device cannot support DAC.
 	 */
-	if (*dma_handle > hwdev->consistent_dma_mask) {
+	if (*dma_handle > hwdev->dma_mask) {
 		free_pages((unsigned long) cpuaddr, get_order(size));
 		return NULL;
 	}
diff -ur linux-2.6.orig/arch/x86_64/kernel/pci-gart.c linux/arch/x86_64/kernel/pci-gart.c
--- linux-2.6.orig/arch/x86_64/kernel/pci-gart.c	2003-08-09 22:12:32.000000000 +0200
+++ linux/arch/x86_64/kernel/pci-gart.c	2003-08-18 00:01:03.000000000 +0200
@@ -142,7 +142,7 @@
 		gfp |= GFP_DMA; 
 		dma_mask = 0xffffffff; 
 	} else {
-		dma_mask = hwdev->consistent_dma_mask; 
+		dma_mask = hwdev->dma_mask; 
 	}
 	if (dma_mask == 0) 
 		dma_mask = 0xffffffff; 
diff -ur linux-2.6.orig/drivers/atm/lanai.c linux/drivers/atm/lanai.c
--- linux-2.6.orig/drivers/atm/lanai.c	2003-08-09 22:12:32.000000000 +0200
+++ linux/drivers/atm/lanai.c	2003-08-17 23:58:19.000000000 +0200
@@ -2039,11 +2039,6 @@
 		    "(itf %d): No suitable DMA available.\n", lanai->number);
 		return -EBUSY;
 	}
-	if (pci_set_consistent_dma_mask(pci, 0xFFFFFFFF) != 0) {
-		printk(KERN_WARNING DEV_LABEL
-		    "(itf %d): No suitable DMA available.\n", lanai->number);
-		return -EBUSY;
-	}
 	/* Get the pci revision byte */
 	result = pci_read_config_byte(pci, PCI_REVISION_ID,
 	    &lanai->pci_revision);
diff -ur linux-2.6.orig/drivers/net/tg3.c linux/drivers/net/tg3.c
--- linux-2.6.orig/drivers/net/tg3.c	2003-08-09 22:12:38.000000000 +0200
+++ linux/drivers/net/tg3.c	2003-08-17 23:57:53.000000000 +0200
@@ -6779,14 +6779,9 @@
 	}
 
 	/* Configure DMA attributes. */
-	if (!pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
+	if (!pci_set_dma_mask(pdev, 0xffffffffffffffffULL))
 		pci_using_dac = 1;
-		if (pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL)) {
-			printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
-			       "for consistent allocations\n");
-			goto err_out_free_res;
-		}
-	} else {
+	else {
 		err = pci_set_dma_mask(pdev, (u64) 0xffffffff);
 		if (err) {
 			printk(KERN_ERR PFX "No usable DMA configuration, "
diff -ur linux-2.6.orig/drivers/pci/pci.c linux/drivers/pci/pci.c
--- linux-2.6.orig/drivers/pci/pci.c	2003-08-09 22:12:40.000000000 +0200
+++ linux/drivers/pci/pci.c	2003-08-17 23:59:12.000000000 +0200
@@ -701,17 +701,6 @@
 	return 0;
 }
 
-int
-pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask)
-{
-	if (!pci_dma_supported(dev, mask))
-		return -EIO;
-
-	dev->consistent_dma_mask = mask;
-
-	return 0;
-}
-     
 static int __devinit pci_init(void)
 {
 	struct pci_dev *dev = NULL;
@@ -763,7 +752,6 @@
 EXPORT_SYMBOL(pci_clear_mwi);
 EXPORT_SYMBOL(pci_set_dma_mask);
 EXPORT_SYMBOL(pci_dac_set_dma_mask);
-EXPORT_SYMBOL(pci_set_consistent_dma_mask);
 EXPORT_SYMBOL(pci_assign_resource);
 EXPORT_SYMBOL(pci_find_parent_resource);
 
diff -ur linux-2.6.orig/drivers/pci/probe.c linux/drivers/pci/probe.c
--- linux-2.6.orig/drivers/pci/probe.c	2003-08-09 22:12:40.000000000 +0200
+++ linux/drivers/pci/probe.c	2003-08-17 23:59:27.000000000 +0200
@@ -519,7 +519,6 @@
 	/* Assume 32-bit PCI; let 64-bit PCI cards (which are far rarer)
 	   set this higher, assuming the system even supports it.  */
 	dev->dma_mask = 0xffffffff;
-	dev->consistent_dma_mask = 0xffffffff;
 	if (pci_setup_device(dev) < 0) {
 		kfree(dev);
 		return NULL;
diff -ur linux-2.6.orig/include/linux/pci.h linux/include/linux/pci.h
--- linux-2.6.orig/include/linux/pci.h	2003-08-09 22:12:47.000000000 +0200
+++ linux/include/linux/pci.h	2003-08-18 00:00:04.000000000 +0200
@@ -390,11 +390,6 @@
 					   or supports 64-bit transfers.  */
 	struct list_head pools;		/* pci_pools tied to this device */
 
-	u64		consistent_dma_mask;/* Like dma_mask, but for
-					       pci_alloc_consistent mappings as
-					       not all hardware supports
-					       64 bit addresses for consistent
-					       allocations such descriptors. */
 	u32             current_state;  /* Current operating state. In ACPI-speak,
 					   this is D0-D3, D0 being fully functional,
 					   and D3 being off. */
@@ -621,7 +616,6 @@
 void pci_clear_mwi(struct pci_dev *dev);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_dac_set_dma_mask(struct pci_dev *dev, u64 mask);
-int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_assign_resource(struct pci_dev *dev, int i);
 
 /* Power management related routines */

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 18 Aug 2003 00:34:17 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> The consistent_dma_mask (and set_... function) is presumably a hack
> which is currently not used by anything (at least in the official tree).
> It's unneeded (it can be easily done in a driver, should a need arrive,
> without polluting the PCI subsystem) and is not supported by "DMA" API.

ia64 does in fact need consistent_dma_mask.

> It isn't even implemented on most platforms - only x86_64 and ia64 have
> support for it, while on the remaining archs using it according to the
> docs (with non-default value) could mean Oops or something like that.

The platforms where it isn't implemented simply support
it identically to how they support the normal dma_mask.

Please read the threads in the archives that caused
consistent_dma_mask to be added to the tree in the first
place before you go around removing it.

Thanks.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> ia64 does in fact need consistent_dma_mask.

For what?
Perhaps a file name?

> > It isn't even implemented on most platforms - only x86_64 and ia64 have
> > support for it, while on the remaining archs using it according to the
> > docs (with non-default value) could mean Oops or something like that.
> 
> The platforms where it isn't implemented simply support
> it identically to how they support the normal dma_mask.

No. This is only true if you set dma_mask = consistent_dma_mask.
If they aren't equal (and don't cover the entire RAM address space)
the thing is broken.
If they have to be equal - why we need 2 masks in the first place?

> Please read the threads in the archives that caused
> consistent_dma_mask to be added to the tree in the first
> place before you go around removing it.

I did that before posting, of course. Which archives do you mean?
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 18 Aug 2003 14:44:19 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "David S. Miller" <davem@redhat.com> writes:
> 
> > ia64 does in fact need consistent_dma_mask.
> 
> For what?

Because on some SGI ia64 platforms the need to allow a different range
for consistent_dma_mask than they do for the normal dma_mask.

The ia64 support code to do things with consistent_dma_mask just isn't
in the tree yet.

Because the other platforms don't to do anything special wrt. this
they can just ignore consitent_dma_mask altogether.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> The ia64 support code to do things with consistent_dma_mask just isn't
> in the tree yet.

Ok. Any pointer so I can see how is it used?

> Because the other platforms don't to do anything special wrt. this
> they can just ignore consitent_dma_mask altogether.

No. The documentation states that consistent_dma_mask (and not dma_mask)
will be used when doing pci_alloc_consistent(). This is, obviously, false
on most platforms.
It is perfectly reasonable to expect that setting consistent_dma_mask
to, say, 28 bits will cause pci_alloc_consistent to return memory from
first 256 MB. This is not true on most platforms, for example i386
happily allocs memory near the top in such case.

If we really need two masks, they can't be ignored on some archs.

Perhaps we should drop the masks at all and always supply a mask to
a alloc/map calls (possibly first checking if the mask is valid)?
I don't know.
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 18 Aug 2003 17:54:42 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "David S. Miller" <davem@redhat.com> writes:
> 
> > Because the other platforms don't to do anything special wrt. this
> > they can just ignore consitent_dma_mask altogether.
> 
> No. The documentation states that consistent_dma_mask (and not dma_mask)
> will be used when doing pci_alloc_consistent().

Then the platforms need to implement the code.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> > No. The documentation states that consistent_dma_mask (and not dma_mask)
> > will be used when doing pci_alloc_consistent().
> 
> Then the platforms need to implement the code.

There is no problem with that, i.e. the changes are trivial (except
for pci_map_*, but that's another story).

I don't know if it wouldn't break something, though. x86-64 and ia64
are much less tested than i386 and the change would alter i386
behaviour to that of x86-64/ia64.

Again: which driver uses the consistent_dma_mask and where I can find it?
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 18 Aug 2003 20:21:48 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Again: which driver uses the consistent_dma_mask and where I can find it?

drivers/net/tg3.c

Others that can handle 64-bit DMA addresses for their
descriptors will do so as well eventually as well.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> drivers/net/tg3.c

No... I know of tg3.c:

        /* Configure DMA attributes. */
        if (!pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
                pci_using_dac = 1;
                if (pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL)) {
                        printk(KERN_ERR PFX "Unable to obtain 64 bit DMA "
                               "for consistent allocations\n");
                        goto err_out_free_res;
                }
        } else {
                err = pci_set_dma_mask(pdev, (u64) 0xffffffff);
                if (err) {
                        printk(KERN_ERR PFX "No usable DMA configuration, "
                               "aborting.\n");
                        goto err_out_free_res;
                }
                pci_using_dac = 0;
        }

As you can see it tg3 uses consistent_dma_mask = dma_mask so this one
doesn't need two masks.


Ok, I assume there is a real need for two masks. Still, having different
archs rely on different variables for the same task is a bug which needs
fixing.

Example:

$ grep DMA_MASK sound/oss/emu10k1/main.c
#define EMU10K1_DMA_MASK                0x1fffffff      /* DMA buffer mask for pci_alloc_consist */
        if (pci_set_dma_mask(pci_dev, EMU10K1_DMA_MASK)) {


Do you see a problem here? It will work if and only if pci_alloc_consistent
uses dma_mask rather than consistent_dma_mask.

Ok, I will make a patch which uses consistent_dma_mask for consistent allocs
on all archs. This will break drivers but they are already broken on
x86-64 and ia64, and it's easier to fix drivers than to write them when
the core is faulty.

Hope that it is ok?
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> Example:

Krzysztof> $ grep DMA_MASK sound/oss/emu10k1/main.c #define
Krzysztof> EMU10K1_DMA_MASK 0x1fffffff /* DMA buffer mask for
Krzysztof> pci_alloc_consist */ if (pci_set_dma_mask(pci_dev,
Krzysztof> EMU10K1_DMA_MASK)) {

Krzysztof> Do you see a problem here? It will work if and only if
Krzysztof> pci_alloc_consistent uses dma_mask rather than
Krzysztof> consistent_dma_mask.

Yes there is a problem, them emu10k driver was expecting the
pci_set_dma_mask call to affect pci_alloc_consistent, which was
unfortunately incorrect usage of the API.

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> "David S. Miller" <davem@redhat.com> writes:
>> The ia64 support code to do things with consistent_dma_mask just
>> isn't in the tree yet.

Krzysztof> Ok. Any pointer so I can see how is it used?

drivers/net/tg3.c was the case where we needed it first. If you grab
the official ia64 kernel patch and look in the arch/ia64/sn/io code
you will find places that consider it.

>> Because the other platforms don't to do anything special wrt. this
>> they can just ignore consitent_dma_mask altogether.

Krzysztof> No. The documentation states that consistent_dma_mask (and
Krzysztof> not dma_mask) will be used when doing
Krzysztof> pci_alloc_consistent(). This is, obviously, false on most
Krzysztof> platforms.

It's not being used on those platforms because I couldn't implement it
on all of them - I just don't have the hardware. We implemented it on
ia64 because thats where we needed it, Andi Kleen then implemented it
on x86_64 because he needed it there. If there are PPC boxes out there
with similar issues then I am sure that the PPC maintainers will
implement support for this when they need it.

Krzysztof> It is perfectly reasonable to expect that
Krzysztof> setting consistent_dma_mask to, say, 28 bits will cause
Krzysztof> pci_alloc_consistent to return memory from first 256
Krzysztof> MB. This is not true on most platforms, for example i386
Krzysztof> happily allocs memory near the top in such case.

The default for pci_alloc_consistent() on ia32 is that
pci_dev->consistent_dma_mask == 32 bit. If you need something else for
a reason, please feel free to implement support for it.

Krzysztof> If we really need two masks, they can't be ignored on some
Krzysztof> archs.

So *fix* it! This is Linux, it's Free Software, you have the source,
you have the right to fix bugs!

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> "David S. Miller" <davem@redhat.com> writes:
>> ia64 does in fact need consistent_dma_mask.

Krzysztof> For what?  Perhaps a file name?

Because some ia64 boxen do not have physical memory in the lower 4GB
region and the PCI-X spec requires cards to support dual-address
cycles (aka 64 bit addressing) so some boxes do not have an MMU
operating when slots are in PCI-X mode. One can argue whether this is
a good idea or not, however it *is* spec compliant.

Krzysztof> No. This is only true if you set dma_mask =
Krzysztof> consistent_dma_mask.  If they aren't equal (and don't cover
Krzysztof> the entire RAM address space) the thing is broken.  If they
Krzysztof> have to be equal - why we need 2 masks in the first place?

Historically pci_alloc_consistent would always rely on the consistent
dma mask being <=32 bit. That is necessary because some adapters may
provide > 32bit addressing in their dynamic descriptors but only 32
bit in their consistent descriptors. This you are likely to find in
cases where the hardware vendor has added 'extended descriptors' to
their chips by sticking extra address bits into random places in their
control structures where there were a few bits free.

So yes, we *do* need both, having different masks for the two is in no
way broken.

We introduced pci_consistent_dma_mask for a reason, remember there are
computers out there that aren't PCs.

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Christoph Hellwig]

On Mon, Aug 18, 2003 at 12:34:17AM +0200, Krzysztof Halasa wrote:
> Hi,
> 
> What do you think about this patch? It kills all references to
> consistent_dma_mask in 2.6.0-test3 tree.
> 
> The consistent_dma_mask (and set_... function) is presumably a hack
> which is currently not used by anything (at least in the official tree).
> It's unneeded (it can be easily done in a driver, should a need arrive,
> without polluting the PCI subsystem) and is not supported by "DMA" API.
> It isn't even implemented on most platforms - only x86_64 and ia64 have
> support for it, while on the remaining archs using it according to the
> docs (with non-default value) could mean Oops or something like that.

So better fix the support.  This code was recently included after DaveM
as pci dma maintainer ACKed it.

Re: [PATCH] RFC: kills consistent_dma_mask

[Pete Zaitcev]

> From: Krzysztof Halasa <khc@pm.waw.pl>
> Date: 18 Aug 2003 00:34:17 +0200

> It's unneeded (it can be easily done in a driver, should a need arrive,
> without polluting the PCI subsystem) and is not supported by "DMA" API.

Are you talking about doing tripple calls, e.g.

       pci_set_dma_mask(pdev, 0xFFFFFFFF);
       foo = pci_alloc_consistent(pdev, size, &handle);
       // Restore for upcoming streaming allocations
       pci_set_dma_mask(pdev, 0xFFFFFFFFFFFFFFFF);

Possibly Jes considered that alternative and decided that it
did not allow for sufficient performance.

> It isn't even implemented on most platforms - only x86_64 and ia64 have
> support for it, while on the remaining archs using it according to the
> docs (with non-default value) could mean Oops or something like that.

Before you go for that, I'd rather see you implementing the
double/tripple calls in drivers, check for effects, THEN
go for removal of the mask. If you cannot do it, plea SGI people
to test it on SN-2 for you (or same for Intel Tiger box).

> This patch doesn't actually change any current kernel behaviour.

Sure it does. It blows all non-mmu ia64 out of the water.

The consistent mask looks a little distasteful to me, and I think
it should not buy us performance because consistent allocations
are not supposed to be fast. They are bad, but what you are doing
is worse: you are trying to ruin the day of legitimate users.
Please, be reasonable. Get SGI buy-in and come back.

-- Pete

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

Pete Zaitcev <zaitcev@redhat.com> writes:

> Are you talking about doing tripple calls, e.g.
> 
>        pci_set_dma_mask(pdev, 0xFFFFFFFF);
>        foo = pci_alloc_consistent(pdev, size, &handle);
>        // Restore for upcoming streaming allocations
>        pci_set_dma_mask(pdev, 0xFFFFFFFFFFFFFFFF);
> 
> Possibly Jes considered that alternative and decided that it
> did not allow for sufficient performance.

Possibly. Is that true?

I could imagine even something like that:

init_module()
{
        using_dac = 1;
        if (!pci_dma_supported(dev, 0xFFFFFFFFFFFFFFFF)) {
                if (!pci_dma_supported(dev, 0xFFFFFFFF))
                      	error;
                using_dac = 0;
        }
}

...

        foo = pci_alloc_consistent(pdev, size, &handle, 0xFFFFFFFF);
        bar = pci_map_single(...,
                using_dac ? 0xFFFFFFFFFFFFFFFF : 0xFFFFFFFF);

I don't think it would be slower. If inlined, if would be even faster.

However, the main problem is not that it isn't beautiful but rather that
it's broken.

> Before you go for that, I'd rather see you implementing the
> double/tripple calls in drivers, check for effects, THEN
> go for removal of the mask.

The problem is that the official kernel does NOT contain any driver which
needs different masks.

> > This patch doesn't actually change any current kernel behaviour.
> 
> Sure it does. It blows all non-mmu ia64 out of the water.

No. The kernel (2.6.0-test3 at least) doesn't count on that under any
circumstances.

> The consistent mask looks a little distasteful to me, and I think
> it should not buy us performance because consistent allocations
> are not supposed to be fast. They are bad, but what you are doing
> is worse: you are trying to ruin the day of legitimate users.

Of course this isn't what I'm trying to do.
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> Pete Zaitcev <zaitcev@redhat.com> writes:
>> Are you talking about doing tripple calls, e.g.
>> 
>> pci_set_dma_mask(pdev, 0xFFFFFFFF); foo =
>> pci_alloc_consistent(pdev, size, &handle); // Restore for upcoming
>> streaming allocations pci_set_dma_mask(pdev, 0xFFFFFFFFFFFFFFFF);
>> 
>> Possibly Jes considered that alternative and decided that it did
>> not allow for sufficient performance.

Krzysztof> Possibly. Is that true?

It's not primarily a performance issue it's more of a correctness
issue. You *need* to be able to distinguish the masks between
consistent and dynamic allocations.

Krzysztof> I could imagine even something like that:

Krzysztof> init_module() { using_dac = 1; if (!pci_dma_supported(dev,
Krzysztof> 0xFFFFFFFFFFFFFFFF)) { if (!pci_dma_supported(dev,
Krzysztof> 0xFFFFFFFF)) error; using_dac = 0; } }

Krzysztof>         foo = pci_alloc_consistent(pdev, size, &handle,
Krzysztof> 0xFFFFFFFF); bar = pci_map_single(..., using_dac ?
Krzysztof> 0xFFFFFFFFFFFFFFFF : 0xFFFFFFFF);

Krzysztof> I don't think it would be slower. If inlined, if would be
Krzysztof> even faster.

Sure you can add the mask to the allocation calls, but thats going to
cost you since you are not going to be able to make it inline. The
consistent allocations have to be programmed into the IOMMU at mapping
time. Having the consistent DMA mask as we do right now is a lot
cleaner and it means the driver doesn't have to do a ton of runtime
accounting.

>> Before you go for that, I'd rather see you implementing the
>> double/tripple calls in drivers, check for effects, THEN go for
>> removal of the mask.

Krzysztof> The problem is that the official kernel does NOT contain
Krzysztof> any driver which needs different masks.

Bzzzt, *wrong*! Take a look at drivers/scsi/aic7xxx/aic7xxx_osm_pci.c,
if you look at the code you will notice that the hardware does support
different masks for consistent vs dynamic allocations (32 bit for
consistent vs 39 or 64 bit for dynamic). However make a note that the
driver uses the current interface incorrectly and thinks that
pci_set_dma_mask() actually applies to pci_alloc_consistent, which is
something it never did.

>> > This patch doesn't actually change any current kernel behaviour.
>> 
>> Sure it does. It blows all non-mmu ia64 out of the water.

Krzysztof> No. The kernel (2.6.0-test3 at least) doesn't count on that
Krzysztof> under any circumstances.

But it will, as people have been telling you repeatedly, there *is* a
need for this stuff, it's just that not all the patches have been
fully integrated yet. I pushed for this change in 2.5.x so we didn't
have to make infrastructure changes to support this in the middle of
2.6.x.

>> The consistent mask looks a little distasteful to me, and I think
>> it should not buy us performance because consistent allocations are
>> not supposed to be fast. They are bad, but what you are doing is
>> worse: you are trying to ruin the day of legitimate users.

Krzysztof> Of course this isn't what I'm trying to do.

Well you are trying to remove something people need and which was put
in there after considerable discussion between the involved
parties. The reasonsing was even documented in
Documentation/DMA-mapping.txt.

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

Jes Sorensen <jes@wildopensource.com> writes:

> Bzzzt, *wrong*! Take a look at drivers/scsi/aic7xxx/aic7xxx_osm_pci.c,
> if you look at the code you will notice that the hardware does support
> different masks for consistent vs dynamic allocations (32 bit for
> consistent vs 39 or 64 bit for dynamic).

The hardware, maybe.

> However make a note that the
> driver uses the current interface incorrectly and thinks that
> pci_set_dma_mask() actually applies to pci_alloc_consistent, which is
> something it never did.

No, it nearly always does. Looks at the actual pci_alloc_consistent on,
say, i386.

Will it be ok if I fix the consistent allocs to use consistent_dma_mask
(some drivers will need a fix on i386 etc.)?
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> Jes Sorensen <jes@wildopensource.com> writes:
>> Bzzzt, *wrong*! Take a look at
>> drivers/scsi/aic7xxx/aic7xxx_osm_pci.c, if you look at the code you
>> will notice that the hardware does support different masks for
>> consistent vs dynamic allocations (32 bit for consistent vs 39 or
>> 64 bit for dynamic).

Krzysztof> The hardware, maybe.

The hardware, yes.

Krzysztof> Will it be ok if I fix the consistent allocs to use
Krzysztof> consistent_dma_mask (some drivers will need a fix on i386
Krzysztof> etc.)?

That would be ideal I'd say.

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Pete" == Pete Zaitcev <zaitcev@redhat.com> writes:

>> It isn't even implemented on most platforms - only x86_64 and ia64
>> have support for it, while on the remaining archs using it
>> according to the docs (with non-default value) could mean Oops or
>> something like that.

Pete> Before you go for that, I'd rather see you implementing the
Pete> double/tripple calls in drivers, check for effects, THEN go for
Pete> removal of the mask. If you cannot do it, plea SGI people to
Pete> test it on SN-2 for you (or same for Intel Tiger box).

Hi Pete,

That would be totally messy. Having drivers do the accounting of what
mask is currently set and have them switch back and forth depending on
what type of allocation is currently being used would be a nightmare
to debug.

Pete> The consistent mask looks a little distasteful to me, and I
Pete> think it should not buy us performance because consistent
Pete> allocations are not supposed to be fast. They are bad, but what
Pete> you are doing is worse: you are trying to ruin the day of
Pete> legitimate users.  Please, be reasonable. Get SGI buy-in and
Pete> come back.

The thing is that we really want to do 32-bit allocations for
consistent allocations if we can in anyway do it since it means the
card can do SAC for all access to control structures. However as you
mention there are cases where this isn't possible and we will have to
take the hit of DAC cycles.

Cheers,
Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Alan Cox]

On Maw, 2003-08-19 at 10:03, Jes Sorensen wrote:
> That would be totally messy. Having drivers do the accounting of what
> mask is currently set and have them switch back and forth depending on
> what type of allocation is currently being used would be a nightmare
> to debug.

It is messy, but the consistent mask only helps a small subset of cases.
Having an __pci_alloc_foo that took the mask as an argument is (a)
trivial (b) adds almost no code  (c) solves the general case problem.

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

Alan> On Maw, 2003-08-19 at 10:03, Jes Sorensen wrote:
>> That would be totally messy. Having drivers do the accounting of
>> what mask is currently set and have them switch back and forth
>> depending on what type of allocation is currently being used would
>> be a nightmare to debug.

Alan> It is messy, but the consistent mask only helps a small subset
Alan> of cases.  Having an __pci_alloc_foo that took the mask as an
Alan> argument is (a) trivial (b) adds almost no code (c) solves the
Alan> general case problem.

And d) puts the accounting back into the drivers in duplicate. So far
we have managed pretty well with the distinction between consistent
and dynamic, but sure if there is hardware out there where it makes
sense to have a more generic interface we should consider it.

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 19 Aug 2003 14:07:18 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> It is messy, but the consistent mask only helps a small subset of cases.
> Having an __pci_alloc_foo that took the mask as an argument is (a)
> trivial (b) adds almost no code  (c) solves the general case problem.

(d) Makes implementations have to verify the mask is usable
on every mapping attempt.

Re: [PATCH] RFC: kills consistent_dma_mask

[Alan Cox]

On Maw, 2003-08-19 at 17:55, David S. Miller wrote:
> (d) Makes implementations have to verify the mask is usable
> on every mapping attempt.

Or once per type with a bit of thought about it. I deal with
hardware that has 2 limits on its consistent allocs and a
different one with its streaming I/O buffers. It doesn't seem
too atypical either

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 19 Aug 2003 19:33:19 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Maw, 2003-08-19 at 17:55, David S. Miller wrote:
> > (d) Makes implementations have to verify the mask is usable
> > on every mapping attempt.
> 
> Or once per type with a bit of thought about it. I deal with
> hardware that has 2 limits on its consistent allocs and a
> different one with its streaming I/O buffers. It doesn't seem
> too atypical either

Are you talking on the platform or the PCI device side?

If on the platform side, the device wants to use the most
capable range/mask/whatever available that also fits it's
limits.

If on the PCI device side, it's also a best fit problem.

Give a specific example so I can map this out in my head.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> (d) Makes implementations have to verify the mask is usable
> on every mapping attempt.

No, unless we have hardware which can return success on first mask check
then result error on subsequent mapping request.

We need to decide, though, as I'm going to fix it that way or another:

1) provide the mask argument to actual mapping requests (pci_map_*,
   pci_alloc_*, DMA API) and drop pci_dev->*dma_mask, or

2) add coherent_dma_mask pointer to struct driver as with normal mask,
   pointing to pci_dev->consistent_dma_mask

1: non-trivial, but IMHO makes things more clean and natural (from both
   system's and driver's view), and fits all special cases.


BTW: Why do we have this pointer (I mean u64 *dma_mask) in struct device?
Does it always point to pci_dev->dma_mask (and to similar value on EISA
etc)? I see some code checks for struct device->dma_mask=NULL, is it only
a safety check or does NULL have some meaning there?

Would it make sense to drop the masks in pci_dev and use (u64 not
pointers) *dma_masks in struct device? If so, would 0 there have the same
meaning as now NULL?
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

I think we should do it the following way:
- adding pci_alloc_consistent_mask(..., u64 mask), pci_map_*_mask(..., mask)
  and DMA API friends
- adding a routine checking if a mask is valid on given system
- renaming existing routines to *_nomask and aliasing old names to them.

then:

- migrating drivers from old ones to _mask (the non-trivial part)

then:

- dropping support for _nomasks and then probably renaming _masks to
  old names.


alternative, probably a cleaner one - using "int bits" instead of "u64 mask".
Devices tend to be X-bit (32-bit, 64-bit, 28-bit etc) rather than to have
0xFFFFFFFFFFFFFFFFFFFFF masks anyway. And the dma_mask has to be
continuous, right? The "bits" value is much more readable, too. Of course,
moving from bits to mask and vice versa is easy, it could even be a macro.

Unless there are objections I'm going to start with *_bits.
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> I think we should do it the following way: - adding
Krzysztof> pci_alloc_consistent_mask(..., u64 mask),
Krzysztof> pci_map_*_mask(..., mask) and DMA API friends - adding a
Krzysztof> routine checking if a mask is valid on given system -
Krzysztof> renaming existing routines to *_nomask and aliasing old
Krzysztof> names to them.

I don't like this approach as I mentioned before. IMHO it is adding
unnecessary overhead to the runtime point. Why should one pass in the
mask 5 times when it is enough to use pci_set_consistent_dma_mask
etc. For the consistent allocations it's just a nuisance however if
you add this to pci_map_*() then it's going to add real overhead to
the hot paths of drivers which is just plain wrong.

I still haven't seen a strong argument for the current API being
insufficient. Alan mentioned one device causing the problem with
multiple consistent masks, but are there many more device like that
out there?

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

Jes Sorensen <jes@trained-monkey.org> writes:

> I don't like this approach as I mentioned before. IMHO it is adding
> unnecessary overhead to the runtime point. Why should one pass in the
> mask 5 times when it is enough to use pci_set_consistent_dma_mask
> etc.

I don't see this overhead. Most (all?) drivers use a fixed mask such
as 0x00ffffff or keep track of the mask in their internal structures
(using_dac etc). The code has to get the mask anyway, either from
pci_dev->(consistent_)dma_mask or from its arguments. Currently the
information is duplicated in both PCI and the driver. I think it may
be even faster to examine a function argument on the stack than to get
the mask from pci_dev (is it?)

If the mapping function is inlined (as with i386 case) the mask can be
optimized to NOP (however, i386 does not currently use dma_mask in
pci_map_*() at all, and it's unclear if it could do that inline).

> I still haven't seen a strong argument for the current API being
> insufficient. Alan mentioned one device causing the problem with
> multiple consistent masks, but are there many more device like that
> out there?

There might be in the future.

In general drivers may need many classes of DMA-able memory. We could,
of course, do that, but I think it's simpler to let the driver
specify mask in every call.


There is one big problem with current API: the DMA (struct driver) API
does not have consistent_dma_mask. If the PCI API is implemented on top
of DMA API, it can't be correct (and, obviously, DMA API on top of PCI
API can't be correct either). So, if we insist on keeping
consistent_dma_mask in pci_dev structure, we need to add it to DMA API
as well. There is no trivial change which can fix this problem.

DMA API is the basis for other things so adding consistent_dma_mask to
it brings other but similar problems here and there.


IMHO the actual implementation of DMA and PCI APIs is quite a mess.
I don't know if there is at least one platform which does it according
to the docs. This means many devices will not work on some platforms
without any good reason.
Moving the masks out of device + pci_dev etc. structs and thus
simplifying the API would help cleaning the code. While it's not
a trivial task, it seems to be easier to fix (and then maintain) than
adding consistent|coherent dma_mask to DMA API etc.

I'm not DMA/PCI API expert (though I know it currently much much better
than 2 weeks ago). I'd appreciate any corrections.
In fact in the beginning I thought it will be much easier.
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 24 Aug 2003 14:06:43 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> The code has to get the mask anyway, either from
> pci_dev->(consistent_)dma_mask or from its arguments.

But it does not have to verify the mask each and every mapping call
currently.  We'll need to do that with your suggested changes.

Nobody is going to agree to any of your proposals at the rate you're
currently going.

> I don't know if there is at least one platform which does it according
> to the docs.

Effectively, the correct effects are obtained on i386, Alpha,
IA64, and sparc for all drivers in the tree.  I can say this because
nobody tries to do anything interesting with consistent_dma_mask
yet, and that is why nobody has any incentive to "fix" it as you
keep complaining we need to do.

See, to show something is broken, you have to show a device that
will break currently.  The consistent_dma_mask is only modified
by tg3, and it does so in such a way that all platforms work properly.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> > The code has to get the mask anyway, either from
> > pci_dev->(consistent_)dma_mask or from its arguments.
> 
> But it does not have to verify the mask each and every mapping call
> currently.  We'll need to do that with your suggested changes.

No, why? What we'll need is to verify the mask at driver startup.
It would be driver responsibility to use only valid (verified) masks.

> Nobody is going to agree to any of your proposals at the rate you're
> currently going.

What do you propose instead?

> Effectively, the correct effects are obtained on i386, Alpha,
> IA64, and sparc for all drivers in the tree.  I can say this because
> nobody tries to do anything interesting with consistent_dma_mask
> yet, and that is why nobody has any incentive to "fix" it as you
> keep complaining we need to do.

False. I have a device which needs different mask for consistent allocs.
In fact the whole story began with me trying to put this driver into
the tree.

> See, to show something is broken, you have to show a device that
> will break currently.

SBE wanXL sync serial adapter. 32 bits for buffers but 28 bits for
consistent data.

>  The consistent_dma_mask is only modified
> by tg3, and it does so in such a way that all platforms work properly.

I can't imagine all devices work properly on all platforms wrt
consistent allocs. Say, sound drivers setting only dma_mask to < 32 bits
and expecting consistent alloc will use that and not consistent_dma_mask.

Of course, there is a question if we want to support such sound cards
on Itaniums and Opterons? Of course they work on i386 as
i386 pci_alloc_consistent() ignores consistent_dma_mask.
-- 
Krzysztof Halasa
Network Administrator

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> "David S. Miller" <davem@redhat.com> writes:
>> See, to show something is broken, you have to show a device that
>> will break currently.

Krzysztof> SBE wanXL sync serial adapter. 32 bits for buffers but 28
Krzysztof> bits for consistent data.

Well if the buffers are dynamic, why would they want to be allocated
using the consistent interface?

Krzysztof> I can't imagine all devices work properly on all platforms
Krzysztof> wrt consistent allocs. Say, sound drivers setting only
Krzysztof> dma_mask to < 32 bits and expecting consistent alloc will
Krzysztof> use that and not consistent_dma_mask.

If sound drivers set the dma_mask to something and expect that to
apply to the consistent allocations, then they aren't complying with
the current API and needs to be fixed.

Krzysztof> Of course, there is a question if we want to support such
Krzysztof> sound cards on Itaniums and Opterons? Of course they work
Krzysztof> on i386 as i386 pci_alloc_consistent() ignores
Krzysztof> consistent_dma_mask.

So fix it on ia32 to respect that mask for consistent allocations,
problem solved.

Jes

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

David, any comments?

"David S. Miller" <davem@redhat.com> writes:

> > The code has to get the mask anyway, either from
> > pci_dev->(consistent_)dma_mask or from its arguments.
> 
> But it does not have to verify the mask each and every mapping call
> currently.  We'll need to do that with your suggested changes.

No, why? What we'll need is to verify the mask at driver startup.
It would be driver responsibility to use only valid (verified) masks.

> Nobody is going to agree to any of your proposals at the rate you're
> currently going.

What do you propose instead?

> Effectively, the correct effects are obtained on i386, Alpha,
> IA64, and sparc for all drivers in the tree.  I can say this because
> nobody tries to do anything interesting with consistent_dma_mask
> yet, and that is why nobody has any incentive to "fix" it as you
> keep complaining we need to do.

False. I have a device which needs different mask for consistent allocs.
In fact the whole story began with me trying to put this driver into
the tree.

> See, to show something is broken, you have to show a device that
> will break currently.

SBE wanXL sync serial adapter. 32 bits for buffers but 28 bits for
consistent data.

>  The consistent_dma_mask is only modified
> by tg3, and it does so in such a way that all platforms work properly.

I can't imagine all devices work properly on all platforms wrt
consistent allocs. Say, sound drivers setting only dma_mask to < 32 bits
and expecting consistent alloc will use that and not consistent_dma_mask.

Of course, there is a question if we want to support such sound cards
on Itaniums and Opterons? Of course they work on i386 as
i386 pci_alloc_consistent() ignores consistent_dma_mask.

-- Krzysztof Halasa, B*FH

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 30 Aug 2003 23:18:50 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> David, any comments?

I'm too busy to partake in this thread right now, sorry.

Re: [PATCH] RFC: kills consistent_dma_mask

[Alan Cox]

On Sul, 2003-08-31 at 02:50, David S. Miller wrote:
> On 30 Aug 2003 23:18:50 +0200
> Krzysztof Halasa <khc@pm.waw.pl> wrote:
> 
> > David, any comments?
> 
> I'm too busy to partake in this thread right now, sorry.

Then I suggest we remove the feature until 2.7 since nobody has time
to make it work in 2.6

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> Then I suggest we remove the feature until 2.7 since nobody has time
> to make it work in 2.6

I could possibly fix it with some help from platforms maintainters
(testing) but I'm not going to waste time knowing for sure that it
will be rejected, and probably doing it the wrong way.
-- 
Krzysztof Halasa, B*FH

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On Sun, 31 Aug 2003 13:52:55 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Sul, 2003-08-31 at 02:50, David S. Miller wrote:
> > On 30 Aug 2003 23:18:50 +0200
> > Krzysztof Halasa <khc@pm.waw.pl> wrote:
> > 
> > > David, any comments?
> > 
> > I'm too busy to partake in this thread right now, sorry.
> 
> Then I suggest we remove the feature until 2.7 since nobody has time
> to make it work in 2.6

The problem is that it works for the people it was created
for, you're going to break things for them.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> The problem is that it works for the people it was created
> for, you're going to break things for them.

Then Documentation/DMA* should be corrected to indicate this is created
for PCI-64 cards only, and only to increase default 32-bit addressing
to 64-bit on x86-64 and ia64.
Such correction would be a help to driver developers.
-- 
Krzysztof Halasa, B*FH

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 01 Sep 2003 09:34:50 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "David S. Miller" <davem@redhat.com> writes:
> 
> > The problem is that it works for the people it was created
> > for, you're going to break things for them.
> 
> Then Documentation/DMA* should be corrected to indicate this is created
> for PCI-64 cards only, and only to increase default 32-bit addressing
> to 64-bit on x86-64 and ia64.
> Such correction would be a help to driver developers.

Sure, I'm fine with this as a termporary thing until we
work out the details properly.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> Sure, I'm fine with this as a termporary thing until we
> work out the details properly.

How about the following patch? It corrects most, if not all, obvious
things.

--- linux-2.6/Documentation/DMA-mapping.txt	2003-08-09 06:36:56.000000000 +0200
+++ linux-2.6/Documentation/DMA-mapping.txt.orig	2003-09-01 19:06:16.000000000 +0200
@@ -76,7 +76,8 @@
 Does your device have any DMA addressing limitations?  For example, is
 your device only capable of driving the low order 24-bits of address
 on the PCI bus for SAC DMA transfers?  If so, you need to inform the
-PCI layer of this fact.
+PCI layer of this fact.  Even if you do, this information will be
+ignored on some architectures, such as i386.
 
 By default, the kernel assumes that your device can address the full
 32-bits in a SAC cycle.  For a 64-bit DAC capable device, this needs
@@ -123,6 +124,11 @@
 2) Use some non-DMA mode for data transfer, if possible.
 3) Ignore this device and do not initialize it.
 
+Most platforms will ignore pci_set_consistent_dma_mask() at all and, for
+example, use the device mask set with pci_set_dma_mask() for consistent
+allocations. Remember that even setting both masks to the same value
+doesn't necessarily mean this value will actually be honoured.
+
 It is recommended that your driver print a kernel KERN_WARNING message
 when you end up performing either #2 or #3.  In this manner, if a user
 of your driver reports that performance is bad or that the device is not
@@ -180,10 +186,8 @@
 		goto ignore_this_device;
 	}
 
-pci_set_consistent_dma_mask() will always be able to set the same or a
-smaller mask as pci_set_dma_mask(). However for the rare case that a
-device driver only uses consistent allocations, one would have to
-check the return value from pci_set_consistent_dma_mask().
+For the rare case that a device driver only uses consistent allocations,
+one would have to check the return value from pci_set_consistent_dma_mask().
 
 If your 64-bit device is going to be an enormous consumer of DMA
 mappings, this can be problematic since the DMA mappings are a
@@ -201,17 +205,17 @@
 	}
 
 When pci_set_dma_mask() is successful, and returns zero, the PCI layer
-saves away this mask you have provided.  The PCI layer will use this
-information later when you make DMA mappings.
+saves away this mask you have provided.  The PCI layer may or may not
+use this information later when you make DMA mappings.
 
 There is a case which we are aware of at this time, which is worth
 mentioning in this documentation.  If your device supports multiple
 functions (for example a sound card provides playback and record
 functions) and the various different functions have _different_
 DMA addressing limitations, you may wish to probe each mask and
-only provide the functionality which the machine can handle.  It
-is important that the last call to pci_set_dma_mask() be for the 
-most specific mask.
+only provide the functionality which the machine can handle. Remember
+that the last call to pci_set_dma_mask() will set the mask which will
+(or will not) be used for subsequent DMA mapping requests.
 
 Here is pseudo-code showing how this might be done:
 
@@ -239,7 +243,10 @@
 
 A sound card was used as an example here because this genre of PCI
 devices seems to be littered with ISA chips given a PCI front end,
-and thus retaining the 16MB DMA addressing limitations of ISA.
+and thus retaining the 16MB DMA addressing limitations of ISA. While
+this example will not probably work for pci_map_* functions, the mask
+will be honoured for pci_alloc_consistent() on all platforms except
+ia64 and x86-64.
 
 			Types of DMA mappings
 
@@ -330,10 +337,10 @@
 default return a DMA address which is SAC (Single Address Cycle)
 addressable.  Even if the device indicates (via PCI dma mask) that it
 may address the upper 32-bits and thus perform DAC cycles, consistent
-allocation will only return > 32-bit PCI addresses for DMA if the
-consistent dma mask has been explicitly changed via
+allocation on ia64 and x86-64 will only return > 32-bit PCI addresses
+for DMA if the consistent dma mask has been explicitly changed via
 pci_set_consistent_dma_mask().  This is true of the pci_pool interface
-as well.
+as well. On other platforms, use pci_set_dma_mask().
 
 pci_alloc_consistent returns two values: the virtual address which you
 can use to access it from the CPU and dma_handle which you pass to the

-- 
Krzysztof Halasa, B*FH

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 01 Sep 2003 19:14:46 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

>  When pci_set_dma_mask() is successful, and returns zero, the PCI layer
> -saves away this mask you have provided.  The PCI layer will use this
> -information later when you make DMA mappings.
> +saves away this mask you have provided.  The PCI layer may or may not
> +use this information later when you make DMA mappings.

Umm, come on, this is inaccurate.

If it is accurate fix the broken platforms.  But this is unrelated to
the consistent DMA issues.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> Umm, come on, this is inaccurate.

I tried to make it accurate. I might be missing something, though.
What exactly do you think is inaccurate?

>  But this is unrelated to
> the consistent DMA issues.

Hmm... What do you mean?

BTW: consistent_dma_mask and dma_mask names are misleading: they are
(in theory) related to allocation vs mapping requests mainly -
the consistent vs non-consistent thing is secondary.
-- 
Krzysztof Halasa, B*FH

Re: [PATCH] RFC: kills consistent_dma_mask

[Alan Cox]

On Llu, 2003-09-01 at 06:22, David S. Miller wrote:
> > > I'm too busy to partake in this thread right now, sorry.
> > 
> > Then I suggest we remove the feature until 2.7 since nobody has time
> > to make it work in 2.6
> 
> The problem is that it works for the people it was created
> for, you're going to break things for them.

Unfortunately those people don't have time to finish the feature for 2.6
so its just going to cause bugs and accidents.

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On Mon, 01 Sep 2003 08:54:18 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> Unfortunately those people don't have time to finish the feature for 2.6
> so its just going to cause bugs and accidents.

It does work for the case it was created for, it you want
to put a note into the documentation mentioning the current
limitations then fine.

But knowing breaking the tree for people is bad practice.

Re: [PATCH] RFC: kills consistent_dma_mask

[Krzysztof Halasa]

"David S. Miller" <davem@redhat.com> writes:

> But knowing breaking the tree for people is bad practice.

How about breaking the existing sound drivers (not checked other things)
on ia64 and x86-64?
-- 
Krzysztof Halasa, B*FH

Re: [PATCH] RFC: kills consistent_dma_mask

[David S. Miller]

On 01 Sep 2003 18:27:45 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> How about breaking the existing sound drivers (not checked other things)
> on ia64 and x86-64?

So ask the port maintainers in question to add the necessary
consistent DMA code to their PCI platform layer.

I think you'll make a lot more progress than you have in this
thread so far, if you do as I suggest.

Re: [PATCH] RFC: kills consistent_dma_mask

[Jes Sorensen]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> There is one big problem with current API: the DMA (struct
Krzysztof> driver) API does not have consistent_dma_mask. If the PCI
Krzysztof> API is implemented on top of DMA API, it can't be correct
Krzysztof> (and, obviously, DMA API on top of PCI API can't be correct
Krzysztof> either). So, if we insist on keeping consistent_dma_mask in
Krzysztof> pci_dev structure, we need to add it to DMA API as
Krzysztof> well. There is no trivial change which can fix this
Krzysztof> problem.

So why are we dancing around the thing like this, the problem is sooo
bloody simple. Add consistent_dma_mask to the DMA API as well. I
already spoke to James briefly about this earlier and he didn't sound
like had anything against us adding this feature there.

Jes