| Message ID | 158751205785.36773.16321096654677399376.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add VFIO mediated device support and IMS support for the idxd driver. | expand |
Dave Jiang <dave.jiang@intel.com> writes: > > +static u32 __dev_ims_desc_mask_irq(struct msi_desc *desc, u32 flag) ...mask_irq()? This is doing both mask and unmask depending on the availability of the ops callbacks. > +{ > + u32 mask_bits = desc->platform.masked; > + const struct platform_msi_ops *ops; > + > + ops = desc->platform.msi_priv_data->ops; > + if (!ops) > + return 0; > + > + if (flag) { flag? Darn, this has a clear boolean meaning of mask or unmask and 'u32 flag' is the most natural and obvious self explaining expression for this, right? > + if (ops->irq_mask) > + mask_bits = ops->irq_mask(desc); > + } else { > + if (ops->irq_unmask) > + mask_bits = ops->irq_unmask(desc); > + } > + > + return mask_bits; What's mask_bits? This is about _ONE_ IMS interrupt. Can it have multiple mask bits and if so then the explanation which I decoded by crystal ball probably looks like this: Bit 0: Don't know whether it's masked Bit 1: Perhaps it's masked Bit 2: Probably it's masked Bit 3: Mostly masked ... Bit 31: Fully masked Or something like that. Makes a lot of sense in a XKCD cartoon at least. > +} > + > +/** > + * dev_ims_mask_irq - Generic irq chip callback to mask IMS interrupts > + * @data: pointer to irqdata associated to that interrupt > + */ > +static void dev_ims_mask_irq(struct irq_data *data) > +{ > + struct msi_desc *desc = irq_data_get_msi_desc(data); > + > + desc->platform.masked = __dev_ims_desc_mask_irq(desc, 1); The purpose of this masked information is? Thanks, tglx
Hi Thomas, On 4/25/2020 2:49 PM, Thomas Gleixner wrote: > Dave Jiang <dave.jiang@intel.com> writes: >> >> +static u32 __dev_ims_desc_mask_irq(struct msi_desc *desc, u32 flag) > > ...mask_irq()? This is doing both mask and unmask depending on the > availability of the ops callbacks. yes, should have called it __dev_ims_desc_mask_unmask_irq perhaps. > >> +{ >> + u32 mask_bits = desc->platform.masked; >> + const struct platform_msi_ops *ops; >> + >> + ops = desc->platform.msi_priv_data->ops; >> + if (!ops) >> + return 0; >> + >> + if (flag) { > > flag? Darn, this has a clear boolean meaning of mask or unmask and 'u32 > flag' is the most natural and obvious self explaining expression for > this, right? will change it a more meaningful name next time around .. > >> + if (ops->irq_mask) >> + mask_bits = ops->irq_mask(desc); >> + } else { >> + if (ops->irq_unmask) >> + mask_bits = ops->irq_unmask(desc); >> + } >> + >> + return mask_bits; > > What's mask_bits? This is about _ONE_ IMS interrupt. Can it have > multiple mask bits and if so then the explanation which I decoded by > crystal ball probably looks like this: > > Bit 0: Don't know whether it's masked > Bit 1: Perhaps it's masked > Bit 2: Probably it's masked > Bit 3: Mostly masked > ... > Bit 31: Fully masked > > Or something like that. Makes a lot of sense in a XKCD cartoon at least. > After a close look, we can simply do away with this mask_bits. Looks like a crystal ball will not be required next time around after all. >> +} >> + >> +/** >> + * dev_ims_mask_irq - Generic irq chip callback to mask IMS interrupts >> + * @data: pointer to irqdata associated to that interrupt >> + */ >> +static void dev_ims_mask_irq(struct irq_data *data) >> +{ >> + struct msi_desc *desc = irq_data_get_msi_desc(data); >> + >> + desc->platform.masked = __dev_ims_desc_mask_irq(desc, 1); > > The purpose of this masked information is? serves no purpose, borrowed this concept from the PCI-msi code but is just junk here. Will be removed next time around. > > Thanks, > > tglx >
diff --git a/drivers/base/ims-msi.c b/drivers/base/ims-msi.c index 738f6d153155..896a5a1b2252 100644 --- a/drivers/base/ims-msi.c +++ b/drivers/base/ims-msi.c @@ -7,11 +7,56 @@ * Author: Megha Dey <megha.dey@intel.com> */ +#include <linux/device.h> #include <linux/dmar.h> +#include <linux/export.h> #include <linux/irq.h> #include <linux/mdev.h> +#include <linux/msi.h> #include <linux/pci.h> +static u32 __dev_ims_desc_mask_irq(struct msi_desc *desc, u32 flag) +{ + u32 mask_bits = desc->platform.masked; + const struct platform_msi_ops *ops; + + ops = desc->platform.msi_priv_data->ops; + if (!ops) + return 0; + + if (flag) { + if (ops->irq_mask) + mask_bits = ops->irq_mask(desc); + } else { + if (ops->irq_unmask) + mask_bits = ops->irq_unmask(desc); + } + + return mask_bits; +} + +/** + * dev_ims_mask_irq - Generic irq chip callback to mask IMS interrupts + * @data: pointer to irqdata associated to that interrupt + */ +static void dev_ims_mask_irq(struct irq_data *data) +{ + struct msi_desc *desc = irq_data_get_msi_desc(data); + + desc->platform.masked = __dev_ims_desc_mask_irq(desc, 1); +} + +/** + * dev_msi_unmask_irq - Generic irq chip callback to unmask IMS interrupts + * @data: pointer to irqdata associated to that interrupt + */ +void dev_ims_unmask_irq(struct irq_data *data) +{ + struct msi_desc *desc = irq_data_get_msi_desc(data); + + desc->platform.masked = __dev_ims_desc_mask_irq(desc, 0); +} + /* * Determine if a dev is mdev or not. Return NULL if not mdev device. * Return mdev's parent dev if success. @@ -69,6 +114,8 @@ static struct irq_chip dev_ims_ir_controller = { .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, .flags = IRQCHIP_SKIP_SET_WAKE, .irq_write_msi_msg = platform_msi_write_msg, + .irq_unmask = dev_ims_unmask_irq, + .irq_mask = dev_ims_mask_irq, }; static struct msi_domain_info ims_ir_domain_info = { diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c index 59160e8cbfb1..6d8840db4a85 100644 --- a/drivers/base/platform-msi.c +++ b/drivers/base/platform-msi.c @@ -16,18 +16,6 @@ #define DEV_ID_SHIFT 21 #define MAX_DEV_MSIS (1 << (32 - DEV_ID_SHIFT)) -/* - * Internal data structure containing a (made up, but unique) devid - * and the callback to write the MSI message. - */ -struct platform_msi_priv_data { - struct device *dev; - void *host_data; - msi_alloc_info_t arg; - const struct platform_msi_ops *ops; - int devid; -}; - /* The devid allocator */ static DEFINE_IDA(platform_msi_devid_ida); diff --git a/include/linux/msi.h b/include/linux/msi.h index 80386468a7bc..8b5f24bf3c47 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -33,10 +33,12 @@ typedef void (*irq_write_msi_msg_t)(struct msi_desc *desc, * platform_msi_desc - Platform device specific msi descriptor data * @msi_priv_data: Pointer to platform private data * @msi_index: The index of the MSI descriptor for multi MSI + * @masked: mask bits */ struct platform_msi_desc { struct platform_msi_priv_data *msi_priv_data; u16 msi_index; + u32 masked; }; /** @@ -370,6 +372,18 @@ struct platform_msi_ops { irq_write_msi_msg_t write_msg; }; +/* + * Internal data structure containing a (made up, but unique) devid + * and the callback to write the MSI message. + */ +struct platform_msi_priv_data { + struct device *dev; + void *host_data; + msi_alloc_info_t arg; + const struct platform_msi_ops *ops; + int devid; +}; + int msi_domain_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force);