Re: [PATCH v10 07/15] platform-msi: Introduce platform_device_msi_rewrite()

On Tue, Sep 08, 2026 at 09:40:49PM +0200, Thomas Gleixner wrote:

On Tue, Sep 08 2026 at 17:17, Pranjal Shrivastava wrote:

...

quoted

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>

Jason. You really should know better by now :(

Apologies for the confusion, Jason just directed me to avoid open coding
this in the iommu driver and have the irq core handle it instead. [1]
Jason had nothing to do with the specific implementation / design here,
that misintepretation / mess is mine :( 

I'm relatively less familiar with the MSI pieces, and gave it a go
(probably should've prefixed this patch with RFC). Sorry about that!

quoted

+/**
+ * platform_device_msi_rewrite - Rewrite the MSI config for a platform device
+ * @dev:		The device for which to rewrite interrupt
+ * @irq:		The interrupt number to be rewritten.
+ * @write_msi_msg:	Callback to write the interrupt message for @dev
+ *
+ * Rewrites the cached MSI message for a platform device.
+ *
+ * Note: Platform MSI does not automatically cache composed messages. The caller's
+ * @write_msi_msg callback is expected to cache the message (e.g. into desc->msg)
+ * during initial configuration so it can be rewritten on resume.
+ */
+void platform_device_msi_rewrite(struct device *dev, unsigned int irq,
+				 irq_write_msi_msg_t write_msi_msg)

Why is this a platform device specific function and why does this need to
hand in the write_msi_msg() callback, which is already known through the
interrupt descriptor and the top level interrupt chip?

I spent an awful lot of time and effort to get rid of these platform MSI
layering violations and now you start adding the same mess again.

Not going to happen.

Ack. I'll address the layering violations

quoted

+{
+	struct msi_desc *desc;
+	struct msi_msg msg;
+
+	if (!irq || !write_msi_msg)
+		return;

Oh well.

quoted

+	desc = irq_get_msi_desc(irq);
+	if (!desc) {
+		dev_err(dev, "Failed to get MSI descriptor for irq %u\n", irq);
+		return;
+	}

Doing this without having the underlying interrupt descriptor locked is
a recipe for an undebuggable disaster waiting to happen. It might be
"safe" in the context you are calling it but it's absolutely not safe in
general.

Ack. I was thinking about races but I assumed the descriptor shoudln't
change but that's a "happy" / unsafe assumption.

quoted

+	__get_cached_msi_msg(desc, &msg);
+	if (!msg.address_hi && !msg.address_lo) {
+		dev_warn(dev, "No cached MSI message found for irq %u\n", irq);

That's just wrong. A message with a zero address is valid, e.g. when an
interrupt is shut down. So if there is random crap after resume in the
message store and the interrupt is valid, but not requested, then the
cached message still has to be written even if it is zero.

So this want's to be a function in the MSI core code. Also this is not a
per interrupt problem it is obviously a per device domain problem.
Simply because the device provides the message store for all MSI interrupts
which originate from that same device and therefore _all_ MSI interrupts
are affected by that, no?

So this all can be solved at the device domain level without sprinkling
per interrupt invocations including conditionals all over the place.

Ack. I was wondering if the irq core should also cache the message for
platform MSIs like it's done for PCI ? Would that be a bad idea? Or is
it this way by design? (I'm having to cache the msg in the iommu driver
atm).

Something like the completely untested below should just work.

Thanks for sharing this! I'll give it a go.

Thanks,
Praan