Re: [PATCH RFC 05/15] ims-msi: Add mask/unmask routines

[Dey, Megha]

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