From: Stefan Richter <stefanr@s5r6.in-berlin.de> Date: 2007-08-04 17:31:22
(Adding Cc: linuxppc-dev, olh)
Robert Hancock wrote:
Stefan Richter wrote:
quoted
Date: Wed, 1 Aug 2007 20:30:36 +0200 (CEST)
From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: ieee1394: revert "sbp2: enforce 32bit DMA mapping"
Revert commit 0555659d63c285ceb7ead3115532e1b71b0f27a7 from 2.6.22-rc1.
The dma_set_mask call somehow failed on a PowerMac G5, PPC64:
http://lkml.org/lkml/2007/8/1/344
Should there ever occur a DMA mapping beyond the physical DMA range, a
proper SBP-2 firmware will report transport errors. So let's leave it
at that.
Isn't this a rather poor workaround? All this means is that if we fail
to set a 32-bit DMA mask, we're likely to blow up at runtime instead of
at initialization time, when we get a DMA mapping over 4GB.
I generally agree with you. But since I actually never heard of
problems that could directly be related to sbp2's DMA areas exceeding
the OHCI-1394 physical DMA range (4GB in most OHCI-1394
implementations), I consider this simple reversion good enough for post
2.6.23-rc1 and especially for 2.6.22.y.
My original commit 0555659.. was a violation of "If it ain't (known)
broken, don't fix it".
If setting 32-bit DMA mask fails on ppc64, that sounds like a problem
with the DMA implementation on that architecture. There are enough cards
out there that only support 32-bit DMA that this really needs to work..
Yes, could the PPC folks please have a look at it? Thanks.
quoted
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Tested-by: Olaf Hering <redacted>
---
Same as commit a9c2f18800753c82c45fc13b27bdc148849bdbb2.
drivers/ieee1394/sbp2.c | 5 -----
1 file changed, 5 deletions(-)
Index: linux-2.6.22/drivers/ieee1394/sbp2.c
===================================================================
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-05 00:15:57
quoted
If setting 32-bit DMA mask fails on ppc64, that sounds like a problem
with the DMA implementation on that architecture. There are enough cards
out there that only support 32-bit DMA that this really needs to work..
Yes, could the PPC folks please have a look at it? Thanks.
Smells like we may have a bug there. No worries though, all current PPC
machines have an iommu that will not give out addresses above 32 bits
anyway, but I'll double check what's up.
Do you see something in dmesg when that happens ?
Ben.
From: Stefan Richter <stefanr@s5r6.in-berlin.de> Date: 2007-08-05 07:55:25
Benjamin Herrenschmidt wrote:
quoted
quoted
If setting 32-bit DMA mask fails on ppc64, that sounds like a problem
with the DMA implementation on that architecture. There are enough cards
out there that only support 32-bit DMA that this really needs to work..
Yes, could the PPC folks please have a look at it? Thanks.
Smells like we may have a bug there. No worries though, all current PPC
machines have an iommu that will not give out addresses above 32 bits
anyway, but I'll double check what's up.
Do you see something in dmesg when that happens ?
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-05 22:04:56
On Sun, 2007-08-05 at 09:54 +0200, Stefan Richter wrote:
Benjamin Herrenschmidt wrote:
quoted
quoted
quoted
If setting 32-bit DMA mask fails on ppc64, that sounds like a problem
with the DMA implementation on that architecture. There are enough cards
out there that only support 32-bit DMA that this really needs to work..
Yes, could the PPC folks please have a look at it? Thanks.
Smells like we may have a bug there. No worries though, all current PPC
machines have an iommu that will not give out addresses above 32 bits
anyway, but I'll double check what's up.
Do you see something in dmesg when that happens ?
There was nothing in Olaf's report, except for trouble in sbp2 _after_
the failure. http://lkml.org/lkml/2007/8/1/344 (I don't have a PMac.)
Hrm, allright, that's a bit weird. Olaf machine has only 256M of RAM
according to that dmesg, and thus, the kernel isn't enabling the iommu,
we use the "trivial" version of the dma mapping ops.
I suspect we have a bug in our imlementation of set_dma_mask though, in
that it does the "dma_supported" check using the previous mask and not
the one passed in :-)
The implementation of dma_supported that we hit in the no-iommu case
looks like that:
static int dma_direct_dma_supported(struct device *dev, u64 mask)
{
/* Could be improved to check for memory though it better be
* done via some global so platforms can set the limit in case
* they have limited DMA windows
*/
return mask >= DMA_32BIT_MASK;
}
So that should have worked. (The comment is a bit obscure, just ignore
it for now).
However, as I said above, our dma_set_mask() wrapper uses the wrong
value (the old, not the new mask). But that still should have worked
since the default dma mask for a PCI device is 0xffffffff....
Thus at this stable, I'm a bit at a loss of why it didn't work, I'll
have to test on one of those machines with some printk's in when I
manage to get to work (dunno when, kid's sick so I may have to stay home
today).
BTW. Any reason why you don't set the DMA mask in the ohci driver rather
than the sbp2 one ?
Cheers,
Ben.
From: Olaf Hering <hidden> Date: 2007-08-06 11:58:18
On Sun, Aug 05, Stefan Richter wrote:
Benjamin Herrenschmidt wrote:
quoted
quoted
quoted
If setting 32-bit DMA mask fails on ppc64, that sounds like a problem
with the DMA implementation on that architecture. There are enough cards
out there that only support 32-bit DMA that this really needs to work..
Yes, could the PPC folks please have a look at it? Thanks.
Smells like we may have a bug there. No worries though, all current PPC
machines have an iommu that will not give out addresses above 32 bits
anyway, but I'll double check what's up.
Do you see something in dmesg when that happens ?
There was nothing in Olaf's report, except for trouble in sbp2 _after_
the failure. http://lkml.org/lkml/2007/8/1/344 (I don't have a PMac.)
sbp2util_remove_command_orb_pool() does not check for lu->hi being NULL.
dev->dma_mask is NULL too, thats why dma_direct_dma_supported() returns
false, and dma_set_mask() will return -EIO.
From: Olaf Hering <hidden> Date: 2007-08-06 13:51:36
On Mon, Aug 06, Benjamin Herrenschmidt wrote:
BTW. Any reason why you don't set the DMA mask in the ohci driver rather
than the sbp2 one ?
I used this patch, and the attached CD was found.
What dma mask should be used in ohci_probe()?
---
drivers/ieee1394/ohci1394.c | 2 ++
drivers/ieee1394/sbp2.c | 3 +++
drivers/pci/pci.c | 1 +
drivers/pci/probe.c | 1 +
include/asm-powerpc/dma-mapping.h | 1 +
5 files changed, 8 insertions(+)
@@ -3223,6 +3223,8 @@ static int __devinit ohci1394_pci_probe(}#endif /* CONFIG_PPC_PMAC */+if(pci_set_dma_mask(dev,DMA_32BIT_MASK))+FAIL(-ENXIO,"Failed to set DMA mask");if(pci_enable_device(dev))FAIL(-ENXIO,"Failed to enable OHCI hardware");pci_set_master(dev);---a/drivers/ieee1394/sbp2.c+++b/drivers/ieee1394/sbp2.c
@@ -775,12 +775,15 @@ static struct sbp2_lu *sbp2_alloc_devicegotofailed_alloc;}#else+printk(KERN_DEBUG"%s():%u dev '%s' %p parent '%s' %p %016lx\n",__func__,__LINE__,hi->host->device.bus_id,hi->host->device.dma_mask,hi->host->device.parent->bus_id,(hi->host->device.parent->dma_mask),*(hi->host->device.parent->dma_mask));if(dma_set_mask(hi->host->device.parent,DMA_32BIT_MASK)){SBP2_ERR("failed to set 4GB DMA mask");gotofailed_alloc;}#endif}+else+printk(KERN_DEBUG"%s():%u dev '%s' parent '%s'\n",__func__,__LINE__,hi->host->device.bus_id,hi->host->device.parent->bus_id);/* Prevent unloading of the 1394 host */if(!try_module_get(hi->host->driver->owner)){---a/drivers/pci/pci.c+++b/drivers/pci/pci.c
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-06 21:43:53
On Mon, 2007-08-06 at 13:58 +0200, Olaf Hering wrote:
On Sun, Aug 05, Stefan Richter wrote:
quoted
Benjamin Herrenschmidt wrote:
quoted
quoted
quoted
If setting 32-bit DMA mask fails on ppc64, that sounds like a problem
with the DMA implementation on that architecture. There are enough cards
out there that only support 32-bit DMA that this really needs to work..
Yes, could the PPC folks please have a look at it? Thanks.
Smells like we may have a bug there. No worries though, all current PPC
machines have an iommu that will not give out addresses above 32 bits
anyway, but I'll double check what's up.
Do you see something in dmesg when that happens ?
There was nothing in Olaf's report, except for trouble in sbp2 _after_
the failure. http://lkml.org/lkml/2007/8/1/344 (I don't have a PMac.)
sbp2util_remove_command_orb_pool() does not check for lu->hi being NULL.
dev->dma_mask is NULL too, thats why dma_direct_dma_supported() returns
false, and dma_set_mask() will return -EIO.
Strange... PCI devices should never have a NULL dma mask. I wonder how
we get there...
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-06 21:48:00
On Mon, 2007-08-06 at 15:51 +0200, Olaf Hering wrote:
On Mon, Aug 06, Benjamin Herrenschmidt wrote:
quoted
BTW. Any reason why you don't set the DMA mask in the ohci driver rather
than the sbp2 one ?
I used this patch, and the attached CD was found.
What dma mask should be used in ohci_probe()?
Allright. So I see two problems here:
- in the code that powerpc uses to generate the PCI tree based on the
open firmware device-tree (instead of probing the bus), we don't set the
dma mask to the default ffffffff.
- our implementation of dma_supported() incorrectly tests against the
-previous- dma mask instead of the one we pass in as an argument.
I'll send a patch later today for you guys to test.
In addition, make sure that ieee1394 properly uses the parent PCI dev
and not some other intermediary struct device for the dma mask. Oh and,
don't do the set_dma_mask() in sbp2, it has nothing to do there. It
should be in the ohci1394 driver.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-06 22:05:46
powerpc has a couple of bugs in the usage of dma_masks that tend to
break when drivers explicitely try to set a 32 bits mask for example.
First the code that generates the pci devices from the OF device-tree
doesn't initialize the mask properly, then our implementation of
set_dma_mask() was trying to validate the -previous- mask value, not the
one passed in as an argument.
This patch should fix these.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Does this fix the problem you've noticed ?
(I do still think that sbp2 isn't the right place for that call btw :-)
Index: linux-work/include/asm-powerpc/dma-mapping.h
===================================================================
@@ -372,6 +372,7 @@dev->current_state=4;/* unknown power state */dev->error_state=pci_channel_io_normal;+dev->dma_mask=0xffffffff;if(!strcmp(type,"pci")||!strcmp(type,"pciex")){/* a PCI-PCI bridge */
From: Stefan Richter <stefanr@s5r6.in-berlin.de> Date: 2007-08-06 22:23:00
Benjamin Herrenschmidt wrote:
Oh and, don't do the set_dma_mask() in sbp2, it has nothing to do there.
It should be in the ohci1394 driver.
That's not quite right. OHCI-1394 implementations can go beyond 4GB bus
address space. (Although I don't know if there are such implementations
available. At least there are two implementations which can set the
so-called Physical Range bigger than 4GB.)
Sbp2 however requires that everything which it DMA-maps resides in the
Physical Range of the controller. This way the CPU is not involved in
most of the data transfers. The OHCI-1394 controller acts as bus bridge
between IEEE 1394 bus and local bus, with a 1:1 mapping of IEEE 1394 bus
addresses to and from local bus addresses --- but not in the whole 48
bits white IEEE 1394 bus address range, only in the
implementation-dependent Physical Range. The minimum Physical Range
that all OHCI-1394 implementations guarantee is 4GB. I could actually
have set a bigger mask in sbp2 when the controller supports a
programmable bigger range.
So that's the story why that dma_set_mask went into sbp2: Sbp2 wants
mappings in a _subset_ of the OHCI-1394 controllers DMA range.
Anyway. For now I will simply go with what 2.6.23-rc has and what
2.6.21 had: No dma_set_mask anywhere in the 1394 subsystem. We can
revisit this whenever an actual need arises.
--
Stefan Richter
-=====-=-=== =--- --==-
http://arcgraph.de/sr/
From: Robert Hancock <hidden> Date: 2007-08-06 22:25:46
Stefan Richter wrote:
Benjamin Herrenschmidt wrote:
quoted
Oh and, don't do the set_dma_mask() in sbp2, it has nothing to do there.
It should be in the ohci1394 driver.
That's not quite right. OHCI-1394 implementations can go beyond 4GB bus
address space. (Although I don't know if there are such implementations
available. At least there are two implementations which can set the
so-called Physical Range bigger than 4GB.)
Sbp2 however requires that everything which it DMA-maps resides in the
Physical Range of the controller. This way the CPU is not involved in
most of the data transfers. The OHCI-1394 controller acts as bus bridge
between IEEE 1394 bus and local bus, with a 1:1 mapping of IEEE 1394 bus
addresses to and from local bus addresses --- but not in the whole 48
bits white IEEE 1394 bus address range, only in the
implementation-dependent Physical Range. The minimum Physical Range
that all OHCI-1394 implementations guarantee is 4GB. I could actually
have set a bigger mask in sbp2 when the controller supports a
programmable bigger range.
So that's the story why that dma_set_mask went into sbp2: Sbp2 wants
mappings in a _subset_ of the OHCI-1394 controllers DMA range.
Anyway. For now I will simply go with what 2.6.23-rc has and what
2.6.21 had: No dma_set_mask anywhere in the 1394 subsystem. We can
revisit this whenever an actual need arises.
Not sure this is a very good idea. This seems rather likely to fail on
x86_64 machines with >4GB of RAM for example..
--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-06 22:29:36
On Tue, 2007-08-07 at 00:22 +0200, Stefan Richter wrote:
Benjamin Herrenschmidt wrote:
quoted
Oh and, don't do the set_dma_mask() in sbp2, it has nothing to do there.
It should be in the ohci1394 driver.
That's not quite right. OHCI-1394 implementations can go beyond 4GB bus
address space. (Although I don't know if there are such implementations
available. At least there are two implementations which can set the
so-called Physical Range bigger than 4GB.)
Hrm..
Sbp2 however requires that everything which it DMA-maps resides in the
Physical Range of the controller. This way the CPU is not involved in
most of the data transfers. The OHCI-1394 controller acts as bus bridge
between IEEE 1394 bus and local bus, with a 1:1 mapping of IEEE 1394 bus
addresses to and from local bus addresses --- but not in the whole 48
bits white IEEE 1394 bus address range, only in the
implementation-dependent Physical Range. The minimum Physical Range
that all OHCI-1394 implementations guarantee is 4GB. I could actually
have set a bigger mask in sbp2 when the controller supports a
programmable bigger range.
So that's the story why that dma_set_mask went into sbp2: Sbp2 wants
mappings in a _subset_ of the OHCI-1394 controllers DMA range.
Anyway. For now I will simply go with what 2.6.23-rc has and what
2.6.21 had: No dma_set_mask anywhere in the 1394 subsystem. We can
revisit this whenever an actual need arises.
Ok, I see your point, however, the problem is that at this stage, in
linux, you really can only set the dma mask on the pci device of the
ohci. That means that sbp2 will effectively change the ohci's DMA mask
for all OHCI operations...
Architectures like powerpc (and possibly others) need to track what
iommu/bridge/etc... a device is on for proper mapping and provide
different DMA operations for different busses. Thus, only devices that
have properly been "setup" by the architecture for DMA can use the DMA
operations.
So while ideally, sbp2 should set the dma mask on itself (or rather on
the sbp2 device) and then use that device for dma mapping operations,
this will not work.
Now, we could try to devise a generic API for use by things such as
ieee1394 to "make DMA'able" sub devices of a pci device with local dma
masks. At least for powerpc, it wouldn't be too hard, but other archs
might have issues if they do things like test for the bus_type.
Cheers,
Ben.
From: Olaf Hering <hidden> Date: 2007-08-06 22:30:30
On Tue, Aug 07, Benjamin Herrenschmidt wrote:
powerpc has a couple of bugs in the usage of dma_masks that tend to
break when drivers explicitely try to set a 32 bits mask for example.
First the code that generates the pci devices from the OF device-tree
doesn't initialize the mask properly, then our implementation of
set_dma_mask() was trying to validate the -previous- mask value, not the
one passed in as an argument.
This patch should fix these.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Does this fix the problem you've noticed ?
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-08-06 22:32:42
On Mon, 2007-08-06 at 16:25 -0600, Robert Hancock wrote:
quoted
Anyway. For now I will simply go with what 2.6.23-rc has and what
2.6.21 had: No dma_set_mask anywhere in the 1394 subsystem. We can
revisit this whenever an actual need arises.
Not sure this is a very good idea. This seems rather likely to fail on
x86_64 machines with >4GB of RAM for example..
Would it ? Isn't the default DMA mask for PCI devices set to 32 bits
anyway ? In which case, swiotlb will take care of the matter.
Cheers,
Ben.
From: Robert Hancock <hidden> Date: 2007-08-06 22:35:46
Benjamin Herrenschmidt wrote:
On Mon, 2007-08-06 at 16:25 -0600, Robert Hancock wrote:
quoted
quoted
Anyway. For now I will simply go with what 2.6.23-rc has and what
2.6.21 had: No dma_set_mask anywhere in the 1394 subsystem. We can
revisit this whenever an actual need arises.
Not sure this is a very good idea. This seems rather likely to fail on
x86_64 machines with >4GB of RAM for example..
Would it ? Isn't the default DMA mask for PCI devices set to 32 bits
anyway ? In which case, swiotlb will take care of the matter.
Cheers,
Ben.
Hmm, that's true, yes. Suppose it shouldn't be a problem then.
I would agree, though, that sbp2 isn't really the place for setting
this, since the DMA mask is presently a property of the device, not of
the user..
--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/
From: Stefan Richter <stefanr@s5r6.in-berlin.de> Date: 2007-08-06 22:49:14
Robert Hancock wrote:
Stefan Richter wrote:
quoted
So that's the story why that dma_set_mask went into sbp2: Sbp2 wants
mappings in a _subset_ of the OHCI-1394 controllers DMA range.
Anyway. For now I will simply go with what 2.6.23-rc has and what
2.6.21 had: No dma_set_mask anywhere in the 1394 subsystem. We can
revisit this whenever an actual need arises.
Not sure this is a very good idea. This seems rather likely to fail on
x86_64 machines with >4GB of RAM for example..
I'll deal with it when a bug report comes in.
1.) The ieee1394 subsystem is known to work on x86_64 with more than 4
GB RAM, so I gather that architecture code already sets a proper DMA
mask for all those 32bit PCI OHCI-1394 implementations out there.
2.) I haven't heard of an OHCI-1394 chip whose overall DMA range was
bigger than the Physical Range. There are only two chips, from ALi and
from Fujitsu, whose Physical Range is curiously bigger than the actual
DMA range.
--
Stefan Richter
-=====-=-=== =--- --===
http://arcgraph.de/sr/
From: Stefan Richter <stefanr@s5r6.in-berlin.de> Date: 2007-08-06 23:00:43
Robert Hancock wrote:
I would agree, though, that sbp2 isn't really the place for setting
this, since the DMA mask is presently a property of the device, not of
the user..
The mask that sbp2 set was because sbp2 has (in theory, not yet in
practice) a _narrower requirement on address ranges than the chip_ ---
hence it has (in theory) a narrower requirement on DMA mappings than the
ohci1394 driver has.
That's because sbp2 uses the controller in a special mode, as a bus
bridge. It is the only user of that feature among the higher-level IEEE
1394 drivers. No other IEEE 1394 application-layer software has this
requirement.
(Well, debugging and forensic tools rely on that mode too, notably
BenH's firescope, but this software runs remote, hence it's a different
beast from sbp2.)
--
Stefan Richter
-=====-=-=== =--- --===
http://arcgraph.de/sr/