Messages in this thread | | | Subject | Re: [PATCH RFC v2 02/18] irq/dev-msi: Add support for a new DEV_MSI irq domain | From | "Dey, Megha" <> | Date | Wed, 22 Jul 2020 09:50:47 -0700 |
| |
Hi Jason,
On 7/21/2020 9:13 AM, Jason Gunthorpe wrote: > On Tue, Jul 21, 2020 at 09:02:28AM -0700, Dave Jiang wrote: >> From: Megha Dey <megha.dey@intel.com> >> >> Add support for the creation of a new DEV_MSI irq domain. It creates a >> new irq chip associated with the DEV_MSI domain and adds the necessary >> domain operations to it. >> >> Add a new config option DEV_MSI which must be enabled by any >> driver that wants to support device-specific message-signaled-interrupts >> outside of PCI-MSI(-X). >> >> Lastly, add device specific mask/unmask callbacks in addition to a write >> function to the platform_msi_ops. >> >> Reviewed-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Megha Dey <megha.dey@intel.com> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> arch/x86/include/asm/hw_irq.h | 5 ++ >> drivers/base/Kconfig | 7 +++ >> drivers/base/Makefile | 1 >> drivers/base/dev-msi.c | 95 +++++++++++++++++++++++++++++++++++++++++ >> drivers/base/platform-msi.c | 45 +++++++++++++------ >> drivers/base/platform-msi.h | 23 ++++++++++ >> include/linux/msi.h | 8 +++ >> 7 files changed, 168 insertions(+), 16 deletions(-) >> create mode 100644 drivers/base/dev-msi.c >> create mode 100644 drivers/base/platform-msi.h >> >> diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h >> index 74c12437401e..8ecd7570589d 100644 >> +++ b/arch/x86/include/asm/hw_irq.h >> @@ -61,6 +61,11 @@ struct irq_alloc_info { >> irq_hw_number_t msi_hwirq; >> }; >> #endif >> +#ifdef CONFIG_DEV_MSI >> + struct { >> + irq_hw_number_t hwirq; >> + }; >> +#endif > > Why is this in this patch? I didn't see an obvious place where it is > used?
Since I have introduced the DEV-MSI domain and related ops, this is required in the dev_msi_set_hwirq and dev_msi_set_desc in this patch.
>> >> +static void __platform_msi_desc_mask_unmask_irq(struct msi_desc *desc, u32 mask) >> +{ >> + const struct platform_msi_ops *ops; >> + >> + ops = desc->platform.msi_priv_data->ops; >> + if (!ops) >> + return; >> + >> + if (mask) { >> + if (ops->irq_mask) >> + ops->irq_mask(desc); >> + } else { >> + if (ops->irq_unmask) >> + ops->irq_unmask(desc); >> + } >> +} >> + >> +void platform_msi_mask_irq(struct irq_data *data) >> +{ >> + __platform_msi_desc_mask_unmask_irq(irq_data_get_msi_desc(data), 1); >> +} >> + >> +void platform_msi_unmask_irq(struct irq_data *data) >> +{ >> + __platform_msi_desc_mask_unmask_irq(irq_data_get_msi_desc(data), 0); >> +} > > This is a bit convoluted, just call the op directly: > > void platform_msi_unmask_irq(struct irq_data *data) > { > const struct platform_msi_ops *ops = desc->platform.msi_priv_data->ops; > > if (ops->irq_unmask) > ops->irq_unmask(desc); > } >
Sure, I will update this.
>> diff --git a/include/linux/msi.h b/include/linux/msi.h >> index 7f6a8eb51aca..1da97f905720 100644 >> +++ b/include/linux/msi.h >> @@ -323,9 +323,13 @@ enum { >> >> /* >> * platform_msi_ops - Callbacks for platform MSI ops >> + * @irq_mask: mask an interrupt source >> + * @irq_unmask: unmask an interrupt source >> * @write_msg: write message content >> */ >> struct platform_msi_ops { >> + unsigned int (*irq_mask)(struct msi_desc *desc); >> + unsigned int (*irq_unmask)(struct msi_desc *desc); > > Why do these functions return things if the only call site throws it > away?
Hmmm, fair enough, I will change it to void.
> > Jason >
| |