Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: hZsb/znyya6IWicCjbNKIdLvLOpNKUlYWedml7PyPj7E/16xRBnV2Wns/3PAfZYare+dDWVa7s
 nzW+qjMSmgZQ==
IronPort-SDR: 0TrHIBMO8krjniVOj8ku1aw5a1xa7KLZrorAxn5r61A2Mcmm+Pt0Hh/hJlGIe7ukIr2PHSH5HI
 z6N45QguWqlA==
Subject: Re: [PATCH RFC 05/15] ims-msi: Add mask/unmask routines
To:     Thomas Gleixner <tglx@linutronix.de>,
        Dave Jiang <dave.jiang@intel.com>, vkoul@kernel.org,
        maz@kernel.org, bhelgaas@google.com, rafael@kernel.org,
        gregkh@linuxfoundation.org, hpa@zytor.com,
        alex.williamson@redhat.com, jacob.jun.pan@intel.com,
        ashok.raj@intel.com, jgg@mellanox.com, yi.l.liu@intel.com,
        baolu.lu@intel.com, kevin.tian@intel.com, sanjay.k.kumar@intel.com,
        tony.luck@intel.com, jing.lin@intel.com, dan.j.williams@intel.com,
        kwankhede@nvidia.com, eric.auger@redhat.com, parav@mellanox.com
Cc:     dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
        x86@kernel.org, linux-pci@vger.kernel.org, kvm@vger.kernel.org
References: <158751095889.36773.6009825070990637468.stgit@djiang5-desk3.ch.intel.com>
 <158751205785.36773.16321096654677399376.stgit@djiang5-desk3.ch.intel.com>
 <87lfmjtevb.fsf@nanos.tec.linutronix.de>
From:   "Dey, Megha" <megha.dey@linux.intel.com>
Message-ID: <cc3a136c-5f1f-20fb-86bd-d22230e0a372@linux.intel.com>
Date:   Sun, 3 May 2020 17:16:18 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <87lfmjtevb.fsf@nanos.tec.linutronix.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>