[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cc3a136c-5f1f-20fb-86bd-d22230e0a372@linux.intel.com>
Date: Sun, 3 May 2020 17:16:18 -0700
From: "Dey, Megha" <megha.dey@...ux.intel.com>
To: Thomas Gleixner <tglx@...utronix.de>,
Dave Jiang <dave.jiang@...el.com>, vkoul@...nel.org,
maz@...nel.org, bhelgaas@...gle.com, rafael@...nel.org,
gregkh@...uxfoundation.org, hpa@...or.com,
alex.williamson@...hat.com, jacob.jun.pan@...el.com,
ashok.raj@...el.com, jgg@...lanox.com, yi.l.liu@...el.com,
baolu.lu@...el.com, kevin.tian@...el.com, sanjay.k.kumar@...el.com,
tony.luck@...el.com, jing.lin@...el.com, dan.j.williams@...el.com,
kwankhede@...dia.com, eric.auger@...hat.com, parav@...lanox.com
Cc: dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
x86@...nel.org, linux-pci@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH RFC 05/15] ims-msi: Add mask/unmask routines
Hi Thomas,
On 4/25/2020 2:49 PM, Thomas Gleixner wrote:
> Dave Jiang <dave.jiang@...el.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
>
Powered by blists - more mailing lists