Hi Thomas,
On 4/25/2020 2:49 PM, Thomas Gleixner wrote:
Dave Jiang <dave.jiang@xxxxxxxxx> 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
