Thanks for the review.
On Wed, Oct 01, 2008 at 02:29:28PM -0700, Randy Dunlap wrote:
> > This guide describes the basics of Message Signaled Interrupts (MSIs),
>
> good (Signaled). Please be consistent with that spelling...
Obviously this is a touchy issue being a difference between US spelling
and Commonwealth spelling. As you note further down, the PCI SIG spells
it the US way and pciutils spells it the Commonwealth way. So we're
caught in a cleft stick here.
I believe the original document used entirely US spellings, and I didn't
notice this when doing the rewrite. I suspect the right thing to do is
use US spellings throughout, including patching lspci. Durn it. Also.
> > 4. How to use MSIs
> >
> > PCI devices are initialised to use pin-based interrupts. The device
> > driver has to set up the device to use MSI or MSI-X. Not all machines
> > support MSIs correctly, and for those machines, the APIs described below
> > will simply fail and the device will continue to use pin-based interrupts.
> >
> > 4.1 Include kernel support for MSIs
> >
> > To support MSI or MSI-X, the kernel must be built with the CONFIG_PCI_MSI
> > option enabled. This option is only available on some architectures,
> > and it may depend on some other options also being set. For example,
> > on x86, you must also enable X86_UP_APIC or SMP in order to see the
> > CONFIG_PCI_MSI option.
>
> in 2.6.27-rc8, it needs both X86_LOCAL_APIC and X86_IO_APIC, but SMP will do that.
It keeps changing ... and it's complicated.
config PCI_MSI
depends on ARCH_SUPPORTS_MSI
(on x86):
config PCI
select ARCH_SUPPORTS_MSI if (X86_LOCAL_APIC && X86_IO_APIC)
config X86_LOCAL_APIC (*)
depends on X86_64 ||
(X86_32 &&
(X86_UP_APIC ||
(SMP && !X86_VOYAGER) ||
X86_GENERICARCH)
)
config X86_UP_APIC
bool "Local APIC support on uniprocessors"
depends on X86_32 && !SMP && !(X86_VOYAGER || X86_GENERICARCH)
config X86_IO_APIC (*)
depends on X86_64 ||
(X86_32 &&
(X86_UP_IOAPIC ||
(SMP && !X86_VOYAGER) ||
X86_GENERICARCH)
)
config X86_UP_IOAPIC
bool "IO-APIC support on uniprocessors"
depends on X86_UP_APIC
The config options marked with a (*) aren't user selectable.
So ... you'll always see MSI if you're configuring a kernel for X86_64,
SMP or X86_GENERICARCH. If you're configuring for a uniprocessor x86-32
non-generic kernel, you need to turn on X86_UP_IOAPIC and X86_UP_APIC
to see the option. Unless you're building for X86_VOYAGER in which case
you can't even see the options.
Now condensing this down to a sentence that isn't overly vague or
completely confusing is somewhat of a challenge. You're right that
my sentence is factually inaccurate, so I do want to fix it, but I don't
have a good replacement in mind. Maybe just:
For example, on x86, you must also enable SMP or X86_UP_APIC and
X86_UP_IOAPIC in order to see the CONFIG_PCI_MSI option.
And ignore the five Voyager users and the X86_GENERICARCH case.
> s/pdev/dev/
Good catch. Fixed.
> >
> > A device driver must always call free_irq() on the interrupt(s)
> > for which it has called request_irq() before calling this function.
> > Failure to do so will result in a BUG_ON(), the device will be left with
>
> s/,/;/ to kill run-on sentences.
Yup, fixed.
> > struct msix_entry {
> > u16 vector; /* kernel uses to write alloc vector */
> > u16 entry; /* driver uses to specify entry */
> > };
> >
> > This allows for the device to use these interrupts in a sparse fashion;
> > for example it could use interrupts 3 and 1027 and allocate only a
> > two-element array. The driver is expected to fill in the 'entry' value
> > in each element of the array to indicate which entries it wants the kernel
> > to assign interrupts for. It is invalid to fill in two entries with the
> > same number.
>
> so in this example, the driver would set msix[0].entry = 3 and
> msix[1].entry = 1-27 (??)
> and then continue with (below...):
Right, good point, added.
> s/pdev/dev/ /* or change the parameter name */
Just changed to 'dev'.
> > number once MSI-X is enabled. The device driver is responsible for
> > keeping track of the interrupts assigned to the MSI-X vectors so it can
> > free them again later.
> >
> > Device drivers should normally call this function once per device
> > during the initialization phase.
>
> or initialisation ? (was "initialised" above)
I tend to prefer the 's' spellings. Fixed.
> > the previously allocated message signaled interrupts. The interrupts may
>
> Good here ;)
Was probably in the original ;-)
> > A device driver must always call free_irq() on the interrupt(s)
> > for which it has called request_irq() before calling this function.
> > Failure to do so will result in a BUG_ON(), the device will be left with
>
> s/,/;/ to avoid run-on sentences.
*nod*.
> > interrupts must all be targetted at the same set of CPUs whereas MSI-X
> > interrupts can all be targetted at different CPUs.
>
> Yes, I would spell it "targeted", but you are writing it, so it's OK.
If we're going with 'Signaled', we should be consistent.
> > where $bridge is the PCI address of the bridge you've enabled (eg
>
> preferred: e.g.
Sure.
Thanks for the proof-reading.
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
