Re: DMA masks

Re: DMA masks

[Russell King - ARM Linux]

On Fri, Aug 09, 2013 at 11:12:36AM +0200, Krzysztof Hałasa wrote:
> Hi,
> 
> I'm trying to understand why the struct device contains a pointer to
> dma_mask and not the actual dma_mask:
> 
> struct device {
>         ...
> 
>         u64             *dma_mask;      /* dma mask (if dma'able device) */
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

"If dma-able device" gives a clue.  It is now the case that the DMA API
refuses DMA (via dma_supported) if the DMA mask pointer is NULL.

So, we're now in the situation where this conveys information, and merely
getting rid of the indirection loses information which may be (and
probably is) relied upon by drivers.

However, placing the storage for the dma_mask pointer into struct device
is something which I've recently discussed with Greg, and is something
he'd like to see happen, so I'll be adding that to my DMA masks patch
series in the very near future.

Re: DMA masks

[Krzysztof Hałasa]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

>> struct device {
>>         ...
>> 
>>         u64             *dma_mask;      /* dma mask (if dma'able device) */
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> "If dma-able device" gives a clue.  It is now the case that the DMA API
> refuses DMA (via dma_supported) if the DMA mask pointer is NULL.

But precisely the same effect can be obtained by setting the (actual)
dma_mask to 0 (most preferably, using DMA API's dma_set_mask()).
This is what coherent_dma_mask does.

Actually, when the (pointer) dma_mask is NULL, the behaviour isn't
exactly simple. I guess this all works because devices without DMA
capability don't try to use DMA.

There is also that old saying that both the DMA masks should default to
0xFFFFFFFF (it does on some archs).

Perhaps I'm wrong, but wouldn't the conversion to simple u64 remove
a lot of unneeded code, make all the archs/platforms behave more
consistently, and generally make the world a bit better?

arch/unicore32/include/asm/dma-mapping.h:
static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size)
{
        if (dev && dev->dma_mask)
                return addr + size - 1 <= *dev->dma_mask;

        return 1;
	^^^^^^^^^
}

arch/microblaze/include/asm/dma-mapping.h:
static inline unsigned long device_to_mask(struct device *dev)
{
        if (dev->dma_mask && *dev->dma_mask)
                return *dev->dma_mask;
        /* Assume devices without mask can take 32 bit addresses */
        return 0xfffffffful;
}

arch/hexagon/kernel/dma.c:
static int check_addr(const char *name, struct device *hwdev,
                      dma_addr_t bus, size_t size)
{
        if (hwdev && hwdev->dma_mask && !dma_capable(hwdev, bus, size)) {
                if (*hwdev->dma_mask >= DMA_BIT_MASK(32))
                        printk(KERN_ERR
                                "%s: overflow %Lx+%zu of device mask %Lx\n",
                                name, (long long)bus, size,
                                (long long)*hwdev->dma_mask);
                return 0;
        }
        return 1;
}

arch/powerpc/include/asm/dma-mapping.h:
static inline unsigned long device_to_mask(struct device *dev)
{
        if (dev->dma_mask && *dev->dma_mask)
                return *dev->dma_mask;
        /* Assume devices without mask can take 32 bit addresses */
        return 0xfffffffful;
}

etc.


Things like this:
	pdev1->dev.dma_mask = &pdev1->dev.coherent_dma_mask;
	pdev1->dev.coherent_dma_mask = DMA_BIT_MASK(32);
(which are all over the place) make me wonder what happens when some
code requires and tries to set different masks for normal and coherent
DMA.

> However, placing the storage for the dma_mask pointer into struct device
> is something which I've recently discussed with Greg, and is something
> he'd like to see happen, so I'll be adding that to my DMA masks patch
> series in the very near future.

Does this mean the dma_mask would remain a pointer, but it would point
(usually) to a variable in struct device?
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: DMA masks

[Russell King - ARM Linux]

On Fri, Aug 09, 2013 at 12:34:11PM +0200, Krzysztof Hałasa wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> >> struct device {
> >>         ...
> >> 
> >>         u64             *dma_mask;      /* dma mask (if dma'able device) */
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > "If dma-able device" gives a clue.  It is now the case that the DMA API
> > refuses DMA (via dma_supported) if the DMA mask pointer is NULL.
> 
> But precisely the same effect can be obtained by setting the (actual)
> dma_mask to 0 (most preferably, using DMA API's dma_set_mask()).
> This is what coherent_dma_mask does.

No.  If dma_mask is NULL, then dma_set_mask() will return -EIO no matter
what.  If dma_mask is non-NULL, dma_set_mask() will succeed if the mask
is supported by the hardware.  For example, on x86:

int dma_set_mask(struct device *dev, u64 mask)
{
        if (!dev->dma_mask || !dma_supported(dev, mask))
                return -EIO;

        *dev->dma_mask = mask;

        return 0;
}
EXPORT_SYMBOL(dma_set_mask);

and this is the same pattern we implement on ARM.  So, a valid dma_mask
pointer pointing at a variable holding zero can have a supported mask
set.

> Things like this:
> 	pdev1->dev.dma_mask = &pdev1->dev.coherent_dma_mask;
> 	pdev1->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> (which are all over the place) make me wonder what happens when some
> code requires and tries to set different masks for normal and coherent
> DMA.

Have you looked at my massive dma-masks patch series earlier this week?
Obviously you haven't...

The separate coherent and streaming masks are only required for a very
small subset of devices (rather systems - non-ARM I add).  For the
general case, especially on ARM where the above pattern seems to be
very prevalent, this does not apply; the two masks are always the same.
Many people took the above shortcut, and while not strictly correct, it
is a work-around for the shortcoming in the core device code.

What my patch set does is pull all that out into a central place so that
rather than having to modify a continually growing number of drivers when
we've decided how we're going fix this, we can fix it in one place and
switch everyone over at the same time.  That may not be needed now that
Greg has agreed to having stream_dma_mask in the struct device, but
even so I think removing the direct initialization from drivers is a good
idea.

> > However, placing the storage for the dma_mask pointer into struct device
> > is something which I've recently discussed with Greg, and is something
> > he'd like to see happen, so I'll be adding that to my DMA masks patch
> > series in the very near future.
> 
> Does this mean the dma_mask would remain a pointer, but it would point
> (usually) to a variable in struct device?

Yes.

Re: DMA masks

[Krzysztof Hałasa]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> No.  If dma_mask is NULL, then dma_set_mask() will return -EIO no matter
> what.  If dma_mask is non-NULL, dma_set_mask() will succeed if the mask
> is supported by the hardware.  For example, on x86:

> and this is the same pattern we implement on ARM.  So, a valid dma_mask
> pointer pointing at a variable holding zero can have a supported mask
> set.

Right. So is it a work around systems unable to provide DMA to devices,
whose drivers would try to use the DMA (not aware of the system
limitations)?

Do you know, by chance, of any particular case using this NULL pointer
trick to prevent (I suppose) a driver from doing DMA?

> Have you looked at my massive dma-masks patch series earlier this week?
> Obviously you haven't...

Right again, no time for basically anything here :-( Will look.

> The separate coherent and streaming masks are only required for a very
> small subset of devices (rather systems - non-ARM I add).  For the
> general case, especially on ARM where the above pattern seems to be
> very prevalent, this does not apply; the two masks are always the same.
> Many people took the above shortcut, and while not strictly correct, it
> is a work-around for the shortcoming in the core device code.

That's my impression, too. BTW I personally have a device (PCI card)
which have different masks (so it's also the case with devices). Old
story though.

> Greg has agreed to having stream_dma_mask in the struct device, but
> even so I think removing the direct initialization from drivers is a good
> idea.

Definitely. I'll take a look, Thanks.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: DMA masks

[James Bottomley]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 845 bytes --]

On Fri, 2013-08-09 at 11:12 +0200, Krzysztof Hałasa wrote:
> Hi,
> 
> I'm trying to understand why the struct device contains a pointer to
> dma_mask and not the actual dma_mask:

It's an anachronism.  The original reason was when this was introduced,
struct pci_device was the only device that actually had one.  On PA-RISC
we needed to walk the device tree by generic device and set the DMA mask
on generic devices, which meant we either had to special case the PCI
device, rearchitect the whole of the PCI device mask setting to use the
generic device, or cheat and make the generic device dma mask a pointer
to the PCI one.  I took the latter option because it's simpler.

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥