linux-kernel - Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0f67c2a847963ee13ad02762866c94bf@walle.cc>
Date:   Thu, 26 Aug 2021 14:25:13 +0200
From:   Michael Walle <michael@...le.cc>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     etnaviv@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org,
        "Lukas F . Hartmann" <lukas@...re.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Christian Gmeiner <christian.gmeiner@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask

Am 2021-08-26 14:19, schrieb Russell King (Oracle):
> On Thu, Aug 26, 2021 at 02:10:06PM +0200, Michael Walle wrote:
>> -	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40);
>> -	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>> +	/*
>> +	 * PTA and MTLB can have 40 bit base addresses, but
>> +	 * unfortunately, an entry in the MTLB can only point to a
>> +	 * 32 bit base address of a STLB. Moreover, to initialize the
>> +	 * MMU we need a command buffer with a 32 bit address because
>> +	 * without an MMU there is only an indentity mapping between
>> +	 * the internal 32 bit addresses and the bus addresses.
>> +	 *
>> +	 * To make things easy, we set the dma_coherent_mask to 32
>> +	 * bit to make sure we are allocating the command buffers and
>> +	 * TLBs in the lower 4 GiB address space.
>> +	 */
>> +	if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) ||
>> +	    dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) {
>> +		dev_dbg(&pdev->dev, "No suitable DMA available\n");
>> +		return -ENODEV;
>> +	}
> 
> This makes no sense. In the previous patch, you initialised
> pdev->dev.dma_mask ot point at the coherent mask, implying that
> it wasn't already set - for which dma_coerce_mask_and_coherent()
> should be used. Now you're just calling dma_set_mask(), which will
> fail if pdev->dev.dma_mask hasn't already been set to point at
> something.
> 
> If it's already been initialised to point at something, then you
> shouldn't be overwriting it in the driver, and you should've used
> dma_set_mask_and_coherent() in your previous patch.
> 
> Confused.

Mh, I see that moving these two lines was a bad idea. See commit
message in patch 2/3:
> Also move the dma mask assignemnts to probe() to keep all DMA related
> settings together.

The actual fix in patch 2/3 is the move of the of_dma_configure()
not the dma_mask assignments.

-michael