Re: [PATCH 5/6] PCI MSI: Refactor interrupt masking code

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 10004 bytes --]

On Mon, 2009-02-23 at 12:28 -0500, Matthew Wilcox wrote:
> From: Matthew Wilcox <willy@linux.intel.com>
> 
> Since most of the callers already know whether they have an MSI or
> an MSI-X capability, split msi_set_mask_bits() into msi_mask_irq()
> and msix_mask_irq().  The only callers which don't (mask_msi_irq()
> and unmask_msi_irq()) can share code in msi_set_mask_bit().  This then
> becomes the only caller of msix_flush_writes(), so we can inline it.
> The flushing read can be to any address that belongs to the device,
> so we can eliminate the calculation too.
> 
> We can also get rid of maskbits_mask from struct msi_desc and simply
> recalculate it on the rare occasion that we need it.  The single-bit
> 'masked' element is replaced by a copy of the 32-bit 'masked' register,
> so this patch does not affect the size of msi_desc.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/pci/msi.c   |  157 +++++++++++++++++++++++++--------------------------
>  include/linux/msi.h |    5 +-
>  2 files changed, 79 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index fcde04d..41f18cb 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -105,17 +105,14 @@ static inline __attribute_const__ u32 msi_mask(unsigned x)
>  	return (1 << (1 << x)) - 1;
>  }
>  
> -static void msix_flush_writes(struct irq_desc *desc)
> +static inline __attribute_const__ u32 msi_capable_mask(u16 control)
>  {

/me wonders why this is __attribute_const__ and not __const like all the
other shorthands, but not your fault.

> @@ -127,34 +124,62 @@ static void msix_flush_writes(struct irq_desc *desc)
>   * Returns 1 if it succeeded in masking the interrupt and 0 if the device
>   * doesn't support MSI masking.
>   */
> -static int msi_set_mask_bits(struct irq_desc *desc, u32 mask, u32 flag)
> +static int msi_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>  {
> -	struct msi_desc *entry;
> +	u32 mask_bits = desc->masked;
>  
> -	entry = get_irq_desc_msi(desc);
> -	BUG_ON(!entry);
> -	if (entry->msi_attrib.is_msix) {
> -		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
> -			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> -		writel(flag, entry->mask_base + offset);
> -		readl(entry->mask_base + offset);
> -	} else {
> -		int pos;
> -		u32 mask_bits;
> +	if (!desc->msi_attrib.maskbit)
> +		return 0;
>  
> -		if (!entry->msi_attrib.maskbit)
> -			return 0;
> +	mask_bits &= ~mask;
> +	mask_bits |= flag;
> +	pci_write_config_dword(desc->dev, desc->mask_pos, mask_bits);
> +	desc->masked = mask_bits;
>  
> -		pos = entry->mask_pos;
> -		pci_read_config_dword(entry->dev, pos, &mask_bits);
> -		mask_bits &= ~mask;
> -		mask_bits |= flag & mask;
> -		pci_write_config_dword(entry->dev, pos, mask_bits);
> -	}
> -	entry->msi_attrib.masked = !!flag;
>  	return 1;
>  }
>  
> +/*
> + * This internal function does not flush PCI writes to the device.
> + * All users must ensure that they read from the device before either
> + * assuming that the device state is up to date, or returning out of this
> + * file.  This saves a few milliseconds when initialising devices with lots
> + * of MSI-X interrupts.
> + */
> +static void msix_mask_irq(struct msi_desc *desc, u32 flag)
> +{
> +	u32 mask_bits = desc->masked;
> +	unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
> +					PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> +	mask_bits &= ~1;
> +	mask_bits |= flag;
> +	writel(mask_bits, desc->mask_base + offset);
> +	desc->masked = mask_bits;
> +}
> +
> +static int msi_set_mask_bit(unsigned irq, u32 flag)
> +{
> +	struct msi_desc *desc = get_irq_msi(irq);
> +
> +	if (desc->msi_attrib.is_msix) {
> +		msix_mask_irq(desc, flag);
> +		readl(desc->mask_base);		/* Flush write to device */
> +		return 1;
> +	} else {
> +		return msi_mask_irq(desc, 1, flag);
> +	}
> +}
> +
> +void mask_msi_irq(unsigned int irq)
> +{
> +	msi_set_mask_bit(irq, 1);
> +}
> +
> +void unmask_msi_irq(unsigned int irq)
> +{
> +	msi_set_mask_bit(irq, 0);
> +}

I don't see why msi_set_mask_bit() or msi_mask_irq() need to return
anything, their return values are never used AFAICT.

> +
>  void read_msi_msg_desc(struct irq_desc *desc, struct msi_msg *msg)
>  {
>  	struct msi_desc *entry = get_irq_desc_msi(desc);
> @@ -230,22 +255,6 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
>  	write_msi_msg_desc(desc, msg);
>  }
>  
> -void mask_msi_irq(unsigned int irq)
> -{
> -	struct irq_desc *desc = irq_to_desc(irq);
> -
> -	msi_set_mask_bits(desc, 1, 1);
> -	msix_flush_writes(desc);
> -}
> -
> -void unmask_msi_irq(unsigned int irq)
> -{
> -	struct irq_desc *desc = irq_to_desc(irq);
> -
> -	msi_set_mask_bits(desc, 1, 0);
> -	msix_flush_writes(desc);
> -}
> -
>  static int msi_free_irqs(struct pci_dev* dev);
>  
>  static struct msi_desc *alloc_msi_entry(struct pci_dev *dev)
> @@ -281,13 +290,9 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
>  	pci_intx_for_msi(dev, 0);
>  	msi_set_enable(dev, 0);
>  	write_msi_msg(dev->irq, &entry->msg);
> -	if (entry->msi_attrib.maskbit) {
> -		struct irq_desc *desc = irq_to_desc(dev->irq);
> -		msi_set_mask_bits(desc, entry->msi_attrib.maskbits_mask,
> -				  entry->msi_attrib.masked);
> -	}
>  
>  	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
> +	msi_mask_irq(entry, msi_capable_mask(control), entry->masked);
>  	control &= ~PCI_MSI_FLAGS_QSIZE;
>  	control |= PCI_MSI_FLAGS_ENABLE;
>  	pci_write_config_word(dev, pos + PCI_MSI_FLAGS, control);
> @@ -307,9 +312,8 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
>  	msix_set_enable(dev, 0);
>  
>  	list_for_each_entry(entry, &dev->msi_list, list) {
> -		struct irq_desc *desc = irq_to_desc(entry->irq);
>  		write_msi_msg(entry->irq, &entry->msg);
> -		msi_set_mask_bits(desc, 1, entry->msi_attrib.masked);
> +		msix_mask_irq(entry, entry->masked);
>  	}
>  
>  	BUG_ON(list_empty(&dev->msi_list));
> @@ -342,6 +346,7 @@ static int msi_capability_init(struct pci_dev *dev)
>  	struct msi_desc *entry;
>  	int pos, ret;
>  	u16 control;
> +	unsigned mask;
>  
>  	msi_set_enable(dev, 0);	/* Ensure msi is disabled as I set it up */
>  
> @@ -356,21 +361,15 @@ static int msi_capability_init(struct pci_dev *dev)
>  	entry->msi_attrib.is_64 = is_64bit_address(control);
>  	entry->msi_attrib.entry_nr = 0;
>  	entry->msi_attrib.maskbit = is_mask_bit_support(control);
> -	entry->msi_attrib.masked = 1;
>  	entry->msi_attrib.default_irq = dev->irq;	/* Save IOAPIC IRQ */
>  	entry->msi_attrib.pos = pos;
> -	if (entry->msi_attrib.maskbit) {
> -		unsigned int base, maskbits, temp;
> -
> -		base = msi_mask_bits_reg(pos, entry->msi_attrib.is_64);
> -		entry->mask_pos = base;
> -		/* All MSIs are unmasked by default, Mask them all */
> -		pci_read_config_dword(dev, base, &maskbits);
> -		temp = msi_mask((control & PCI_MSI_FLAGS_QMASK) >> 1);
> -		maskbits |= temp;
> -		pci_write_config_dword(dev, base, maskbits);
> -		entry->msi_attrib.maskbits_mask = temp;
> -	}
> +
> +	entry->mask_pos = msi_mask_bits_reg(pos, entry->msi_attrib.is_64);
> +	/* All MSIs are unmasked by default, Mask them all */
> +	pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
> +	mask = msi_capable_mask(control);
> +	msi_mask_irq(entry, mask, mask);

This looked a little weird at first, in that we're unconditionally doing
the mask - but we're not, msi_mask_irq() checks for us. I guess it's no
drama reading from mask_pos even if it's not implemented.

> @@ -435,11 +434,12 @@ static int msix_capability_init(struct pci_dev *dev,
>  		entry->msi_attrib.is_msix = 1;
>  		entry->msi_attrib.is_64 = 1;
>  		entry->msi_attrib.entry_nr = j;
> -		entry->msi_attrib.maskbit = 1;
> -		entry->msi_attrib.masked = 1;
>  		entry->msi_attrib.default_irq = dev->irq;
>  		entry->msi_attrib.pos = pos;
>  		entry->mask_base = base;
> +		entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE +
> +					PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> +		msix_mask_irq(entry, 1);

I was going to say "why bother with the readl". But checking the spec,
the rest of the bits are reserved and we mustn't muck with them.

> @@ -556,9 +556,11 @@ int pci_enable_msi(struct pci_dev* dev)
>  }
>  EXPORT_SYMBOL(pci_enable_msi);
>  
> -void pci_msi_shutdown(struct pci_dev* dev)
> +void pci_msi_shutdown(struct pci_dev *dev)
>  {
> -	struct msi_desc *entry;
> +	struct msi_desc *desc;
> +	u32 mask;
> +	u16 ctrl;
>  
>  	if (!pci_msi_enable || !dev || !dev->msi_enabled)
>  		return;
> @@ -568,18 +570,13 @@ void pci_msi_shutdown(struct pci_dev* dev)
>  	dev->msi_enabled = 0;
>  
>  	BUG_ON(list_empty(&dev->msi_list));
> -	entry = list_entry(dev->msi_list.next, struct msi_desc, list);
> -	/* Return the the pci reset with msi irqs unmasked */
> -	if (entry->msi_attrib.maskbit) {
> -		u32 mask = entry->msi_attrib.maskbits_mask;
> -		struct irq_desc *desc = irq_to_desc(dev->irq);
> -		msi_set_mask_bits(desc, mask, ~mask);
> -	}
> -	if (entry->msi_attrib.is_msix)
> -		return;

You loose this return case, but we should never have hit it AFAICS
because of the check of !dev->msi_enabled earlier - so I think it's ok.

> +	desc = list_first_entry(&dev->msi_list, struct msi_desc, list);
> +	pci_read_config_word(dev, desc->msi_attrib.pos + PCI_MSI_FLAGS, &ctrl);
> +	mask = msi_capable_mask(ctrl);
> +	msi_mask_irq(desc, mask, ~mask);
>  
>  	/* Restore dev->irq to its default pin-assertion irq */
> -	dev->irq = entry->msi_attrib.default_irq;
> +	dev->irq = desc->msi_attrib.default_irq;
>  }


cheers 

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]