From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1757784AbcC2RIq (ORCPT <rfc822;w@1wt.eu>);
	Tue, 29 Mar 2016 13:08:46 -0400
Received: from mail-wm0-f52.google.com ([74.125.82.52]:37056 "EHLO
	mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751831AbcC2RIo (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 29 Mar 2016 13:08:44 -0400
Subject: Re: [RFC v5 06/17] dma-reserved-iommu: iommu_get/put_single_reserved
To: Jean-Philippe Brucker <Jean-Philippe.Brucker@arm.com>
References: <1456856877-4817-1-git-send-email-eric.auger@linaro.org>
 <1456856877-4817-7-git-send-email-eric.auger@linaro.org>
 <20160310115253.GA14609@e106794-lin.cambridge.arm.com>
Cc: eric.auger@st.com, robin.murphy@arm.com, alex.williamson@redhat.com,
        will.deacon@arm.com, joro@8bytes.org, tglx@linutronix.de,
        jason@lakedaemon.net, marc.zyngier@arm.com,
        christoffer.dall@linaro.org, linux-arm-kernel@lists.infradead.org,
        kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, patches@linaro.org,
        Manish.Jaggi@caviumnetworks.com, linux-kernel@vger.kernel.org,
        iommu@lists.linux-foundation.org
From: Eric Auger <eric.auger@linaro.org>
Message-ID: <56FAB656.7030400@linaro.org>
Date: Tue, 29 Mar 2016 19:07:34 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160310115253.GA14609@e106794-lin.cambridge.arm.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jean-Philippe,
On 03/10/2016 12:52 PM, Jean-Philippe Brucker wrote:
> Hi Eric,
> 
> On Tue, Mar 01, 2016 at 06:27:46PM +0000, Eric Auger wrote:
>> [...]
>> +
>> +int iommu_get_single_reserved(struct iommu_domain *domain,
>> +			      phys_addr_t addr, int prot,
>> +			      dma_addr_t *iova)
>> +{
>> +	unsigned long order = __ffs(domain->ops->pgsize_bitmap);
>> +	size_t page_size = 1 << order;
>> +	phys_addr_t mask = page_size - 1;
>> +	phys_addr_t aligned_addr = addr & ~mask;
>> +	phys_addr_t offset  = addr - aligned_addr;
>> +	struct iommu_reserved_binding *b;
>> +	struct iova *p_iova;
>> +	struct iova_domain *iovad =
>> +		(struct iova_domain *)domain->reserved_iova_cookie;
>> +	int ret;
>> +
>> +	if (!iovad)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&domain->reserved_mutex);
> 
> I believe this function could get called from the chunk of __setup_irq
> that is executed atomically:
> 
>     * request_threaded_irq
>     * __setup_irq
>     * irq_startup
>     * irq_domain_activate_irq
>     * msi_domain_activate
>     * msi_compose
>     * iommu_get_single_reserved
> 
> If this is the case, we should probably use a spinlock to protect the
> iova_domain...
Please apologize for the delay, I was in vacation.
Thank you for spotting this flow. I will rework the locking.
> 
>> +
>> +	b = find_reserved_binding(domain, aligned_addr, page_size);
>> +	if (b) {
>> +		*iova = b->iova + offset;
>> +		kref_get(&b->kref);
>> +		ret = 0;
>> +		goto unlock;
>> +	}
>> +
>> +	/* there is no existing reserved iova for this pa */
>> +	p_iova = alloc_iova(iovad, 1, iovad->dma_32bit_pfn, true);
>> +	if (!p_iova) {
>> +		ret = -ENOMEM;
>> +		goto unlock;
>> +	}
>> +	*iova = p_iova->pfn_lo << order;
>> +
>> +	b = kzalloc(sizeof(*b), GFP_KERNEL);
> 
> ... and GFP_ATOMIC here.
OK

Thank you for your time!

Best Regards

Eric
> 
> Thanks,
> Jean-Philippe
> 
>> +	if (!b) {
>> +		ret = -ENOMEM;
>> +		goto free_iova_unlock;
>> +	}
>> +
>> +	ret = iommu_map(domain, *iova, aligned_addr, page_size, prot);
>> +	if (ret)
>> +		goto free_binding_iova_unlock;
>> +
>> +	kref_init(&b->kref);
>> +	kref_get(&b->kref);
>> +	b->domain = domain;
>> +	b->addr = aligned_addr;
>> +	b->iova = *iova;
>> +	b->size = page_size;
>> +
>> +	link_reserved_binding(domain, b);
>> +
>> +	*iova += offset;
>> +	goto unlock;
>> +
>> +free_binding_iova_unlock:
>> +	kfree(b);
>> +free_iova_unlock:
>> +	free_iova(iovad, *iova >> order);
>> +unlock:
>> +	mutex_unlock(&domain->reserved_mutex);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_get_single_reserved);