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

[Marc Zyngier <maz@kernel.org>]

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.