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

[Thomas Gleixner]

On Thu, Mar 25 2021 at 17:23, Marc Zyngier wrote:
>> +/**
>> + * irq_set_auxdata - Set auxiliary data
>> + * @irq:	Interrupt to update
>> + * @which:	Selector which data to update
>> + * @auxval:	Auxiliary data value
>> + *
>> + * Function to update auxiliary data for an interrupt, e.g. to update data
>> + * which is stored in a shared register or data storage (e.g. IMS).
>> + */
>> +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val)
>
> This looks to me like a massively generalised version of
> irq_set_irqchip_state(), only without any defined semantics when it
> comes to the 'which' state, making it look like the irqchip version of
> an ioctl.
>
> We also have the irq_set_vcpu_affinity() callback that is used to
> perpetrate all sort of sins (and I have abused this interface more
> than I should admit it).
>
> Can we try and converge on a single interface that allows for
> "side-band state" to be communicated, with documented state?
>
>> +{
>> +	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?

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;
               };
        };

Thanks,

        tglx