Re: [PATCH v1 02/13] genirq/msi: Rename iommu_dma_compose_msi_msg() to msi_msg_set_addr()
From: Thomas Gleixner <hidden>
Date: 2025-02-13 12:11:52
Also in:
linux-iommu, linux-kselftest, linux-patches, lkml
On Sat, Feb 08 2025 at 01:02, Nicolin Chen wrote:
From: Jason Gunthorpe <jgg@nvidia.com> The new function is used to take in a u64 MSI address and store it in the
Which new function? The subject claims this is a rename. That's confusing at best.
msi_msg. If the iommu has provided an alternative address then that is replaced instead. All callers have a tidy u64 already so this also consolidates the repeated low/high code into a small helper.
Now I'm really confused. Something like:
genirq/msi: Refactor iommu_dma_compose_msi_msg()
The call sites of iommu_dma_compose_msi_msg() have the following
pattern:
1) Set the MSI message address to the non-translated address
2) Invoke iommu_dma_compose_msi_msg() to apply IOVA translation if
available.
Rework and rename iommu_dma_compose_msi_msg() to handles both
address set up variants in one consolidated helper.
Or something to that effect.
-/** - * iommu_dma_compose_msi_msg() - Apply translation to an MSI message - * @desc: MSI descriptor prepared by iommu_dma_prepare_msi() - * @msg: MSI message containing target physical address - */
Please keep and rework the documentation comment.
-static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc, - struct msi_msg *msg) +static inline void msi_msg_set_addr(struct msi_desc *desc, struct msi_msg *msg, + u64 msi_addr)
No line break required. You have 100 characters available.
quoted hunk ↗ jump to hunk
{ #ifdef CONFIG_IRQ_MSI_IOMMU if (desc->iommu_msi_page_shift) {@@ -314,11 +309,14 @@ static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc, << desc->iommu_msi_page_shift; msg->address_hi = upper_32_bits(msi_iova); - msg->address_lo = lower_32_bits(msi_iova) | - (msg->address_lo & - ((1 << desc->iommu_msi_page_shift) - 1)); + msg->address_lo = + lower_32_bits(msi_iova) | + (msi_addr & ((1 << desc->iommu_msi_page_shift) - 1));
Please avoid unrelated random formatting changes. Especially in this case this is even more non-sensical as you introduced the original formatting in the previous patch. So fix it up there and make it: msg->address_lo = lower_32_bits(msi_iova) | (msg->address_lo & ((1 << desc->iommu_msi_page_shift) - 1)); which is way more readable than this displaced variant above.
+ return; - iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg); + msg->data = its_get_event_id(d); + msi_msg_set_addr(irq_data_get_msi_desc(d), msg, + its_dev->its->get_msi_base(its_dev));
No line break required here and at the other places.
Thanks,
tglx