linux-kernel - Re: [PATCH 1/1] PCI: layerscape-ep: set 64-bit 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: <c6f07289-2e69-fab4-32dc-46fe5e5713e7@wanadoo.fr>
Date:   Thu, 21 Sep 2023 22:04:31 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Frank Li <Frank.li@....com>
Cc:     Krzysztof Wilczyński <kw@...ux.com>,
        imx@...ts.linux.dev, Rob Herring <robh@...nel.org>,
        "open list:PCI DRIVER FOR FREESCALE LAYERSCAPE" 
        <linux-pci@...r.kernel.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Minghuan Lian <minghuan.Lian@....com>,
        "moderated list:PCI DRIVER FOR FREESCALE LAYERSCAPE" 
        <linux-arm-kernel@...ts.infradead.org>,
        Roy Zang <roy.zang@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "open list:PCI DRIVER FOR FREESCALE LAYERSCAPE" 
        <linuxppc-dev@...ts.ozlabs.org>, Mingkai Hu <mingkai.hu@....com>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 1/1] PCI: layerscape-ep: set 64-bit DMA mask

Le 21/09/2023 à 20:35, Frank Li a écrit :
> On Thu, Sep 21, 2023 at 07:59:51PM +0200, Christophe JAILLET wrote:
>> Le 21/09/2023 à 17:37, Frank Li a écrit :
>>> From: Guanhua Gao <guanhua.gao@....com>
>>>
>>> Set DMA mask and coherent DMA mask to enable 64-bit addressing.
>>>
>>> Signed-off-by: Guanhua Gao <guanhua.gao@....com>
>>> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@....com>
>>> Signed-off-by: Frank Li <Frank.Li@....com>
>>> ---
>>>    drivers/pci/controller/dwc/pci-layerscape-ep.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> index de4c1758a6c33..6fd0dea38a32c 100644
>>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> @@ -249,6 +249,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>>>    	pcie->big_endian = of_property_read_bool(dev->of_node, "big-endian");
>>> +	/* set 64-bit DMA mask and coherent DMA mask */
>>> +	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)))
>>> +		if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)))
>>
>> As stated in [1], dma_set_mask() with a 64-bit mask will never
>> fail if dev->dma_mask is non-NULL.
>>
>> So, if it fails, the 32 bits case will also fail for the same reason.
>> There is no need for the 2nd test.
>>
>>
>> See [1] for Christoph Hellwig comment about it.
> 
> I don't think it is true. the below is dma_set_mask()'s implementation

I'll try to recollect a more detailled explanation from Christoph.

I also checked all paths some times ago, and the conclusion was that if 
dma_set_mask(64) failed, dma_set_mask(32) would fail for the exact same 
reasons.

I'll try to find the corresponding mail and come back to you.

I don't thing that implementation details have changed since that times, 
so the conclusion should still be valid.

Adding Christoph in cc, if he wants to give another look at it, or if he 
beats me finding the 1 or 2 years old mails.

CJ

> 
> int dma_set_mask(struct device *dev, u64 mask)
> {
> 	/*
> 	 * Truncate the mask to the actually supported dma_addr_t width to
> 	 * avoid generating unsupportable addresses.
> 	 */
> 	mask = (dma_addr_t)mask;
> 
> 	if (!dev->dma_mask || !dma_supported(dev, mask))
> 				^^^^^^^
> 		return -EIO;
> 
> 	arch_dma_set_mask(dev, mask);
> 	*dev->dma_mask = mask;
> 	return 0;
> }
> 
> dma_supported() may return failiure.
> 
> static int dma_supported(struct device *dev, u64 mask)
> {
> 	const struct dma_map_ops *ops = get_dma_ops(dev);
> 
> 	/*
> 	 * ->dma_supported sets the bypass flag, so we must always call
> 	 * into the method here unless the device is truly direct mapped.
> 	 */
> 	if (!ops)
> 		return dma_direct_supported(dev, mask);
> 	if (!ops->dma_supported)
> 		return 1;
> 	return ops->dma_supported(dev, mask);
>                      ^^^^^^
> 			DMA driver or IOMMU driver may return failure.
> }
> 
>   
> Frank
> 
>>
>> CJ
>>
>>
>> [1]: https://lkml.org/lkml/2021/6/7/398
>>
>>> +			return -EIO;
>>> +
>>>    	platform_set_drvdata(pdev, pcie);
>>>    	ret = dw_pcie_ep_init(&pci->ep);
>>
>