All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Jan Beulich" <jbeulich@novell.com>
To: linux-pci@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Subject: Is msix_flush_writes() really needed? And multi_msi_*() flawed?
Date: Fri, 07 Nov 2008 08:53:49 +0000	[thread overview]
Message-ID: <4914102D.76E4.0078.0@novell.com> (raw)

msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
and that function already does follow writel() by readl() in the MSI-X case.

Also, isn't the single use of multi_msi_capable() broken (in the event that
the Multiple Message Capable field was 5, the shift would be undefined,
on x86 in particular would yield 1 as the result, where 0 would be needed),
and the subsequent twiddling of temp needlessly complicated (subtracting
one should be sufficient here).

And isn't multi_msi_enable(), though unused (since msi_{en,dis}able() are
unused), broken altogether (shifting num right by 1 instead of taking the
binary log)?

In case so:

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 drivers/pci/msi.c |   30 ++----------------------------
 drivers/pci/msi.h |    2 +-
 2 files changed, 3 insertions(+), 29 deletions(-)

--- linux-2.6.28-rc3/drivers/pci/msi.c	2008-11-03 15:53:11.000000000 +0100
+++ 2.6.28-rc3-pci-msi-misc/drivers/pci/msi.c	2008-11-07 09:11:36.000000000 +0100
@@ -103,29 +103,6 @@ static void msix_set_enable(struct pci_d
 	}
 }
 
-static void msix_flush_writes(unsigned int irq)
-{
-	struct msi_desc *entry;
-
-	entry = get_irq_msi(irq);
-	BUG_ON(!entry || !entry->dev);
-	switch (entry->msi_attrib.type) {
-	case PCI_CAP_ID_MSI:
-		/* nothing to do */
-		break;
-	case PCI_CAP_ID_MSIX:
-	{
-		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
-			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
-		readl(entry->mask_base + offset);
-		break;
-	}
-	default:
-		BUG();
-		break;
-	}
-}
-
 /*
  * PCI 2.3 does not specify mask bits for each MSI interrupt.  Attempting to
  * mask all MSI interrupts by clearing the MSI enable bit does not work
@@ -255,13 +232,11 @@ void write_msi_msg(unsigned int irq, str
 void mask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bits(irq, 1, 1);
-	msix_flush_writes(irq);
 }
 
 void unmask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bits(irq, 1, 0);
-	msix_flush_writes(irq);
 }
 
 static int msi_free_irqs(struct pci_dev* dev);
@@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
 		pci_read_config_dword(dev,
 			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
 			&maskbits);
-		temp = (1 << multi_msi_capable(control));
-		temp = ((temp - 1) & ~temp);
-		maskbits |= temp;
+		temp = 1U << (multi_msi_capable(control) - 1);
+		maskbits |= (temp << 1) - 1;
 		pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);
 		entry->msi_attrib.maskbits_mask = temp;
 	}
--- linux-2.6.28-rc3/drivers/pci/msi.h	2007-02-04 19:44:54.000000000 +0100
+++ 2.6.28-rc3-pci-msi-misc/drivers/pci/msi.h	2008-11-07 09:13:09.000000000 +0100
@@ -23,7 +23,7 @@
 #define multi_msi_capable(control) \
 	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
 #define multi_msi_enable(control, num) \
-	control |= (((num >> 1) << 4) & PCI_MSI_FLAGS_QSIZE);
+	control |= ((ilog2(num) << 4) & PCI_MSI_FLAGS_QSIZE)
 #define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
 #define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
 #define msi_enable(control, num) multi_msi_enable(control, num); \

             reply	other threads:[~2008-11-07  8:53 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-11-07  8:53 Jan Beulich [this message]
2008-11-08  8:28 ` Is msix_flush_writes() really needed? And multi_msi_*() flawed? Grant Grundler
2008-11-08 14:10   ` Matthew Wilcox
2008-11-08 18:37     ` Grant Grundler
2008-11-09  2:13       ` Matthew Wilcox
2008-11-09  7:52         ` Grant Grundler
2008-11-10  8:38   ` Jan Beulich
2008-11-09 23:07 ` Michael Ellerman
2008-11-10  4:34   ` Grant Grundler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4914102D.76E4.0078.0@novell.com \
    --to=jbeulich@novell.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=xen-devel@lists.xensource.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.