On 18 Aug 2003 00:34:17 +0200
Krzysztof Halasa <[email protected]> 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.
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.
On 18 Aug 2003 14:44:19 +0200
Krzysztof Halasa <[email protected]> wrote:
> "David S. Miller" <[email protected]> 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.
"David S. Miller" <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> writes:
Krzysztof> "David S. Miller" <[email protected]> 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
> From: Krzysztof Halasa <[email protected]>
> 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
Pete Zaitcev <[email protected]> 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
On 18 Aug 2003 17:54:42 +0200
Krzysztof Halasa <[email protected]> wrote:
> "David S. Miller" <[email protected]> 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.
"David S. Miller" <[email protected]> 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
"David S. Miller" <[email protected]> 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
On 18 Aug 2003 20:21:48 +0200
Krzysztof Halasa <[email protected]> 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.
"David S. Miller" <[email protected]> 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
>>>>> "Pete" == Pete Zaitcev <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> writes:
Krzysztof> "David S. Miller" <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> writes:
Krzysztof> Pete Zaitcev <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> 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
Jes Sorensen <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> writes:
Krzysztof> Jes Sorensen <[email protected]> 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
>>>>> "Alan" == Alan Cox <[email protected]> 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
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.
On 19 Aug 2003 14:07:18 +0100
Alan Cox <[email protected]> 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.
On 19 Aug 2003 19:33:19 +0100
Alan Cox <[email protected]> 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.
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
"David S. Miller" <[email protected]> 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
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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> 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
On 24 Aug 2003 14:06:43 +0200
Krzysztof Halasa <[email protected]> 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.
Jes Sorensen <[email protected]> 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
"David S. Miller" <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> writes:
Krzysztof> "David S. Miller" <[email protected]> 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
>>>>> "Krzysztof" == Krzysztof Halasa <[email protected]> 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
David, any comments?
"David S. Miller" <[email protected]> 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
On 30 Aug 2003 23:18:50 +0200
Krzysztof Halasa <[email protected]> wrote:
> David, any comments?
I'm too busy to partake in this thread right now, sorry.
On Sul, 2003-08-31 at 02:50, David S. Miller wrote:
> On 30 Aug 2003 23:18:50 +0200
> Krzysztof Halasa <[email protected]> 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
Alan Cox <[email protected]> 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
On Sun, 31 Aug 2003 13:52:55 +0100
Alan Cox <[email protected]> wrote:
> On Sul, 2003-08-31 at 02:50, David S. Miller wrote:
> > On 30 Aug 2003 23:18:50 +0200
> > Krzysztof Halasa <[email protected]> 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.
"David S. Miller" <[email protected]> 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
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.
On 01 Sep 2003 09:34:50 +0200
Krzysztof Halasa <[email protected]> wrote:
> "David S. Miller" <[email protected]> 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.
On Mon, 01 Sep 2003 08:54:18 +0100
Alan Cox <[email protected]> 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.
"David S. Miller" <[email protected]> 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
"David S. Miller" <[email protected]> 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
On 01 Sep 2003 19:14:46 +0200
Krzysztof Halasa <[email protected]> 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.
On 01 Sep 2003 18:27:45 +0200
Krzysztof Halasa <[email protected]> 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.
"David S. Miller" <[email protected]> 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