The reasons why dma_free_attrs() should not be called from IRQ context
are not necessarily obvious and somewhat buried in the development
history, so let's start by documenting the warning itself to help anyone
who does happen to hit it and wonder what the deal is.
However, this check turns out to be slightly over-restrictive for the
way that per-device memory has been spliced into the general API, since
for that case we know that dma_declare_coherent_memory() has created an
appropriate CPU mapping for the entire area and nothing dynamic should
be happening. Given that the usage model for per-device memory is often
more akin to streaming DMA than 'real' coherent DMA (e.g. allocating and
freeing space to copy short-lived packets in and out), it is also
somewhat more reasonable for those operations to happen in IRQ handlers
for such devices.
A somewhat similar line of reasoning also applies at the other end for
the mask check in dma_alloc_attrs() too - indeed, a device which cannot
access anything other than its own local memory probably *shouldn't*
have a valid mask for the general coherent DMA API.
Therefore, let's move the per-device area hooks up ahead of the assorted
checks, so that they get a chance to resolve the request before we get
as far as definite "you're doing it wrong" territory.
Reported-by: Fredrik Noring <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
include/linux/dma-mapping.h | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index f9cc309507d9..ffeca3ab59c0 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -512,12 +512,12 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
const struct dma_map_ops *ops = get_dma_ops(dev);
void *cpu_addr;
- BUG_ON(!ops);
- WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
-
if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
return cpu_addr;
+ BUG_ON(!ops);
+ WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
+
/* let the implementation decide on the zone to allocate from: */
flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
@@ -537,12 +537,19 @@ static inline void dma_free_attrs(struct device *dev, size_t size,
{
const struct dma_map_ops *ops = get_dma_ops(dev);
- BUG_ON(!ops);
- WARN_ON(irqs_disabled());
-
if (dma_release_from_dev_coherent(dev, get_order(size), cpu_addr))
return;
+ BUG_ON(!ops);
+ /*
+ * On non-coherent platforms which implement DMA-coherent buffers via
+ * non-cacheable remaps, ops->free() may call vunmap(). Thus arriving
+ * here in IRQ context is a) at risk of a BUG_ON() or trying to sleep
+ * on some machines, and b) an indication that the driver is probably
+ * misusing the coherent API anyway.
+ */
+ WARN_ON(irqs_disabled());
+
if (!ops->free || !cpu_addr)
return;
--
2.17.1.dirty
Thank you Robin,
On Tue, Jul 03, 2018 at 02:08:30PM +0100, Robin Murphy wrote:
> The reasons why dma_free_attrs() should not be called from IRQ context
> are not necessarily obvious and somewhat buried in the development
> history, so let's start by documenting the warning itself to help anyone
> who does happen to hit it and wonder what the deal is.
>
> However, this check turns out to be slightly over-restrictive for the
> way that per-device memory has been spliced into the general API, since
> for that case we know that dma_declare_coherent_memory() has created an
> appropriate CPU mapping for the entire area and nothing dynamic should
> be happening. Given that the usage model for per-device memory is often
> more akin to streaming DMA than 'real' coherent DMA (e.g. allocating and
> freeing space to copy short-lived packets in and out), it is also
> somewhat more reasonable for those operations to happen in IRQ handlers
> for such devices.
>
> A somewhat similar line of reasoning also applies at the other end for
> the mask check in dma_alloc_attrs() too - indeed, a device which cannot
> access anything other than its own local memory probably *shouldn't*
> have a valid mask for the general coherent DMA API.
>
> Therefore, let's move the per-device area hooks up ahead of the assorted
> checks, so that they get a chance to resolve the request before we get
> as far as definite "you're doing it wrong" territory.
I have tested this patch and it corrects both problems with the PS2 OHCI
driver. I believe there is a fair chance that drivers/usb/host/ohci-sm501.c
and drivers/usb/host/ohci-tmio.c are fixed as well, since they are similar.
Tested-by: Fredrik Noring <[email protected]>
Fredrik
> Reported-by: Fredrik Noring <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> include/linux/dma-mapping.h | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index f9cc309507d9..ffeca3ab59c0 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -512,12 +512,12 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
> const struct dma_map_ops *ops = get_dma_ops(dev);
> void *cpu_addr;
>
> - BUG_ON(!ops);
> - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> -
> if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
> return cpu_addr;
>
> + BUG_ON(!ops);
> + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> +
> /* let the implementation decide on the zone to allocate from: */
> flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);
>
> @@ -537,12 +537,19 @@ static inline void dma_free_attrs(struct device *dev, size_t size,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> - BUG_ON(!ops);
> - WARN_ON(irqs_disabled());
> -
> if (dma_release_from_dev_coherent(dev, get_order(size), cpu_addr))
> return;
>
> + BUG_ON(!ops);
> + /*
> + * On non-coherent platforms which implement DMA-coherent buffers via
> + * non-cacheable remaps, ops->free() may call vunmap(). Thus arriving
> + * here in IRQ context is a) at risk of a BUG_ON() or trying to sleep
> + * on some machines, and b) an indication that the driver is probably
> + * misusing the coherent API anyway.
> + */
> + WARN_ON(irqs_disabled());
> +
> if (!ops->free || !cpu_addr)
> return;
>
> --
> 2.17.1.dirty
>
> - BUG_ON(!ops);
> - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> -
> if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
> return cpu_addr;
>
> + BUG_ON(!ops);
> + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
I think doing dma on a device without ops is completely broken no matter
what you think of it, so I very much disagree with that part of the change.
Also while I don't think not having a dma mask is a good idea even for
a driver purely using dma coherent pools. If the pools really are on
the device itself I can see why it might not matter, but for the case
commonly used on some ARM SOCs where we just reserve memory for certain
devices from a system pool it very much does matter.
There really is no good excuse to not set a coherent mask in the drivers.
On 05/07/18 20:36, Christoph Hellwig wrote:
>> - BUG_ON(!ops);
>> - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
>> -
>> if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
>> return cpu_addr;
>>
>> + BUG_ON(!ops);
>> + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
>
> I think doing dma on a device without ops is completely broken no matter
> what you think of it, so I very much disagree with that part of the change.
>
> Also while I don't think not having a dma mask is a good idea even for
> a driver purely using dma coherent pools. If the pools really are on
> the device itself I can see why it might not matter, but for the case
> commonly used on some ARM SOCs where we just reserve memory for certain
> devices from a system pool it very much does matter.
>
> There really is no good excuse to not set a coherent mask in the drivers.
Right, I was rather on the fence about this - on the one hand it is
objectively wrong per the API for drivers to call dma_alloc_coherent()
without a prior successful dma_set_coherent_mask() call, but then I
thought that in the case when they're *only* using it as a proxy for
dma_alloc_from_dev_coherent() and explicitly don't want regular
allocations from kernel memory to ever happen, then maybe it might be
somewhat reasonable. But indeed I hadn't really given enough thought to
the reserved-memory carveout case, where we definitely don't want to let
a legitimate warning be hidden on a developer's machine but hit by users
with different system configurations.
Fredrik, are you happy to fix up your driver to initialise a suitable
mask at probe time?
Robin.
Hi Robin,
On Fri, Jul 06, 2018 at 12:57:11PM +0100, Robin Murphy wrote:
> On 05/07/18 20:36, Christoph Hellwig wrote:
> > > - BUG_ON(!ops);
> > > - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> > > -
> > > if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
> > > return cpu_addr;
> > > + BUG_ON(!ops);
> > > + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> >
> > I think doing dma on a device without ops is completely broken no matter
> > what you think of it, so I very much disagree with that part of the change.
> >
> > Also while I don't think not having a dma mask is a good idea even for
> > a driver purely using dma coherent pools. If the pools really are on
> > the device itself I can see why it might not matter, but for the case
> > commonly used on some ARM SOCs where we just reserve memory for certain
> > devices from a system pool it very much does matter.
> >
> > There really is no good excuse to not set a coherent mask in the drivers.
>
> Right, I was rather on the fence about this - on the one hand it is
> objectively wrong per the API for drivers to call dma_alloc_coherent()
> without a prior successful dma_set_coherent_mask() call, but then I thought
> that in the case when they're *only* using it as a proxy for
> dma_alloc_from_dev_coherent() and explicitly don't want regular allocations
> from kernel memory to ever happen, then maybe it might be somewhat
> reasonable. But indeed I hadn't really given enough thought to the
> reserved-memory carveout case, where we definitely don't want to let a
> legitimate warning be hidden on a developer's machine but hit by users with
> different system configurations.
>
> Fredrik, are you happy to fix up your driver to initialise a suitable mask
> at probe time?
The driver currently only uses dma_declare_coherent_memory and
dma_release_declared_memory. It allocates 256 KiB of DMA memory from the IOP
(a MIPS R3000 I/O processor that is separate from the main MIPS R5900) with
iop_dma_addr = iop_alloc(size);
and then declares it with
dma_declare_coherent_memory(dev,
iop_bus_to_phys(iop_dma_addr),
iop_dma_addr, size, flags);
where iop_bus_to_phys is:
#define IOP_MEMORY_BASE_ADDRESS 0x1c000000
phys_addr_t iop_bus_to_phys(iop_addr_t baddr)
{
return (u32)baddr + IOP_MEMORY_BASE_ADDRESS;
}
Does dma_set_coherent_mask want a device object representing the IOP? Such
a thing is currently not implemented, but can certainly be done.
As you noted, the kernel cannot and must not allocate any kind of normal
memory for this device. Typical DMA addresses 0-0x200000 are mapped to
0x1c000000-0x1c200000 and is memory managed exclusively by the IOP. What
would be a suitable mask for that?
drivers/usb/host/ohci-sm501.c and drivers/usb/host/ohci-tmio.c are similar,
so I suppose they need to be fixed as well?
Fredrik
On 06/07/18 15:19, Fredrik Noring wrote:
> Hi Robin,
>
> On Fri, Jul 06, 2018 at 12:57:11PM +0100, Robin Murphy wrote:
>> On 05/07/18 20:36, Christoph Hellwig wrote:
>>>> - BUG_ON(!ops);
>>>> - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
>>>> -
>>>> if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
>>>> return cpu_addr;
>>>> + BUG_ON(!ops);
>>>> + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
>>>
>>> I think doing dma on a device without ops is completely broken no matter
>>> what you think of it, so I very much disagree with that part of the change.
>>>
>>> Also while I don't think not having a dma mask is a good idea even for
>>> a driver purely using dma coherent pools. If the pools really are on
>>> the device itself I can see why it might not matter, but for the case
>>> commonly used on some ARM SOCs where we just reserve memory for certain
>>> devices from a system pool it very much does matter.
>>>
>>> There really is no good excuse to not set a coherent mask in the drivers.
>>
>> Right, I was rather on the fence about this - on the one hand it is
>> objectively wrong per the API for drivers to call dma_alloc_coherent()
>> without a prior successful dma_set_coherent_mask() call, but then I thought
>> that in the case when they're *only* using it as a proxy for
>> dma_alloc_from_dev_coherent() and explicitly don't want regular allocations
>> from kernel memory to ever happen, then maybe it might be somewhat
>> reasonable. But indeed I hadn't really given enough thought to the
>> reserved-memory carveout case, where we definitely don't want to let a
>> legitimate warning be hidden on a developer's machine but hit by users with
>> different system configurations.
>>
>> Fredrik, are you happy to fix up your driver to initialise a suitable mask
>> at probe time?
>
> The driver currently only uses dma_declare_coherent_memory and
> dma_release_declared_memory. It allocates 256 KiB of DMA memory from the IOP
> (a MIPS R3000 I/O processor that is separate from the main MIPS R5900) with
>
> iop_dma_addr = iop_alloc(size);
>
> and then declares it with
>
> dma_declare_coherent_memory(dev,
> iop_bus_to_phys(iop_dma_addr),
> iop_dma_addr, size, flags);
>
> where iop_bus_to_phys is:
>
> #define IOP_MEMORY_BASE_ADDRESS 0x1c000000
> phys_addr_t iop_bus_to_phys(iop_addr_t baddr)
> {
> return (u32)baddr + IOP_MEMORY_BASE_ADDRESS;
> }
>
> Does dma_set_coherent_mask want a device object representing the IOP? Such
> a thing is currently not implemented, but can certainly be done.
Nope, just the same OHCI device as the dma_declare_coherent_memory() call.
> As you noted, the kernel cannot and must not allocate any kind of normal
> memory for this device. Typical DMA addresses 0-0x200000 are mapped to
> 0x1c000000-0x1c200000 and is memory managed exclusively by the IOP. What
> would be a suitable mask for that?
For the sake of accuracy, I guess maybe DMA_BIT_MASK(20) since that's
what the OHCI's effective addressing capability is, even if it does
happen to be to remote IOP RAM. Alternatively, there is perhaps some
degree of argument for deliberately picking a nonzero but useless value
like 1, although it looks like the MIPS allocator (at least the
dma-default one) never actually checks whether the page it gets is
within range of the device's coherent mask, which it probably should do.
> drivers/usb/host/ohci-sm501.c and drivers/usb/host/ohci-tmio.c are similar,
> so I suppose they need to be fixed as well?
Possibly, or maybe they get away with it by virtue of bus code setting
some default mask. Since those look to be pretty old bits of hardware
that I know nothing about, I'd be hesitant to start adding stuff. If
there is anyone still using such things and running mainline on them, I
guess we'll hear about it in due course...
Robin.
Hi Robin,
> > Does dma_set_coherent_mask want a device object representing the IOP? Such
> > a thing is currently not implemented, but can certainly be done.
>
> Nope, just the same OHCI device as the dma_declare_coherent_memory() call.
Ah... and then some kind of dma_ops structure is needed to avoid -EIO?
Possibly using set_dma_ops()?
By the way, the DMA hardware supports executing device->{memory,FIFO},
{memory,FIFO}->device and FIFO<->memory simultaneously, with hardware stall
control, between different devices where memory or FIFOs act as intermediate
buffers. Memory can also be a source or a destination.
That might be a way to have the OHCI efficiently transfer data from/to main
memory. I suppose it would be two simultaneously linked DMA transfers such
as
OHCI <-> SIF <-> main memory
or even three linked DMA transfers as in
OHCI <-> IOP memory <-> SIF <-> main memory
where SIF is the IOP DMA interface, which is a bidirectional hardware FIFO.
Does the kernel DMA subsystem support simultaneously linked DMA transfers?
In addition, the DMA hardware also supports scatter-gather (chaining), so
memory need not be physically continuous.
> > As you noted, the kernel cannot and must not allocate any kind of normal
> > memory for this device. Typical DMA addresses 0-0x200000 are mapped to
> > 0x1c000000-0x1c200000 and is memory managed exclusively by the IOP. What
> > would be a suitable mask for that?
>
> For the sake of accuracy, I guess maybe DMA_BIT_MASK(20) since that's what
> the OHCI's effective addressing capability is, even if it does happen to be
> to remote IOP RAM.
Isn't it 21 for 2 MiB? Hmm... I'm considering raising that to 8 MiB since
there are such devices too.
> Alternatively, there is perhaps some degree of argument
> for deliberately picking a nonzero but useless value like 1, although it
> looks like the MIPS allocator (at least the dma-default one) never actually
> checks whether the page it gets is within range of the device's coherent
> mask, which it probably should do.
Perhaps Maciej knows more about the details of the MIPS allocator?
Fredrik
Hello Fredrik,
> Gesendet: Freitag, 06. Juli 2018 um 22:54 Uhr
> Von: "Fredrik Noring" <[email protected]>
> An: "Robin Murphy" <[email protected]>
> Cc: "Christoph Hellwig" <[email protected]>, [email protected], [email protected], "Maciej W. Rozycki" <[email protected]>, [email protected]
> Betreff: Re: [PATCH] dma-mapping: Relax warnings for per-device areas
>
> Hi Robin,
>
> > > Does dma_set_coherent_mask want a device object representing the IOP? Such
> > > a thing is currently not implemented, but can certainly be done.
> >
> > Nope, just the same OHCI device as the dma_declare_coherent_memory() call.
>
> Ah... and then some kind of dma_ops structure is needed to avoid -EIO?
> Possibly using set_dma_ops()?
>
> By the way, the DMA hardware supports executing device->{memory,FIFO},
> {memory,FIFO}->device and FIFO<->memory simultaneously, with hardware stall
> control, between different devices where memory or FIFOs act as intermediate
> buffers. Memory can also be a source or a destination.
>
> That might be a way to have the OHCI efficiently transfer data from/to main
> memory. I suppose it would be two simultaneously linked DMA transfers such
> as
>
> OHCI <-> SIF <-> main memory
>
> or even three linked DMA transfers as in
>
> OHCI <-> IOP memory <-> SIF <-> main memory
>
> where SIF is the IOP DMA interface, which is a bidirectional hardware FIFO.
>
> Does the kernel DMA subsystem support simultaneously linked DMA transfers?
> In addition, the DMA hardware also supports scatter-gather (chaining), so
> memory need not be physically continuous.
Don't forget that the SIF DMA packets are limited and the kernel will block/reschedule when it is out of SIF DMA packets. The allocation is implemented inside the SBIOS. You may easily get a deadlock or a livelock when you just let it run without thinking about the design. When you use the old CDVD driver on IOP, the RPC code inside SBIOS tries to simulate the interface like the new CDVD driver. The problem is that this is done by a busy loop waiting for a free SIF DMA packet. This would block the complete Linux kernel for an unknown time.
As I understand you, you wanted to move the SBIOS code inside the Linux kernel. I am not sure whether you already have done it. When you do this, it is easier to fix the CDVD problem, but you need to think about booting using the official RTE disc from Sony for Linux, because it loads different modules and a different SBIOS. As this is the official way to start Linux on the PS2 which is supported by Sony, we should also support this in the official Linux kernel. Kernelloader can partially simulate it, but you need the files from the RTE disc.
> > > As you noted, the kernel cannot and must not allocate any kind of normal
> > > memory for this device. Typical DMA addresses 0-0x200000 are mapped to
> > > 0x1c000000-0x1c200000 and is memory managed exclusively by the IOP. What
> > > would be a suitable mask for that?
> >
> > For the sake of accuracy, I guess maybe DMA_BIT_MASK(20) since that's what
> > the OHCI's effective addressing capability is, even if it does happen to be
> > to remote IOP RAM.
>
> Isn't it 21 for 2 MiB? Hmm... I'm considering raising that to 8 MiB since
> there are such devices too.
At least on some models I think you can desolder the RAM and replace it by a larger memory up to 4GB, because the 1394 Lead Vehicle Manual lists a feature:
"Hardware generated response to received read or write requests in a designated 4GB address range without CPU involvment."
The 1394 Lead Vehicle is not used in the PS2, but it is very similar to the IOP and it is the only manual we have about IOP. So I think the DMA mask for the device must be at least 32 Bits, because the device is able to access full 32 Bit. The EE where Linux is running may only be able to access a part of it directly. I think SIF DMA is always able to access it completely, as this is an official feature which is documented. The mapping at 0x1c000000-0x1c200000 seems just to be good luck, because it is not documented. As this is no official interface Sony is able to remove this mapping at any time in a new model. I don't know where the border of the mapping is, but in my experiements I have seen some hints that it can be different depending on which hardware or software is used. It looks like the more stuff is integrated into one single chip, the lower is the border, because I have seen strange behaviour and exceptions when accessing this memory on newer PS2 model. I limited the memory to 256KB for USB OHCI because of this strange behaviour on some models, but I wasn't able to figure out what was the real cause of the problem. I just recognized that it was stable with the 256KB limit.
So the question is: What is the purpose of the DMA mask in Linux? Is it the area which can be accessed by the device? Or is it the area which can be accessed by the CPU? For the device it is 32 Bit. For the CPU it depends on the software and hardware and can be 0, because nothing may be shared with the CPU. Even with DMA mask 0, the SIF DMA is still able to access the full 32 Bit. Each memory access by the OHCI driver can't be done directly and needs first to be transferred to IOP memory via SIF DMA before it can be accessed by OHCI DMA. This is what you called linked DMA transfer. As far as I remember the USB sub-system and the OHCI driver was not written to handle memory which can't be written by the CPU at all. So I assume that you first need to allocate some temporary memory which is used to copy the data to or from the IOP DMA memory. Then I think you can increase the 256KB limit without getting an unstable system.
I heard someone talking about problems in the SMMU which were fixed by increasing the DMA mask. This lets me believe that the DMA mask is something which is required by SMMU and therefore 32 Bit should be correct. But when a hardware designer tries to add an IOMMU to the PS2, there would be at least 3 different IOMMUs needed, because we have 3 different buses (for EE, IOP and GS).
> > Alternatively, there is perhaps some degree of argument
> > for deliberately picking a nonzero but useless value like 1, although it
> > looks like the MIPS allocator (at least the dma-default one) never actually
> > checks whether the page it gets is within range of the device's coherent
> > mask, which it probably should do.
>
> Perhaps Maciej knows more about the details of the MIPS allocator?
The original approach for USB OHCI in Linux 2.4 was that IOP memory is handled as PCI memory. I.e. the driver was "thinking" that it allocates PCI memory, but indeed there was IOP memory allocated. All DMA ops where implemented in the PCI ops and so it was just working like a PCI card with device local memory.
Best regards
Jürgen Urban
Hi Jürgen, Robin,
> Don't forget that the SIF DMA packets are limited and the kernel will
> block/reschedule when it is out of SIF DMA packets. The allocation is
> implemented inside the SBIOS. You may easily get a deadlock or a livelock
> when you just let it run without thinking about the design. When you use
> the old CDVD driver on IOP, the RPC code inside SBIOS tries to simulate
> the interface like the new CDVD driver. The problem is that this is done
> by a busy loop waiting for a free SIF DMA packet. This would block the
> complete Linux kernel for an unknown time.
>
> As I understand you, you wanted to move the SBIOS code inside the Linux
> kernel. I am not sure whether you already have done it. When you do this,
> it is easier to fix the CDVD problem, but you need to think about booting
> using the official RTE disc from Sony for Linux, because it loads
> different modules and a different SBIOS. As this is the official way to
> start Linux on the PS2 which is supported by Sony, we should also support
> this in the official Linux kernel. Kernelloader can partially simulate it,
> but you need the files from the RTE disc.
The kernel no longer needs or uses the SBIOS, partly due to the issues
with having binary blobs of code that do kernel services. SBIOS memory is
reclaimed, so the SBIOS does not even exist when the kernel is running.
DMA is therefore only limited by the hardware design, which supports both
multiple simultaneously interconnected DMA controllers via memory or FIFOs,
and chained (scatter-gather) transfers.
Robin, does the kernel DMA subsystem support interconnected DMA controllers?
That involves arbitration of hardware FIFO resources (for example the SIF).
The Kernelloader boot program is not needed either, for any service, because
the IOP is reset and initialised by the kernel itself. Booting the kernel is
much faster and reliable without using the Kernelloader which frequently
crashes or refuses to load IOP modules.
The Kernelloader can still be used, if one wishes, but it's optional and not
a requirement.
> At least on some models I think you can desolder the RAM and replace it by
> a larger memory up to 4GB, because the 1394 Lead Vehicle Manual lists a
> feature:
>
> "Hardware generated response to received read or write requests in a
> designated 4GB address range without CPU involvment."
>
> The 1394 Lead Vehicle is not used in the PS2, but it is very similar to
> the IOP and it is the only manual we have about IOP. So I think the DMA
> mask for the device must be at least 32 Bits, because the device is able
> to access full 32 Bit. The EE where Linux is running may only be able to
> access a part of it directly. I think SIF DMA is always able to access it
> completely, as this is an official feature which is documented. The
> mapping at 0x1c000000-0x1c200000 seems just to be good luck, because it is
> not documented. As this is no official interface Sony is able to remove
> this mapping at any time in a new model. I don't know where the border of
> the mapping is, but in my experiements I have seen some hints that it can
> be different depending on which hardware or software is used. It looks
> like the more stuff is integrated into one single chip, the lower is the
> border, because I have seen strange behaviour and exceptions when
> accessing this memory on newer PS2 model. I limited the memory to 256KB
> for USB OHCI because of this strange behaviour on some models, but I
> wasn't able to figure out what was the real cause of the problem. I just
> recognized that it was stable with the 256KB limit.
Perhaps we need to invent a memory map zone within the IOP. I hope that we
can make full use of the DMA hardware, because DMA is by a wide margin the
most efficient kind of transfer.
> So the question is: What is the purpose of the DMA mask in Linux? Is it
> the area which can be accessed by the device? Or is it the area which can
> be accessed by the CPU? For the device it is 32 Bit. For the CPU it
> depends on the software and hardware and can be 0, because nothing may be
> shared with the CPU.
That's a good question. The DMA mapping updates that cause regressions need
some kind of mask, but I agree, it's unclear what that mask actually means.
Especially considering that the kernel cannot allocate normal memory for IOP
DMA anyway, so what is the purpose of the mask then?
> Even with DMA mask 0, the SIF DMA is still able to access the full 32 Bit.
> Each memory access by the OHCI driver can't be done directly and needs
> first to be transferred to IOP memory via SIF DMA before it can be
> accessed by OHCI DMA. This is what you called linked DMA transfer.
Right. So the IOP has DMA controllers that are also capable of simultaneously
interconnected (linked) transfers such as device<->memory<->SIF? That would
be very fortunate. A slight complication is that the SIF eventually needs
arbitration to support simultaneous transfers for multiple devices such as
the OHIC, ATA, iLink, Ethernet, etc.
Are you aware of any documentation describing the IOP DMA controllers?
> As far as I remember the USB sub-system and the OHCI driver was not
> written to handle memory which can't be written by the CPU at all. So I
> assume that you first need to allocate some temporary memory which is used
> to copy the data to or from the IOP DMA memory.
Exactly, that OHCI design is still used. Also important, we need to
investigate why OHCI interrupts sometimes are lost. Do you have any idea?
> Then I think you can increase the 256KB limit without getting an unstable
> system.
I would like to learn more about the source of this problem. I'm considering
making an IOP device driver, so that it can be examined more easily.
> I heard someone talking about problems in the SMMU which were fixed by
> increasing the DMA mask. This lets me believe that the DMA mask is
> something which is required by SMMU and therefore 32 Bit should be
> correct. But when a hardware designer tries to add an IOMMU to the PS2,
> there would be at least 3 different IOMMUs needed, because we have 3
> different buses (for EE, IOP and GS).
I'm not sure what you mean with SMMU, since this is not ARM hardware? Also,
the Graphics Synthesizer (GS) is a serial interface and not really as bus,
isn't it?
> The original approach for USB OHCI in Linux 2.4 was that IOP memory is
> handled as PCI memory. I.e. the driver was "thinking" that it allocates
> PCI memory, but indeed there was IOP memory allocated. All DMA ops where
> implemented in the PCI ops and so it was just working like a PCI card with
> device local memory.
Is PCI a good or valid model for the IOP?
Fredrik
Hello Fredrik,
> Gesendet: Samstag, 07. Juli 2018 um 08:32 Uhr
> Von: "Fredrik Noring" <[email protected]>
> An: "Jürgen Urban" <[email protected]>, "Robin Murphy" <[email protected]>
> Cc: "Christoph Hellwig" <[email protected]>, [email protected], [email protected], "Maciej W. Rozycki" <[email protected]>
> Betreff: Re: [PATCH] dma-mapping: Relax warnings for per-device areas
>
> Hi Jürgen, Robin,
>
> > Don't forget that the SIF DMA packets are limited and the kernel will
> > block/reschedule when it is out of SIF DMA packets. The allocation is
> > implemented inside the SBIOS. You may easily get a deadlock or a livelock
> > when you just let it run without thinking about the design. When you use
> > the old CDVD driver on IOP, the RPC code inside SBIOS tries to simulate
> > the interface like the new CDVD driver. The problem is that this is done
> > by a busy loop waiting for a free SIF DMA packet. This would block the
> > complete Linux kernel for an unknown time.
> >
> > As I understand you, you wanted to move the SBIOS code inside the Linux
> > kernel. I am not sure whether you already have done it. When you do this,
> > it is easier to fix the CDVD problem, but you need to think about booting
> > using the official RTE disc from Sony for Linux, because it loads
> > different modules and a different SBIOS. As this is the official way to
> > start Linux on the PS2 which is supported by Sony, we should also support
> > this in the official Linux kernel. Kernelloader can partially simulate it,
> > but you need the files from the RTE disc.
>
> The kernel no longer needs or uses the SBIOS, partly due to the issues
> with having binary blobs of code that do kernel services. SBIOS memory is
> reclaimed, so the SBIOS does not even exist when the kernel is running.
>
> DMA is therefore only limited by the hardware design, which supports both
> multiple simultaneously interconnected DMA controllers via memory or FIFOs,
> and chained (scatter-gather) transfers.
>
> Robin, does the kernel DMA subsystem support interconnected DMA controllers?
> That involves arbitration of hardware FIFO resources (for example the SIF).
>
> The Kernelloader boot program is not needed either, for any service, because
> the IOP is reset and initialised by the kernel itself. Booting the kernel is
> much faster and reliable without using the Kernelloader which frequently
> crashes or refuses to load IOP modules.
>
> The Kernelloader can still be used, if one wishes, but it's optional and not
> a requirement.
>
> > At least on some models I think you can desolder the RAM and replace it by
> > a larger memory up to 4GB, because the 1394 Lead Vehicle Manual lists a
> > feature:
> >
> > "Hardware generated response to received read or write requests in a
> > designated 4GB address range without CPU involvment."
> >
> > The 1394 Lead Vehicle is not used in the PS2, but it is very similar to
> > the IOP and it is the only manual we have about IOP. So I think the DMA
> > mask for the device must be at least 32 Bits, because the device is able
> > to access full 32 Bit. The EE where Linux is running may only be able to
> > access a part of it directly. I think SIF DMA is always able to access it
> > completely, as this is an official feature which is documented. The
> > mapping at 0x1c000000-0x1c200000 seems just to be good luck, because it is
> > not documented. As this is no official interface Sony is able to remove
> > this mapping at any time in a new model. I don't know where the border of
> > the mapping is, but in my experiements I have seen some hints that it can
> > be different depending on which hardware or software is used. It looks
> > like the more stuff is integrated into one single chip, the lower is the
> > border, because I have seen strange behaviour and exceptions when
> > accessing this memory on newer PS2 model. I limited the memory to 256KB
> > for USB OHCI because of this strange behaviour on some models, but I
> > wasn't able to figure out what was the real cause of the problem. I just
> > recognized that it was stable with the 256KB limit.
>
> Perhaps we need to invent a memory map zone within the IOP. I hope that we
> can make full use of the DMA hardware, because DMA is by a wide margin the
> most efficient kind of transfer.
>
> > So the question is: What is the purpose of the DMA mask in Linux? Is it
> > the area which can be accessed by the device? Or is it the area which can
> > be accessed by the CPU? For the device it is 32 Bit. For the CPU it
> > depends on the software and hardware and can be 0, because nothing may be
> > shared with the CPU.
>
> That's a good question. The DMA mapping updates that cause regressions need
> some kind of mask, but I agree, it's unclear what that mask actually means.
> Especially considering that the kernel cannot allocate normal memory for IOP
> DMA anyway, so what is the purpose of the mask then?
>
> > Even with DMA mask 0, the SIF DMA is still able to access the full 32 Bit.
> > Each memory access by the OHCI driver can't be done directly and needs
> > first to be transferred to IOP memory via SIF DMA before it can be
> > accessed by OHCI DMA. This is what you called linked DMA transfer.
>
> Right. So the IOP has DMA controllers that are also capable of simultaneously
> interconnected (linked) transfers such as device<->memory<->SIF? That would
> be very fortunate. A slight complication is that the SIF eventually needs
> arbitration to support simultaneous transfers for multiple devices such as
> the OHIC, ATA, iLink, Ethernet, etc.
>
> Are you aware of any documentation describing the IOP DMA controllers?
"EE's User Manual" describe the EE side and the IOP side is basically the same.
As the IOP is very similar to the PS1, many stuff of the PS1 documentation is matching. The main difference is the graphic as the graphic is emulated by the EE in PS1 mode of the PS2.
http://hwdocs.webs.com/ps1
> > As far as I remember the USB sub-system and the OHCI driver was not
> > written to handle memory which can't be written by the CPU at all. So I
> > assume that you first need to allocate some temporary memory which is used
> > to copy the data to or from the IOP DMA memory.
>
> Exactly, that OHCI design is still used. Also important, we need to
> investigate why OHCI interrupts sometimes are lost. Do you have any idea?
>
> > Then I think you can increase the 256KB limit without getting an unstable
> > system.
>
> I would like to learn more about the source of this problem. I'm considering
> making an IOP device driver, so that it can be examined more easily.
>
> > I heard someone talking about problems in the SMMU which were fixed by
> > increasing the DMA mask. This lets me believe that the DMA mask is
> > something which is required by SMMU and therefore 32 Bit should be
> > correct. But when a hardware designer tries to add an IOMMU to the PS2,
> > there would be at least 3 different IOMMUs needed, because we have 3
> > different buses (for EE, IOP and GS).
>
> I'm not sure what you mean with SMMU, since this is not ARM hardware?
It seems that the DMA mask was introduced for the SMMU. I.e. we need to define the DMA mask in a way how it would be done when a SMMU would have been used in the PS2.
> Also, the Graphics Synthesizer (GS) is a serial interface and not really as bus,
> isn't it?
You can't directly access the GS memory. There is always a DMA transfer needed. This means that there must be some bus behind the serial interface.
You can see this also in the document "Sony's Emotionally Charged Chip" at:
http://hwdocs.webs.com/ps2
PCIe is also just a serial interface, but we call it PCI bus. The advantage of PCI is that it is transparent for the CPU and that you are not forced to use DMA.
> > The original approach for USB OHCI in Linux 2.4 was that IOP memory is
> > handled as PCI memory. I.e. the driver was "thinking" that it allocates
> > PCI memory, but indeed there was IOP memory allocated. All DMA ops where
> > implemented in the PCI ops and so it was just working like a PCI card with
> > device local memory.
>
> Is PCI a good or valid model for the IOP?
In the new kernel PCI is declared as bus and I think that IOP should also be declared as a bus. Same can be done for GS. As the kernel has generic code for handling a bus, this can help getting stuff working like the DMA chain. As in some cases there are different paths from the source bus to the destination bus, the kernel may have already some mechanism implemented to do load balancing of the bus. This would help to improve the graphic performance.
Best regards
Jürgen Urban
Hi Christoph, Robin,
On Thu, Jul 05, 2018 at 09:36:13PM +0200, Christoph Hellwig wrote:
> > - BUG_ON(!ops);
> > - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> > -
> > if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
> > return cpu_addr;
> >
> > + BUG_ON(!ops);
> > + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
>
> I think doing dma on a device without ops is completely broken no matter
> what you think of it, so I very much disagree with that part of the change.
>
> Also while I don't think not having a dma mask is a good idea even for
> a driver purely using dma coherent pools. If the pools really are on
> the device itself I can see why it might not matter, but for the case
> commonly used on some ARM SOCs where we just reserve memory for certain
> devices from a system pool it very much does matter.
>
> There really is no good excuse to not set a coherent mask in the drivers.
Here are three other regressions related to the coherent mask WARN_ON_ONCE:
WARNING: CPU: 0 PID: 382 at ./include/linux/dma-mapping.h:516 .dma_pool_alloc+0xb8/0x238
Modules linked in: ehci_hcd(+) usbcore usb_common
CPU: 0 PID: 382 Comm: modprobe Not tainted 4.18.0-rc3-00022-g06ddb9e4cfce #52
NIP: c00000000018f540 LR: c00000000018f4f4 CTR: c00000000018f488
REGS: c00000000d536f20 TRAP: 0700 Not tainted (4.18.0-rc3-00022-g06ddb9e4cfce)
MSR: 8000000000028032 <SF,EE,IR,DR,RI> CR: 24024242 XER: 20000000
IRQMASK: 0
GPR00: c00000000018f4f4 c00000000d5371a0 c000000000a81e00 c00000000d52a858
GPR04: 0000000000001000 c00000000d34c318 0000000000000001 c00000000e517290
GPR08: 000000000dbbb000 c0000000004d5b00 0000000000000000 d00000000027f6f0
GPR12: c00000000018f488 c000000000b0b000 c000000000636bd0 0000000000000000
GPR16: c000000000680ab0 0000000000000100 0000000000000000 c000000000a11848
GPR20: 00000000006000c0 c0000000004d8210 c00000000d572400 d000000000281850
GPR24: c00000000d5372c0 c00000000d39a610 0000000000000000 00000000006000c0
GPR28: c00000000d39a600 006000c0006000c0 c00000000d39a600 c00000000d34c300
NIP [c00000000018f540] .dma_pool_alloc+0xb8/0x238
LR [c00000000018f4f4] .dma_pool_alloc+0x6c/0x238
Call Trace:
[c00000000d5371a0] [c00000000018f4f4] .dma_pool_alloc+0x6c/0x238 (unreliable)
[c00000000d537250] [d0000000002765ec] .ehci_qh_alloc+0x4c/0xd4 [ehci_hcd]
[c00000000d5372f0] [d00000000027b7f0] .ehci_setup+0x1f4/0x554 [ehci_hcd]
[c00000000d537380] [d00000000027bb7c] .ps3_ehci_hc_reset+0x2c/0x6c [ehci_hcd]
[c00000000d537400] [d000000000216740] .usb_add_hcd+0x31c/0x82c [usbcore]
[c00000000d5374d0] [d00000000027b1dc] .ps3_ehci_probe+0x124/0x1d8 [ehci_hcd]
[c00000000d537570] [c00000000005b350] .ps3_system_bus_probe+0x64/0x80
[c00000000d5375e0] [c00000000033fa7c] .driver_probe_device+0x298/0x384
[c00000000d537680] [c00000000033fc14] .__driver_attach+0xac/0xf4
[c00000000d537710] [c00000000033d7c8] .bus_for_each_dev+0x84/0xb0
[c00000000d5377b0] [c00000000033f394] .driver_attach+0x24/0x38
[c00000000d537820] [c00000000033edb4] .bus_add_driver+0x1e4/0x224
[c00000000d5378c0] [c000000000340580] .driver_register+0xe0/0x124
[c00000000d537940] [c00000000005ba48] .ps3_system_bus_driver_register+0x20/0x34
[c00000000d5379b0] [d00000000027ecb4] .ehci_hcd_init+0x8c/0xbc [ehci_hcd]
[c00000000d537a30] [c00000000000beb0] .do_one_initcall+0xac/0x1c0
[c00000000d537b00] [c0000000000e06ec] .do_init_module+0x6c/0x1fc
[c00000000d537ba0] [c0000000000dfd18] .load_module+0x1fbc/0x2164
[c00000000d537d20] [c0000000000e0048] .__se_sys_finit_module+0x94/0xa4
[c00000000d537e30] [c00000000000a2a4] system_call+0x5c/0x70
Instruction dump:
e89e0038 39200000 2fa30000 419e0008 e92301f8 7d2a0074 794ad182 0b0a0000
419e0014 e9430208 2faa0000 409e0008 <0fe00000> e9290000 57a6045e 2fa90000
---[ end trace 9066c3fd23e9bf8f ]---
WARNING: CPU: 0 PID: 382 at ./include/linux/dma-mapping.h:516 .ehci_setup+0x294/0x554 [ehci_hcd]
Modules linked in: ehci_hcd(+) usbcore usb_common
CPU: 0 PID: 382 Comm: modprobe Tainted: G W 4.18.0-rc3-00022-g06ddb9e4cfce #52
NIP: d00000000027b890 LR: d00000000027b844 CTR: c00000000018f058
REGS: c00000000d537070 TRAP: 0700 Tainted: G W (4.18.0-rc3-00022-g06ddb9e4cfce)
MSR: 8000000000028032 <SF,EE,IR,DR,RI> CR: 28024242 XER: 00000000
IRQMASK: 0
GPR00: d00000000027b844 c00000000d5372f0 d000000000288a00 c00000000d52a858
GPR04: 0000000000000800 c00000000d3b8bc0 0000000000000000 c00000000d32e480
GPR08: 0000000000000000 c0000000004d5b00 0000000000000000 d00000000027f000
GPR12: c00000000018f058 c000000000b0b000 c000000000636bd0 0000000000000000
GPR16: c000000000680ab0 0000000000000100 0000000000000000 c000000000a11848
GPR20: 00000000006000c0 c0000000004d8210 c00000000d572400 d000000000281850
GPR24: d0000000002379b0 0000000000000031 d000000000237b00 d000000000237b30
GPR28: d000000000281b80 c00000000d3b8a70 0000000000000016 c00000000d3b8800
NIP [d00000000027b890] .ehci_setup+0x294/0x554 [ehci_hcd]
LR [d00000000027b844] .ehci_setup+0x248/0x554 [ehci_hcd]
Call Trace:
[c00000000d5372f0] [d00000000027b844] .ehci_setup+0x248/0x554 [ehci_hcd] (unreliable)
[c00000000d537380] [d00000000027bb7c] .ps3_ehci_hc_reset+0x2c/0x6c [ehci_hcd]
[c00000000d537400] [d000000000216740] .usb_add_hcd+0x31c/0x82c [usbcore]
[c00000000d5374d0] [d00000000027b1dc] .ps3_ehci_probe+0x124/0x1d8 [ehci_hcd]
[c00000000d537570] [c00000000005b350] .ps3_system_bus_probe+0x64/0x80
[c00000000d5375e0] [c00000000033fa7c] .driver_probe_device+0x298/0x384
[c00000000d537680] [c00000000033fc14] .__driver_attach+0xac/0xf4
[c00000000d537710] [c00000000033d7c8] .bus_for_each_dev+0x84/0xb0
[c00000000d5377b0] [c00000000033f394] .driver_attach+0x24/0x38
[c00000000d537820] [c00000000033edb4] .bus_add_driver+0x1e4/0x224
[c00000000d5378c0] [c000000000340580] .driver_register+0xe0/0x124
[c00000000d537940] [c00000000005ba48] .ps3_system_bus_driver_register+0x20/0x34
[c00000000d5379b0] [d00000000027ecb4] .ehci_hcd_init+0x8c/0xbc [ehci_hcd]
[c00000000d537a30] [c00000000000beb0] .do_one_initcall+0xac/0x1c0
[c00000000d537b00] [c0000000000e06ec] .do_init_module+0x6c/0x1fc
[c00000000d537ba0] [c0000000000dfd18] .load_module+0x1fbc/0x2164
[c00000000d537d20] [c0000000000e0048] .__se_sys_finit_module+0x94/0xa4
[c00000000d537e30] [c00000000000a2a4] system_call+0x5c/0x70
Instruction dump:
39200000 2fa30000 78841764 419e0008 e92301f8 7d2a0074 794ad182 0b0a0000
419e0014 e9430208 2faa0000 409e0008 <0fe00000> e9290000 2fa90000 409e006c
---[ end trace 9066c3fd23e9bf90 ]---
WARNING: CPU: 1 PID: 397 at ./include/linux/dma-mapping.h:516 .ohci_init+0x224/0x498 [ohci_hcd]
Modules linked in: ohci_hcd(+) ehci_hcd usbcore usb_common
CPU: 1 PID: 397 Comm: modprobe Tainted: G W 4.18.0-rc3-00022-g06ddb9e4cfce #52
NIP: d0000000002a6ac0 LR: d0000000002a6a80 CTR: c0000000000c6e40
REGS: c00000000d4c70e0 TRAP: 0700 Tainted: G W (4.18.0-rc3-00022-g06ddb9e4cfce)
MSR: 8000000000028032 <SF,EE,IR,DR,RI> CR: 28022222 XER: 20000000
IRQMASK: 0
GPR00: d0000000002a6a80 c00000000d4c7360 d0000000002b3100 c00000000d52a458
GPR04: d0000000002ab980 c00000000d8ee298 0000000000000000 0000000000000000
GPR08: ffffffffffffffff c0000000004d5b00 0000000000000000 d0000000002a8ff8
GPR12: c0000000000c6e40 c000000007fffc00 c000000000636bd0 0000000000000000
GPR16: c000000000680ab0 0000000000000100 0000000000000000 c000000000a11848
GPR20: 00000000006000c0 c0000000004d8210 c00000000d8dce80 d0000000002ac1d0
GPR24: d0000000002379b0 0000000000000033 d000000000237b00 d000000000237b30
GPR28: 0000000000000000 c00000000d8ee138 d0000000002ac500 c00000000d8ee270
NIP [d0000000002a6ac0] .ohci_init+0x224/0x498 [ohci_hcd]
LR [d0000000002a6a80] .ohci_init+0x1e4/0x498 [ohci_hcd]
Call Trace:
[c00000000d4c7360] [d0000000002a6a80] .ohci_init+0x1e4/0x498 [ohci_hcd] (unreliable)
[c00000000d4c73f0] [d000000000216740] .usb_add_hcd+0x31c/0x82c [usbcore]
[c00000000d4c74c0] [d0000000002a3650] .ps3_ohci_probe+0x120/0x1dc [ohci_hcd]
[c00000000d4c7560] [c00000000005b350] .ps3_system_bus_probe+0x64/0x80
[c00000000d4c75d0] [c00000000033fa7c] .driver_probe_device+0x298/0x384
[c00000000d4c7670] [c00000000033fc14] .__driver_attach+0xac/0xf4
[c00000000d4c7700] [c00000000033d7c8] .bus_for_each_dev+0x84/0xb0
[c00000000d4c77a0] [c00000000033f394] .driver_attach+0x24/0x38
[c00000000d4c7810] [c00000000033edb4] .bus_add_driver+0x1e4/0x224
[c00000000d4c78b0] [c000000000340580] .driver_register+0xe0/0x124
[c00000000d4c7930] [c00000000005ba48] .ps3_system_bus_driver_register+0x20/0x34
[c00000000d4c79a0] [d0000000002a8c80] .ohci_hcd_mod_init+0x8c/0xfc [ohci_hcd]
[c00000000d4c7a30] [c00000000000beb0] .do_one_initcall+0xac/0x1c0
[c00000000d4c7b00] [c0000000000e06ec] .do_init_module+0x6c/0x1fc
[c00000000d4c7ba0] [c0000000000dfd18] .load_module+0x1fbc/0x2164
[c00000000d4c7d20] [c0000000000e0048] .__se_sys_finit_module+0x94/0xa4
[c00000000d4c7e30] [c00000000000a2a4] system_call+0x5c/0x70
Instruction dump:
913f0438 39200000 2fa30000 419e0008 e92301f8 7d2a0074 794ad182 0b0a0000
419e0014 e9430208 2faa0000 409e0008 <0fe00000> e9290000 2fa90000 409e0138
---[ end trace 9066c3fd23e9bf91 ]---
Fredrik
On Sun, Jul 15, 2018 at 02:28:27PM +0200, Fredrik Noring wrote:
> Hi Christoph, Robin,
>
> On Thu, Jul 05, 2018 at 09:36:13PM +0200, Christoph Hellwig wrote:
> > > - BUG_ON(!ops);
> > > - WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> > > -
> > > if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
> > > return cpu_addr;
> > >
> > > + BUG_ON(!ops);
> > > + WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
> >
> > I think doing dma on a device without ops is completely broken no matter
> > what you think of it, so I very much disagree with that part of the change.
> >
> > Also while I don't think not having a dma mask is a good idea even for
> > a driver purely using dma coherent pools. If the pools really are on
> > the device itself I can see why it might not matter, but for the case
> > commonly used on some ARM SOCs where we just reserve memory for certain
> > devices from a system pool it very much does matter.
> >
> > There really is no good excuse to not set a coherent mask in the drivers.
>
> Here are three other regressions related to the coherent mask WARN_ON_ONCE:
They are a pretty strong indication that yes, you should really set
the coherent mask if you ever do coherent allocations..
Hi Christoph, Geoff,
[ CC-ing Geoff to give him an opportunity to chime in about the PS3 part. ]
> > Here are three other regressions related to the coherent mask WARN_ON_ONCE:
>
> They are a pretty strong indication that yes, you should really set
> the coherent mask if you ever do coherent allocations..
I'm unfortunately unfamiliar with the USB drivers for the PS3. Geoff, what
do you think about setting a coherent mask?
Christoph, what mask value would you suggest for the PS2 driver? Typical
DMA addresses 0-0x200000 are mapped to 0x1c000000-0x1c200000 and is memory
managed exclusively by the IOP. Robin indicated that DMA_BIT_MASK(20) or a
nonzero but useless value such as 1 are possibly candidates. It seems quite
unclear what the coherent mask actually means in this case, doesn't it?
Fredrik
Hi,
On 07/17/2018 09:33 AM, Fredrik Noring wrote:
>>> Here are three other regressions related to the coherent mask WARN_ON_ONCE:
>>
>> They are a pretty strong indication that yes, you should really set
>> the coherent mask if you ever do coherent allocations..
>
> I'm unfortunately unfamiliar with the USB drivers for the PS3. Geoff, what
> do you think about setting a coherent mask?
I guess we need to add a dma_set_coherent_mask() call in the driver
probe routines. I'll send out a patch.
-Geoff
On Wed, Jul 18, 2018 at 02:55:59PM -0700, Geoff Levand wrote:
> Hi,
>
> On 07/17/2018 09:33 AM, Fredrik Noring wrote:
> >>> Here are three other regressions related to the coherent mask WARN_ON_ONCE:
> >>
> >> They are a pretty strong indication that yes, you should really set
> >> the coherent mask if you ever do coherent allocations..
> >
> > I'm unfortunately unfamiliar with the USB drivers for the PS3. Geoff, what
> > do you think about setting a coherent mask?
>
> I guess we need to add a dma_set_coherent_mask() call in the driver
> probe routines. I'll send out a patch.
Yes, exactly!