From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754548AbcHXIWH (ORCPT ); Wed, 24 Aug 2016 04:22:07 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:58985 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750999AbcHXIUa (ORCPT ); Wed, 24 Aug 2016 04:20:30 -0400 Date: Wed, 24 Aug 2016 10:16:26 +0200 (CEST) From: Thomas Gleixner To: Robin Murphy cc: joro@8bytes.org, will.deacon@arm.com, iommu@lists.linux-foundation.org, devicetree@vger.kernel.org, lorenzo.pieralisi@arm.com, jean-philippe.brucker@arm.com, punit.agrawal@arm.com, thunder.leizhen@huawei.com, eric.auger@redhat.com, Jason Cooper , Marc Zyngier , linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 19/19] iommu/dma: Add support for mapping MSIs In-Reply-To: <4c901ff0f6355039de55b0bc0df283065f02efa1.1471975357.git.robin.murphy@arm.com> Message-ID: References: <4c901ff0f6355039de55b0bc0df283065f02efa1.1471975357.git.robin.murphy@arm.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 23 Aug 2016, Robin Murphy wrote: > + cookie = domain->iova_cookie; > + iovad = &cookie->iovad; > + > + spin_lock(&cookie->msi_lock); > + list_for_each_entry(msi_page, &cookie->msi_page_list, list) > + if (msi_page->phys_hi == msg->address_hi && > + msi_page->phys_lo - msg->address_lo < iovad->granule) > + goto unlock; > + > + ret = __iommu_dma_map_msi_page(dev, msg, domain, &msi_page); > +unlock: > + spin_unlock(&cookie->msi_lock); > + > + if (!ret) { > + msg->address_hi = upper_32_bits(msi_page->iova); > + msg->address_lo &= iova_mask(iovad); > + msg->address_lo += lower_32_bits(msi_page->iova); > + } else { > + /* > + * We're called from a void callback, so the best we can do is > + * 'fail' by filling the message with obviously bogus values. > + * Since we got this far due to an IOMMU being present, it's > + * not like the existing address would have worked anyway... > + */ > + msg->address_hi = ~0U; > + msg->address_lo = ~0U; > + msg->data = ~0U; > + } The above is really horrible to parse. I had to read it five times to understand the logic. static struct iommu_dma_msi_page * find_or_map_msi_page(struct iommu_dma_cookie *cookie, struct msi_msg *msg) { struct iova_domain *iovad = &cookie->iovad; struct iommu_dma_msi_page *page; list_for_each_entry(*page, &cookie->msi_page_list, list) { if (page->phys_hi == msg->address_hi && page->phys_lo - msg->address_lo < iovad->granule) return page; } /* * FIXME: __iommu_dma_map_msi_page() should return a page or NULL. * The integer return value is pretty pointless. If seperate error * codes are required that's what ERR_PTR() is for .... */ ret = __iommu_dma_map_msi_page(dev, msg, domain, &page); return ret ? ERR_PTR(ret) : page; } So now the code in iommu_dma_map_msi_msg() becomes: spin_lock(&cookie->msi_lock); msi_page = find_or_map_msi_page(cookie, msg); spin_unlock(&cookie->msi_lock); if (!IS_ERR_OR_NULL(msi_page)) { msg->address_hi = upper_32_bits(msi_page->iova); msg->address_lo &= iova_mask(iovad); msg->address_lo += lower_32_bits(msi_page->iova); } else { /* * We're called from a void callback, so the best we can do is * 'fail' by filling the message with obviously bogus values. * Since we got this far due to an IOMMU being present, it's * not like the existing address would have worked anyway... */ msg->address_hi = ~0U; msg->address_lo = ~0U; msg->data = ~0U; } Hmm? Thanks, tglx