Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Received-SPF: TempError (protection.outlook.com: error in processing during
 lookup of arm.com: DNS Timeout)
From:   Brian Starkey <Brian.Starkey@arm.com>
To:     John Stultz <john.stultz@linaro.org>
CC:     lkml <linux-kernel@vger.kernel.org>,
        Laura Abbott <labbott@redhat.com>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Pratik Patel <pratikp@codeaurora.org>,
        Vincent Donnefort <Vincent.Donnefort@arm.com>,
        Sudipto Paul <Sudipto.Paul@arm.com>,
        "Andrew F . Davis" <afd@ti.com>,
        Christoph Hellwig <hch@infradead.org>,
        Chenbo Feng <fengc@google.com>,
        Alistair Strachan <astrachan@google.com>,
        Hridya Valsaraju <hridya@google.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        nd <nd@arm.com>
Subject: Re: [RESEND][PATCH v8 4/5] dma-buf: heaps: Add CMA heap to dmabuf
 heaps
Thread-Topic: [RESEND][PATCH v8 4/5] dma-buf: heaps: Add CMA heap to dmabuf
 heaps
Thread-Index: AQHVZOOH7YpCGzy7L0qigIXTV5Du46c57ZCA
Date:   Mon, 23 Sep 2019 22:10:19 +0000
Message-ID: <20190923221013.ovwqdr34777cluj2@DESKTOP-E1NTVVP.localdomain>
References: <20190906184712.91980-1-john.stultz@linaro.org>
 <20190906184712.91980-5-john.stultz@linaro.org>
In-Reply-To: <20190906184712.91980-5-john.stultz@linaro.org>
Accept-Language: en-GB, en-US
Content-Language: en-US
user-agent: NeoMutt/20180716-849-147d51-dirty
Authentication-Results-Original: spf=none (sender IP is )
 smtp.mailfrom=Brian.Starkey@arm.com; 
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-publictraffictype: Email
X-MS-Office365-Filtering-Correlation-Id: 1dbd4e47-0fd3-4c83-48e7-08d74072d7a2
X-MS-Office365-Filtering-HT: Tenant
X-Microsoft-Antispam-Untrusted: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(5600167)(711020)(4605104)(1401327)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020);SRVR:AM6PR08MB3829;
X-MS-TrafficTypeDiagnostic: AM6PR08MB3829:|AM6PR08MB3829:|AM0PR08MB3761:
x-ms-exchange-transport-forked: True
X-Microsoft-Antispam-PRVS: <AM0PR08MB376161D5C8A3D7BEE60B897CF0850@AM0PR08MB3761.eurprd08.prod.outlook.com>
x-checkrecipientrouted: true
x-ms-oob-tlc-oobclassifiers: OLM:9508;OLM:9508;
x-forefront-prvs: 0169092318
X-Forefront-Antispam-Report-Untrusted: SFV:NSPM;SFS:(10009020)(4636009)(366004)(136003)(396003)(39850400004)(346002)(376002)(189003)(199004)(6506007)(26005)(7736002)(81166006)(186003)(52116002)(316002)(99286004)(64756008)(66476007)(66446008)(229853002)(66946007)(4326008)(25786009)(66556008)(446003)(9686003)(386003)(76176011)(6512007)(6246003)(102836004)(7416002)(58126008)(305945005)(476003)(6436002)(11346002)(5660300002)(2906002)(14454004)(71200400001)(71190400001)(54906003)(14444005)(6486002)(486006)(81156014)(256004)(44832011)(1076003)(6916009)(8676002)(6116002)(478600001)(8936002)(66066001)(86362001)(3846002);DIR:OUT;SFP:1101;SCL:1;SRVR:AM6PR08MB3829;H:AM6PR08MB3829.eurprd08.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1;
received-spf: None (protection.outlook.com: arm.com does not designate
 permitted sender hosts)
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam-Message-Info-Original: 0blQdPxk0ik0KEUsOjtxHziQ8W/GKiu7lwEK0cJpgtrVbwoeArZFd5dM4uLaSz64xd0vGm1BrwqcOiWJRF4Z/NhI60uwYkjxLIQjUkxYNQqrfKK2sDjOnF+pME45e65WLnZFvvU4Nrh72FzZSxhRtMFsjalMk9ezdYImXNYnh2mJ4buTCKkDfMoQ7+8EGM/Eady4uoXAaLWOjexEC7geTdRKm5vCI3msWgqvhg9ETKlcJA02wA5y0K9w+t6sargs1YSLjIH4iREX2Mr+y1AgMDSnFw5kKCMpDWVdM4Bj29pEA2Fylit3E3fnstDoARTkyBEuHKhpLU6stie6rmgPmHLNuMzuRRAGhW4xuEUnZDTDlb8Z6zybOzQvYHaNtubNP7Htn1bLkqa9bfarQJ9+hCaoJxjETRT0PhSbgOLq5uQ=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9484091C05350E49B0D3FF94BD7EBD2F@eurprd08.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR08MB3829
Original-Authentication-Results: spf=none (sender IP is )
 smtp.mailfrom=Brian.Starkey@arm.com; 
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped: DB5EUR03FT023.eop-EUR03.prod.protection.outlook.com
X-Forefront-Antispam-Report: CIP:63.35.35.123;IPV:CAL;SCL:-1;CTRY:IE;EFV:NLI;SFV:NSPM;SFS:(10009020)(4636009)(136003)(346002)(39860400002)(376002)(396003)(189003)(199004)(478600001)(356004)(76176011)(86362001)(26826003)(66066001)(23726003)(6116002)(3846002)(46406003)(22756006)(14444005)(5660300002)(70586007)(70206006)(1076003)(47776003)(76130400001)(186003)(336012)(486006)(126002)(54906003)(6486002)(102836004)(8676002)(63350400001)(11346002)(81156014)(14454004)(81166006)(2906002)(25786009)(6862004)(6246003)(9686003)(99286004)(8746002)(446003)(50466002)(4326008)(97756001)(8936002)(6512007)(316002)(58126008)(305945005)(6506007)(26005)(386003)(229853002)(476003)(7736002);DIR:OUT;SFP:1101;SCL:1;SRVR:AM0PR08MB3761;H:64aa7808-outbound-1.mta.getcheckrecipient.com;FPR:;SPF:TempError;LANG:en;PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com;MX:1;A:1;
X-MS-Office365-Filtering-Correlation-Id-Prvs: 53780cd3-2012-4cb5-8eac-08d74072d21d
X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(5600167)(710020)(711020)(4605104)(1401327)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020);SRVR:AM0PR08MB3761;
NoDisclaimer: True
X-Forefront-PRVS: 0169092318
X-Microsoft-Antispam-Message-Info: Y8nVz8qLPoqSff8uxbHNnxb/Wbe5URFfL2sMpde0EX9aIluhj0i8UQKuRwbsOnalA5TvxTYMwq7MqD5pK+Xl9NUAdgTB8pqsRrNup33B+BvUvydvZLlXRlhuYyFSsRVcozkENID8ymFNJUCAXst/ihJy+0/OZvlTAlW9rYfsmdRGmTFeQnk/S4+hl/Mu+G8Lnl11MklCwHNRrSMwV/Op8mTl0V4ANl2tXz4dXVcq3FRQlTqt4EcuiXMAci9qdm32xS3PmLJwunYY9F6GiWRI0kTicSNSvutH3e/QBYGhoA5+8wkeineXyk8Me/Zwyyx6TguovgaZ8h/71iablxcL4cr++/2Fy+zWl7lPKSGEKeC9+yW1TLz1rT7guxBpnMmc+9FZyvaliqB6NLqaMZboAgGTI93MBNXJ1KgoT8DvhfM=
X-OriginatorOrg: arm.com
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2019 22:10:28.3872
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1dbd4e47-0fd3-4c83-48e7-08d74072d7a2
X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR08MB3761
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi John,

I spotted one thing below which might be harmless, but best to check.

On Fri, Sep 06, 2019 at 06:47:11PM +0000, John Stultz wrote:
> This adds a CMA heap, which allows userspace to allocate
> a dma-buf of contiguous memory out of a CMA region.
>=20
> This code is an evolution of the Android ION implementation, so
> thanks to its original author and maintainters:
>   Benjamin Gaignard, Laura Abbott, and others!
>=20
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Pratik Patel <pratikp@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Vincent Donnefort <Vincent.Donnefort@arm.com>
> Cc: Sudipto Paul <Sudipto.Paul@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Chenbo Feng <fengc@google.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Switch allocate to return dmabuf fd
> * Simplify init code
> * Checkpatch fixups
> v3:
> * Switch to inline function for to_cma_heap()
> * Minor cleanups suggested by Brian
> * Fold in new registration style from Andrew
> * Folded in changes from Andrew to use simplified page list
>   from the heap helpers
> v4:
> * Use the fd_flags when creating dmabuf fd (Suggested by
>   Benjamin)
> * Use precalculated pagecount (Suggested by Andrew)
> v6:
> * Changed variable names to improve clarity, as suggested
>   by Brian
> v7:
> * Use newly lower-cased init_heap_helper_buffer helper
> * Use new dmabuf export helper
> v8:
> * Make struct dma_heap_ops const (Suggested by Christoph)
> * Condense dma_heap_buffer and heap_helper_buffer (suggested by
>   Christoph)
> * Checkpatch whitespace fixups
> ---

...

> +
> +/* dmabuf heap CMA operations functions */
> +static int cma_heap_allocate(struct dma_heap *heap,
> +			     unsigned long len,
> +			     unsigned long fd_flags,
> +			     unsigned long heap_flags)
> +{
> +	struct cma_heap *cma_heap =3D dma_heap_get_data(heap);
> +	struct heap_helper_buffer *helper_buffer;
> +	struct page *cma_pages;
> +	size_t size =3D PAGE_ALIGN(len);
> +	unsigned long nr_pages =3D size >> PAGE_SHIFT;
> +	unsigned long align =3D get_order(size);
> +	struct dma_buf *dmabuf;
> +	int ret =3D -ENOMEM;
> +	pgoff_t pg;
> +
> +	if (align > CONFIG_CMA_ALIGNMENT)
> +		align =3D CONFIG_CMA_ALIGNMENT;
> +
> +	helper_buffer =3D kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> +	if (!helper_buffer)
> +		return -ENOMEM;
> +
> +	init_heap_helper_buffer(helper_buffer, cma_heap_free);
> +	helper_buffer->flags =3D heap_flags;
> +	helper_buffer->heap =3D heap;
> +	helper_buffer->size =3D len;
> +
> +	cma_pages =3D cma_alloc(cma_heap->cma, nr_pages, align, false);
> +	if (!cma_pages)
> +		goto free_buf;
> +
> +	if (PageHighMem(cma_pages)) {
> +		unsigned long nr_clear_pages =3D nr_pages;
> +		struct page *page =3D cma_pages;
> +
> +		while (nr_clear_pages > 0) {
> +			void *vaddr =3D kmap_atomic(page);
> +
> +			memset(vaddr, 0, PAGE_SIZE);
> +			kunmap_atomic(vaddr);
> +			page++;
> +			nr_clear_pages--;
> +		}
> +	} else {
> +		memset(page_address(cma_pages), 0, size);
> +	}
> +
> +	helper_buffer->pagecount =3D nr_pages;
> +	helper_buffer->pages =3D kmalloc_array(helper_buffer->pagecount,
> +					     sizeof(*helper_buffer->pages),
> +					     GFP_KERNEL);
> +	if (!helper_buffer->pages) {
> +		ret =3D -ENOMEM;
> +		goto free_cma;
> +	}
> +
> +	for (pg =3D 0; pg < helper_buffer->pagecount; pg++) {
> +		helper_buffer->pages[pg] =3D &cma_pages[pg];
> +		if (!helper_buffer->pages[pg])

Is this ever really possible? If cma_pages is non-NULL (which you
check earlier), then only if the pointer arithmetic overflows right?

If it's just redundant, then you could remove it (and in that case add
my r-b). But maybe you meant to check something else?

Cheers,
-Brian