Re: [Patch V2 08/13] genirq: Set auxiliary data for an interrupt

[Marc Zyngier]

On Thu, 25 Mar 2021 18:59:48 +0000,
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> On Thu, Mar 25 2021 at 17:23, Marc Zyngier wrote:
> >> +{
> >> +	struct irq_desc *desc;
> >> +	struct irq_data *data;
> >> +	unsigned long flags;
> >> +	int res = -ENODEV;
> >> +
> >> +	desc = irq_get_desc_buslock(irq, &flags, 0);
> >> +	if (!desc)
> >> +		return -EINVAL;
> >> +
> >> +	for (data = &desc->irq_data; data; data = irqd_get_parent_data(data)) {
> >> +		if (data->chip->irq_set_auxdata) {
> >> +			res = data->chip->irq_set_auxdata(data, which, val);
> >
> > And this is where things can break: because you don't define what
> > 'which' is, you can end-up with two stacked layers clashing in their
> > interpretation of 'which', potentially doing the wrong thing.
> >
> > Short of having a global, cross architecture definition of all the
> > possible states, this is frankly dodgy.
> 
> My bad, I suggested this in the first place.
> 
> So what you suggest is to make 'which' an enum and have that in
> include/linux/whatever.h so we end up with unique identifiers accross
> architectures, irqdomains and whatever, right?

Exactly. As long as 'which' is unique and unambiguous, we can solve
the stacking issue (which is oddly starting to smell like the ghost of
the SVR3 STREAMS... /me runs ;-).

Once we have that, I can start killing the irq_set_vcpu_affinity()
abuse I have been guilty of over the past years. Even more, we could
kill irq_set_vcpu_affinity() altogether, because we have a generic way
of passing side-band information from a driver down to the IRQ stack.

> That makes a lot of sense.
> 
> Though that leaves the question of the data type for 'val'. While u64 is
> probably good enough for most stuff, anything which needs more than that
> is left out (again). union as arguments are horrible especially if you
> need the counterpart irq_get_auxdata() for which you need a pointer and
> then you can't do a forward declaration. Something like this might work
> though and avoid to make the pointer business unconditional:
> 
>         struct irq_auxdata {
>                union {
>         	     u64        val;
>                      struct foo *foo;
>                };
>         };

I guess that at some point, irq_get_auxdata() will become a
requirement so we might as well bite the bullet now, and the above
looks like a good start to me.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.