Jan Beulich
2008-Nov-07 08:53 UTC
[Xen-devel] Is msix_flush_writes() really needed? And multi_msi_*() flawed?
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); \
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2008-Nov-10 08:38 UTC
[Xen-devel] Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?
>>> Grant Grundler <grundler@parisc-linux.org> 08.11.08 09:28 >>> >On Fri, Nov 07, 2008 at 08:53:49AM +0000, Jan Beulich wrote: >> @@ -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; > >Isn''t this the new code the same as: > maskbits |= (1U << multi_msi_capable(control)) - 1;No.>So the "& ~temp" got dropped...which looks correct to me. >"& ~temp" isn''t needed given only one bit could be set in temp. > >In any case, I''m feeling a bit dense since I''m not seeing the problem. >If multi_msi_capable(control) is 5, how is the shift of a >signed int ("1") undefined?multi_msi_capable() already shifts 1 by the value read out of the control word, so if the bit field read is 5, we''d shift 1 left by 32 here.>> @@ -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) > >I''ve no clue what the issue is here. >Can you point at a section in the PCI sec that defines this?What this apparently tries to do is set the number of vectors the device is allowed to actually use (i.e. the field named ''Multiple Message Enable'' in section 6.8.1.3 of the 3.0 copy I''m looking at). Actually, as I now realize this probably rather ought to be ilog2(num - 1) + 1, since the spec requires the requested value to be rounded up, not down. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel