Hi Thomas,
On 02/26/2016 07:19 PM, Thomas Gleixner wrote:
> On Fri, 26 Feb 2016, Eric Auger wrote:
>> +static int msi_map_doorbell(struct iommu_domain *d, struct msi_msg *msg)
>> +{
>> +#ifdef CONFIG_IOMMU_DMA_RESERVED
>> + dma_addr_t iova;
>> + phys_addr_t addr;
>> + int ret;
>> +
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> + addr = ((phys_addr_t)(msg->address_hi) << 32) | msg->address_lo;
>> +#else
>> + addr = msg->address_lo;
>> +#endif
>> +
>> + ret = iommu_get_single_reserved(d, addr, IOMMU_WRITE, &iova);
>> + if (!ret) {
>> + msg->address_lo = lower_32_bits(iova);
>> + msg->address_hi = upper_32_bits(iova);
>> + }
>> + return ret;
>> +#else
>> + return -ENODEV;
>> +#endif
>
> This is completely unreadable. Please make this in a way which is parseable.
> A few small inline functions do the trick.
OK I will rewrite this.
>
>> +static void msi_unmap_doorbell(struct iommu_domain *d, struct msi_msg *msg)
>> +{
>> +#ifdef CONFIG_IOMMU_DMA_RESERVED
>> + dma_addr_t iova;
>> +
>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> + iova = ((dma_addr_t)(msg->address_hi) << 32) | msg->address_lo;
>> +#else
>> + iova = msg->address_lo;
>> +#endif
>> +
>> + iommu_put_single_reserved(d, iova);
>> +#endif
>
> Ditto.
ok
>
>> +}
>> +
>> +#ifdef CONFIG_IOMMU_API
>> +static struct iommu_domain *
>> + irq_data_to_msi_mapping_domain(struct irq_data *irq_data)
>
> If you split lines, then the function name starts at the beginning of the line
> and not at some randome place.
ok
>
>> +{
>> +
>> + struct msi_desc *desc;
>> + struct device *dev;
>> + struct iommu_domain *d;
>> + int ret;
>
> Please order variables by descending length
ok
>
>> + desc = irq_data_get_msi_desc(irq_data);
>> + if (!desc)
>> + return NULL;
>> +
>> + dev = msi_desc_to_dev(desc);
>> +
>> + d = iommu_get_domain_for_dev(dev);
>> + if (!d)
>> + return NULL;
>> +
>> + ret = iommu_domain_get_attr(d, DOMAIN_ATTR_MSI_MAPPING, NULL);
>> + if (!ret)
>> + return d;
>> + else
>> + return NULL;
>
> Does anyone except you understand the purpose of the function? Comments have
> been invented for a reason.
ok I will comment on the role of those functions.
>
>> +}
>> +#else
>> +static inline iommu_domain *
>> + irq_data_to_msi_mapping_domain(struct irq_data *irq_data)
>> +{
>> + return NULL;
>> +}
>> +#endif /* CONFIG_IOMMU_API */
>> +
>> +static int msi_compose(struct irq_data *irq_data,
>> + struct msi_msg *msg, bool erase)
>> +{
>> + struct msi_msg old_msg;
>> + struct iommu_domain *d;
>> + int ret = 0;
>> +
>> + d = irq_data_to_msi_mapping_domain(irq_data);
>> + if (unlikely(d))
>> + get_cached_msi_msg(irq_data->irq, &old_msg);
>> +
>> + if (erase)
>> + memset(msg, 0, sizeof(*msg));
>> + else
>> + ret = irq_chip_compose_msi_msg(irq_data, msg);
>> +
>> + if (!d)
>> + goto out;
>> +
>> + /* handle (re-)mapping of MSI doorbell */
>> + if ((old_msg.address_lo != msg->address_lo) ||
>> + (old_msg.address_hi != msg->address_hi))
>> + msi_unmap_doorbell(d, &old_msg);
>> +
>> + if (!erase)
>> + WARN_ON(msi_map_doorbell(d, msg));
>> +
>> +out:
>> + return ret;
>> +}
>
> No, this is not the way we do this. You replace existing functionality by some
> new fangled thing. which behaves differently.
>
> This wants to be seperate patches, which first create a wrapper for
> irq_chip_compose_msi_msg() and then adds the new functionality to it including
> a proper explanation.
>
> I have no idea how the above is supposed to be the same as the existing code
> for the non iommu case.
Sure I will decompose things and provide more explanation.
Thank you for your time.
Best Regards
Eric
>
> Thanks,
>
> tglx
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
