Re: [PATCH 1/4] genirq/msi: Add .msi_teardown() callback as the reverse of .msi_prepare()

Thomas Gleixner
2025-05-12T14:29:42Z

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.

> 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