Re: [PATCH v5 19/19] iommu/dma: Add support for mapping MSIs

[Thomas Gleixner]

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