Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

From: Christoph Hellwig <hch@lst.de>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Nathan Jones <nathanj439@gmail.com>,
	Vladimir Murzin <vladimir.murzin@arm.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Christoph Hellwig <hch@lst.de>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return
Date: Thu, 29 Nov 2018 17:26:27 +0100	[thread overview]
Message-ID: <20181129162627.GB27068@lst.de> (raw)
In-Reply-To: <20181129102259.GR30658@n2100.armlinux.org.uk>

On Thu, Nov 29, 2018 at 10:22:59AM +0000, Russell King - ARM Linux wrote:
> I don't think so - if we were to remove the call to
> dma_mmap_from_dev_coherent(), it reintroduces the bug.
> 
> Quite why we have dma_mmap_from_dev_coherent() returning a 0/1 and
> error code via pointer I'm really not sure.
> 
> 	ret = dma_mmap_from_dev_coherent(...);
> 	if (ret)
> 		return ret > 0 ? 0 : ret;
> 
> and have dma_mmap_from_dev_coherent() return -ve for errnos, 1 if
> mapped via the coherent pool mechanism, or 0 otherwise.
> 
> The down-side is that 'ret' would be zero for the follow-on code,
> which would need explicit initialisation - but at least it's then
> obvious what is going on.

The above would be better than the current calling conventions, which
are horrible.  But the magic positive error code also tends to lead
to subtle errors sometimes.  My preference for this pattern is something
like:

	bool mapped;

	ret = dma_mmap_from_dev_coherent(..., &mapped);
	if (ret || mapped)
		return ret;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel