lkml.org 
[lkml]   [2016]   [Aug]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 19/19] iommu/dma: Add support for mapping MSIs
From
Date
On 24/08/16 09:16, Thomas Gleixner wrote:
> 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.

Yeah, on reflection it is needlessly hideous. I think we should take
this as a clear lesson that whenever you find yourself thinking "Man, I
wish I had Python's for...else construct here", you're doing it wrong ;)

> 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?

OK, I've turned map_msi_page into get_msi_page (returning a page) and
just hoisted the list lookup into that, which leads to knock-on
simplifications throughout and is _much_ nicer. I now can't imagine why
I didn't get that far in the first place - thanks for the reality check!

Robin.

>
> Thanks,
>
> tglx
>

\
 
 \ /
  Last update: 2016-09-17 09:57    [W:0.161 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site