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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87h61plx64.ffs@tglx>
Date: Mon, 12 May 2025 16:29:39 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Marc Zyngier <maz@...nel.org>, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org
Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>, Sascha Bischoff
 <sascha.bischoff@....com>, Timothy Hayes <timothy.hayes@....com>
Subject: Re: [PATCH 1/4] genirq/msi: Add .msi_teardown() callback as the
 reverse of .msi_prepare()

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