Re: [Patch V2 08/13] genirq: Set auxiliary data for an interrupt
From: Marc Zyngier <maz@kernel.org>
Date: 2021-03-25 17:24:12
Also in:
linux-iommu, linux-pci, lkml
On Fri, 26 Feb 2021 20:11:12 +0000, Megha Dey [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Introduce a new function pointer in the irq_chip structure(irq_set_auxdata) which is responsible for updating data which is stored in a shared register or data storage. For example, the idxd driver uses the auxiliary data API to enable/set and disable PASID field that is in the IMS entry (introduced in a later patch) and that data are not typically present in MSI entry. Reviewed-by: Tony Luck <tony.luck@intel.com> Signed-off-by: Megha Dey <redacted> --- include/linux/interrupt.h | 2 ++ include/linux/irq.h | 4 ++++ kernel/irq/manage.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+)diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index 967e257..461ed1c 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h@@ -496,6 +496,8 @@ extern int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which, extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which, bool state); +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val); + #ifdef CONFIG_IRQ_FORCED_THREADING # ifdef CONFIG_PREEMPT_RT # define force_irqthreads (true)diff --git a/include/linux/irq.h b/include/linux/irq.h index 2efde6a..fc19f32 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h@@ -491,6 +491,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) * irq_request_resources * @irq_compose_msi_msg: optional to compose message content for MSI * @irq_write_msi_msg: optional to write message content for MSI + * @irq_set_auxdata: Optional function to update auxiliary data e.g. in + * shared registers * @irq_get_irqchip_state: return the internal state of an interrupt * @irq_set_irqchip_state: set the internal state of a interrupt * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine@@ -538,6 +540,8 @@ struct irq_chip { void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); + int (*irq_set_auxdata)(struct irq_data *data, unsigned int which, u64 auxval); + int (*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state); int (*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 85ede4e..68ff559 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c@@ -2860,3 +2860,35 @@ bool irq_check_status_bit(unsigned int irq, unsigned int bitmask) return res; } EXPORT_SYMBOL_GPL(irq_check_status_bit); + +/** + * 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. Thanks, M. -- Without deviation from the norm, progress is not possible.