While the MSI ops do have a .msi_prepare() callback that is
responsible for setting up the relevant (usually per-device)
allocation, we don't have a callback reversing this setup.
For this purpose, let's a .msi_teardown() callback. This is
reliying on the msi_domain_info structure having a non-NULL
alloc_data field.
Nobody is populating this field yet, so there is no change
in behaviour yet.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
include/linux/msi.h | 10 ++++++++--
kernel/irq/msi.c | 12 ++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 86e42742fd0fb..0a44a2cba3105 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -420,6 +420,7 @@ struct msi_domain_info;
* @msi_init: Domain specific init function for MSI interrupts
* @msi_free: Domain specific function to free a MSI interrupts
* @msi_prepare: Prepare the allocation of the interrupts in the domain
+ * @msi_teardown: Reverse the effects of @msi_prepare
* @prepare_desc: Optional function to prepare the allocated MSI descriptor
* in the domain
* @set_desc: Set the msi descriptor for an interrupt
@@ -435,8 +436,9 @@ struct msi_domain_info;
* @get_hwirq, @msi_init and @msi_free are callbacks used by the underlying
* irqdomain.
*
- * @msi_check, @msi_prepare, @prepare_desc and @set_desc are callbacks used by the
- * msi_domain_alloc/free_irqs*() variants.
+ * @msi_check, @msi_prepare, @msi_teardown, @prepare_desc and
+ * @set_desc are callbacks used by the msi_domain_alloc/free_irqs*()
+ * variants.
*
* @domain_alloc_irqs, @domain_free_irqs can be used to override the
* default allocation/free functions (__msi_domain_alloc/free_irqs). This
@@ -458,6 +460,8 @@ struct msi_domain_ops {
int (*msi_prepare)(struct irq_domain *domain,
struct device *dev, int nvec,
msi_alloc_info_t *arg);
+ void (*msi_teardown)(struct irq_domain *domain,
+ msi_alloc_info_t *arg);
void (*prepare_desc)(struct irq_domain *domain, msi_alloc_info_t *arg,
struct msi_desc *desc);
void (*set_desc)(msi_alloc_info_t *arg,
@@ -486,6 +490,7 @@ struct msi_domain_ops {
* @handler: Optional: associated interrupt flow handler
* @handler_data: Optional: associated interrupt flow handler data
* @handler_name: Optional: associated interrupt flow handler name
+ * @alloc_data: Optional: associated interrupt allocation data
* @data: Optional: domain specific data
*/
struct msi_domain_info {
@@ -498,6 +503,7 @@ struct msi_domain_info {
irq_flow_handler_t handler;
void *handler_data;
const char *handler_name;
+ msi_alloc_info_t *alloc_data;
void *data;
};
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index c05ba7ca00faa..a65ccf19b15d9 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -796,6 +796,11 @@ static int msi_domain_ops_prepare(struct irq_domain *domain, struct device *dev,
return 0;
}
+static void msi_domain_ops_teardown(struct irq_domain *domain,
+ msi_alloc_info_t *arg)
+{
+}
+
static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
struct msi_desc *desc)
{
@@ -821,6 +826,7 @@ static struct msi_domain_ops msi_domain_ops_default = {
.get_hwirq = msi_domain_ops_get_hwirq,
.msi_init = msi_domain_ops_init,
.msi_prepare = msi_domain_ops_prepare,
+ .msi_teardown = msi_domain_ops_teardown,
.set_desc = msi_domain_ops_set_desc,
};
@@ -842,6 +848,8 @@ static void msi_domain_update_dom_ops(struct msi_domain_info *info)
ops->msi_init = msi_domain_ops_default.msi_init;
if (ops->msi_prepare == NULL)
ops->msi_prepare = msi_domain_ops_default.msi_prepare;
+ if (ops->msi_teardown == NULL)
+ ops->msi_teardown = msi_domain_ops_default.msi_teardown;
if (ops->set_desc == NULL)
ops->set_desc = msi_domain_ops_default.set_desc;
}
@@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid)
dev->msi.data->__domains[domid].domain = NULL;
info = domain->host_data;
+
+ if (info->alloc_data)
+ info->ops->msi_teardown(domain, info->alloc_data);
+
if (irq_domain_is_msi_device(domain))
fwnode = domain->fwnode;
irq_domain_remove(domain);
--
2.39.2On Sun, May 11 2025 at 17:35, Marc Zyngier wrote:
> While the MSI ops do have a .msi_prepare() callback that is
> responsible for setting up the relevant (usually per-device)
> allocation, we don't have a callback reversing this setup.
..., there is no callback reversing ...
> For this purpose, let's a .msi_teardown() callback. This is
'let's a ...' is not a sentence. Just say: add a .... calback.
> reliying on the msi_domain_info structure having a non-NULL
^^^^^ spell check is your friend.
> alloc_data field.
>
> Nobody is populating this field yet, so there is no change
No driver is ..
>
> +static void msi_domain_ops_teardown(struct irq_domain *domain,
> + msi_alloc_info_t *arg)
No line break required.
> +{
> +}
> +
> static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
> {
> @@ -821,6 +826,7 @@ static struct msi_domain_ops msi_domain_ops_default = {
> .get_hwirq = msi_domain_ops_get_hwirq,
> .msi_init = msi_domain_ops_init,
> .msi_prepare = msi_domain_ops_prepare,
> + .msi_teardown = msi_domain_ops_teardown,
> .set_desc = msi_domain_ops_set_desc,
> };
>
> @@ -842,6 +848,8 @@ static void msi_domain_update_dom_ops(struct msi_domain_info *info)
> ops->msi_init = msi_domain_ops_default.msi_init;
> if (ops->msi_prepare == NULL)
> ops->msi_prepare = msi_domain_ops_default.msi_prepare;
> + if (ops->msi_teardown == NULL)
> + ops->msi_teardown = msi_domain_ops_default.msi_teardown;
> if (ops->set_desc == NULL)
> ops->set_desc = msi_domain_ops_default.set_desc;
> }
> @@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid)
>
> dev->msi.data->__domains[domid].domain = NULL;
> info = domain->host_data;
> +
> + if (info->alloc_data)
> + info->ops->msi_teardown(domain, info->alloc_data);
Hmm, that's weird.
Why not call it unconditionally. The empty teardown() default callback
does not care about @arg being NULL. No?
Thanks,
tglx
On Mon, 12 May 2025 15:29:39 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, May 11 2025 at 17:35, Marc Zyngier wrote:
>
> > While the MSI ops do have a .msi_prepare() callback that is
> > responsible for setting up the relevant (usually per-device)
> > allocation, we don't have a callback reversing this setup.
>
> ..., there is no callback reversing ...
>
> > For this purpose, let's a .msi_teardown() callback. This is
>
> 'let's a ...' is not a sentence. Just say: add a .... calback.
>
> > reliying on the msi_domain_info structure having a non-NULL
>
> ^^^^^ spell check is your friend.
I rely on humans for that. But maybe I should ask someone to put these
recommendations into one of these AI bots, and generate the stuff
automatically. It will be devoid of any actual reasoning, but at least
it will have the "officially sanctioned" verbiage.
>
> > alloc_data field.
> >
> > Nobody is populating this field yet, so there is no change
>
> No driver is ..
No. There is definitely no driver populating this, nor there will ever
be. That's 100% MSI infrastructure.
>
> >
> > +static void msi_domain_ops_teardown(struct irq_domain *domain,
> > + msi_alloc_info_t *arg)
>
> No line break required.
You mean...
>
> > +{
> > +}
> > +
> > static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
> > struct msi_desc *desc)
... not like this?
> > {
> > @@ -821,6 +826,7 @@ static struct msi_domain_ops msi_domain_ops_default = {
> > .get_hwirq = msi_domain_ops_get_hwirq,
> > .msi_init = msi_domain_ops_init,
> > .msi_prepare = msi_domain_ops_prepare,
> > + .msi_teardown = msi_domain_ops_teardown,
> > .set_desc = msi_domain_ops_set_desc,
> > };
> >
> > @@ -842,6 +848,8 @@ static void msi_domain_update_dom_ops(struct msi_domain_info *info)
> > ops->msi_init = msi_domain_ops_default.msi_init;
> > if (ops->msi_prepare == NULL)
> > ops->msi_prepare = msi_domain_ops_default.msi_prepare;
> > + if (ops->msi_teardown == NULL)
> > + ops->msi_teardown = msi_domain_ops_default.msi_teardown;
> > if (ops->set_desc == NULL)
> > ops->set_desc = msi_domain_ops_default.set_desc;
> > }
> > @@ -1088,6 +1096,10 @@ void msi_remove_device_irq_domain(struct device *dev, unsigned int domid)
> >
> > dev->msi.data->__domains[domid].domain = NULL;
> > info = domain->host_data;
> > +
> > + if (info->alloc_data)
> > + info->ops->msi_teardown(domain, info->alloc_data);
>
> Hmm, that's weird.
>
> Why not call it unconditionally. The empty teardown() default callback
> does not care about @arg being NULL. No?
Because at this point, nothing populates that pointer. It is only
after patch 3 that this pointer is valid. After patch 2, we get a
non-default callback, which should never be presented with a NULL
allocation context.
And since I value keeping things bisectable, it has to happen in this
order.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Mon, May 12 2025 at 16:57, Marc Zyngier wrote:
> On Mon, 12 May 2025 15:29:39 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> > +static void msi_domain_ops_teardown(struct irq_domain *domain,
>> > + msi_alloc_info_t *arg)
>>
>> No line break required.
>
> You mean...
>
>>
>> > +{
>> > +}
>> > +
>> > static void msi_domain_ops_set_desc(msi_alloc_info_t *arg,
>> > struct msi_desc *desc)
>
> ... not like this?
Right. That was written before we went to 100 characters line length.
© 2016 - 2025 Red Hat, Inc.