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
>
