PCI: clean up the MSI code a bit
PATCH 1/1 PCI: clean up the MSI code a bit
Ok, I'm very sorry for wasting people's time on this. In the end, I agree that the MSI code should stay as is. It's just to complex and confusing to enable it always for all devices at this time. I'll put the pci_enable/pci_disable idea on my TODO list to try to help out with some of the logic that every-other pci driver seems to have to duplicate all the time. That seems like the best way forward.
So, here's a patch to try to make up for starting this discussion in the first place. It's a small cleanup to the MSI code to help make the irq handling logic a bit easier to follow and understand (it also reduces the size of the compiled code as a nice side benefit:
text data bss dec hex filename 7543 136 972 8651 21cb drivers/pci/msi.o - before patch 7482 136 972 8590 218e drivers/pci/msi.o - after patch
Hey, every byte counts :)
I'm adding this to my pci tree and it will show up in the -mm tree eventually.
thanks,
greg k-h
------------------
PCI: clean up the MSI code a bit.
Mostly just cleans up the irq handling logic to be smaller and a bit more descriptive as to what it really does.
--- drivers/pci/msi.c | 88 ++++++++++++++++++++---------------------------------- drivers/pci/msi.h | 9 ++--- 2 files changed, 37 insertions(+), 60 deletions(-)
drivers/pci/msi.c +33 -55
@@ -28,10 +28,10 @@static kmem_cache_t * msi_cachep;static int pci_msi_enable =1 ; -static int last_alloc_vector =0 ; -static int nr_released_vectors =0 ; +static int last_alloc_vector; +static int nr_released_vectors;static int nr_reserved_vectors =NR_HP_RESERVED_VECTORS ; -static int nr_msix_devices =0 ; +static int nr_msix_devices;#ifndef CONFIG_X86_IO_APIC int vector_irq[NR_VECTORS ] = { [0 ...NR_VECTORS -1 ] =-1 };
@@ -170,44 +170,30 @@return 0 ;/* never anything pending */ } -static void release_msi (unsigned int vector); -static void shutdown_msi_irq (unsigned int vector) -{ -release_msi (vector); -} - -#define shutdown_msi_irq_wo_maskbit shutdown_msi_irq -static void enable_msi_irq_wo_maskbit (unsigned int vector) {} -static void disable_msi_irq_wo_maskbit (unsigned int vector) {} -static void ack_msi_irq_wo_maskbit (unsigned int vector) {} -static void end_msi_irq_wo_maskbit (unsigned int vector) +static unsigned int startup_msi_irq_w_maskbit (unsigned int vector) { -move_msi (vector); -ack_APIC_irq (); +startup_msi_irq_wo_maskbit (vector); +unmask_MSI_irq (vector); +return 0 ;/* never anything pending */ } -static unsigned int startup_msi_irq_w_maskbit (unsigned int vector) +static void shutdown_msi_irq (unsigned int vector) {struct msi_desc *entry;unsigned long flags;spin_lock_irqsave (&msi_lock, flags); entry = msi_desc[vector]; -if (!entry || !entry->dev) { -spin_unlock_irqrestore (&msi_lock, flags); -return 0 ; - } - entry->msi_attrib.state =1 ;/* Mark it active */ +if (entry && entry->dev) + entry->msi_attrib.state =0 ;/* Mark it not active */ spin_unlock_irqrestore (&msi_lock, flags); - -unmask_MSI_irq (vector); -return 0 ;/* never anything pending */ } -#define shutdown_msi_irq_w_maskbit shutdown_msi_irq -#define enable_msi_irq_w_maskbit unmask_MSI_irq -#define disable_msi_irq_w_maskbit mask_MSI_irq -#define ack_msi_irq_w_maskbit mask_MSI_irq +static void end_msi_irq_wo_maskbit (unsigned int vector) +{ +move_msi (vector); +ack_APIC_irq (); +}static void end_msi_irq_w_maskbit (unsigned int vector) {
@@ -216,6 +202,10 @@ack_APIC_irq (); } +static void do_nothing (unsigned int vector) +{ +} +/* * Interrupt Type for MSI-X PCI/PCI-X/PCI-Express Devices, * which implement the MSI-X Capability Structure.
@@ -223,10 +213,10 @@static struct hw_interrupt_type msix_irq_type = { .typename ="PCI-MSI-X" , .startup = startup_msi_irq_w_maskbit, - .shutdown = shutdown_msi_irq_w_maskbit, - .enable = enable_msi_irq_w_maskbit, - .disable = disable_msi_irq_w_maskbit, - .ack = ack_msi_irq_w_maskbit, + .shutdown = shutdown_msi_irq, + .enable = unmask_MSI_irq, + .disable = mask_MSI_irq, + .ack = mask_MSI_irq, .end = end_msi_irq_w_maskbit, .set_affinity = set_msi_irq_affinity };
@@ -239,10 +229,10 @@static struct hw_interrupt_type msi_irq_w_maskbit_type = { .typename ="PCI-MSI" , .startup = startup_msi_irq_w_maskbit, - .shutdown = shutdown_msi_irq_w_maskbit, - .enable = enable_msi_irq_w_maskbit, - .disable = disable_msi_irq_w_maskbit, - .ack = ack_msi_irq_w_maskbit, + .shutdown = shutdown_msi_irq, + .enable = unmask_MSI_irq, + .disable = mask_MSI_irq, + .ack = mask_MSI_irq, .end = end_msi_irq_w_maskbit, .set_affinity = set_msi_irq_affinity };
@@ -255,10 +245,10 @@static struct hw_interrupt_type msi_irq_wo_maskbit_type = { .typename ="PCI-MSI" , .startup = startup_msi_irq_wo_maskbit, - .shutdown = shutdown_msi_irq_wo_maskbit, - .enable = enable_msi_irq_wo_maskbit, - .disable = disable_msi_irq_wo_maskbit, - .ack = ack_msi_irq_wo_maskbit, + .shutdown = shutdown_msi_irq, + .enable = do_nothing, + .disable = do_nothing, + .ack = do_nothing, .end = end_msi_irq_wo_maskbit, .set_affinity = set_msi_irq_affinity };
@@ -407,7 +397,7 @@ {struct msi_desc *entry; - entry = (struct msi_desc *)kmem_cache_alloc (msi_cachep,SLAB_KERNEL ); + entry =kmem_cache_alloc (msi_cachep,SLAB_KERNEL );if (!entry)return NULL ;
@@ -796,18 +786,6 @@ } } -static void release_msi (unsigned int vector) -{ -struct msi_desc *entry; -unsigned long flags; - -spin_lock_irqsave (&msi_lock, flags); - entry = msi_desc[vector]; -if (entry && entry->dev) - entry->msi_attrib.state =0 ;/* Mark it not active */ -spin_unlock_irqrestore (&msi_lock, flags); -} -static int msi_free_vector (struct pci_dev * dev,int vector,int reassign) {struct msi_desc *entry;
@@ -924,7 +902,7 @@/** * pci_enable_msix - configure device's MSI-X capability structure * @dev: pointer to the pci_dev data structure of MSI-X device function -* @data: pointer to an array of MSI-X entries +* @entries: pointer to an array of MSI-X entries * @nvec: number of MSI-X vectors requested for allocation by device driver * * Setup the MSI-X capability structure of device function with the number
drivers/pci/msi.h +4 -5
@@ -41,11 +41,11 @@#define PCI_MSIX_FLAGS_BIRMASK (7 << 0)#define PCI_MSIX_FLAGS_BITMASK (1 << 0) -#define PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET 0 -#define PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET 4 -#define PCI_MSIX_ENTRY_DATA_OFFSET 8 -#define PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET 12#define PCI_MSIX_ENTRY_SIZE 16 +#define PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET 0 +#define PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET 4 +#define PCI_MSIX_ENTRY_DATA_OFFSET 8 +#define PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET 12#define msi_control_reg (base) (base + PCI_MSI_FLAGS)#define msi_lower_address_reg (base) (base + PCI_MSI_ADDRESS_LO)
@@ -64,7 +64,6 @@#define msi_enable (control, num) multi_msi_enable(control, num); \ control |= PCI_MSI_FLAGS_ENABLE -#define msix_control_reg msi_control_reg#define msix_table_offset_reg (base) (base + 0x04)#define msix_pba_offset_reg (base) (base + 0x08)#define msix_enable (control) control |= PCI_MSIX_FLAGS_ENABLE
Quoted 6 lines · Greg KH
Ok, I'm very sorry for wasting people's time on this. In the end, I agree that the MSI code should stay as is. It's just to complex and confusing to enable it always for all devices at this time. I'll put the pci_enable/pci_disable idea on my TODO list to try to help out with some of the logic that every-other pci driver seems to have to duplicate all the time. That seems like the best way forward.
I disagree it should stay as it is. Basically you are trading a bit less complexity in Infiniband now for a lot of code everywhere.
Does not seem like a good tradeoff.
-Andi
Quoted 2 lines · Andi Kleen
I disagree it should stay as it is. Basically you are trading a bit less complexity in Infiniband now for a lot of code everywhere.
It's not just infiniband. It's tg3 and e1000 as well.
grant
Quoted 5 lines · Grant Grundler
Quoted 2 lines · Andi Kleen
I disagree it should stay as it is. Basically you are trading a bit less complexity in Infiniband now for a lot of code everywhere.
It's not just infiniband. It's tg3 and e1000 as well.
Yes, it's every device that wants to enable MSI. So far, only one driver that wants to enable MSI, has to handle broken devices. And odds are, that driver just isn't tested properly yet :)
So I stand by my decision now, it's just too complex to enable MSI for everyone and expect drivers to disable it properly if they need to. The logic is just convoluted (see the patch for details.) As proof, I got it completly wrong the first time, and I'm still not sure that I got it correct after working on this for a while. :)
In the end, the pci_enable/pci_disable interface is the way to go.
thanks,
greg k-h
Quoted 8 lines · Greg KH
Quoted 5 lines · Grant Grundler
Quoted 2 lines · Andi Kleen
I disagree it should stay as it is. Basically you are trading a bit less complexity in Infiniband now for a lot of code everywhere.
It's not just infiniband. It's tg3 and e1000 as well.
Yes, it's every device that wants to enable MSI. So far, only one
No, it is every device that wants to use MSI-X.
Also see the proposals from Jeff/Stefan et.al. for simpler interfaces for this. No need to actually make it messy if we have nice helpers.
Quoted 4 lines
driver that wants to enable MSI, has to handle broken devices. And odds are, that driver just isn't tested properly yet :)
So I stand by my decision now, it's just too complex to enable MSI for
I think you decision is wrong here.
Quoted 6 lines
everyone and expect drivers to disable it properly if they need to. The logic is just convoluted (see the patch for details.) As proof, I got it completly wrong the first time, and I'm still not sure that I got it correct after working on this for a while. :)
In the end, the pci_enable/pci_disable interface is the way to go.
No, it isnt.
-Andi
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi.
Quoted 2 lines
Also see the proposals from Jeff/Stefan et.al. for simpler interfaces for this. No need to actually make it messy if we have nice helpers.
Could even be simplified one step closer.
My helper could simply be incorporated into pci_enable_msix().
pci_enable_msix(dev)
{
if (is_dev_msi(dev)) {
cur_status=MSI;
pci_disable_msi(dev);
}
else if (is_dev_msix(dev))
return(ALREADY_MSIX);
else
cur_status=OLD_FASHIONED;
if (!__pci_enable_msix(dev))
if (cur_status==MSI)
pci_enable_msi(dev);
}Add error-checking, etc, etc, etc.
Naturally pci_enable_msix() is renamed to __pci_enable_msix() in my example. You can also just put it inside here, or whatever.
That way, if you want to enable msix and it fails, it will return to whatever it was using before pci_enable_msix() was called.
Why this ALSO will work is that msi will already be enabled for devices that handle it (I'm thinking of this together with making msi default).
That's why we cache the current status, so that we can return the device to the previous state, be it normal or be it msi mode.
Hm. Can a device that fails MSI (due to any reason - NB not supporting it or some other reason) go to MSIX mode at all?
If it can't, then it's even shorter.
pci_enable_msix(dev)
{
if (is_dev_msi(dev))
pci_disable_msi(dev);
else if (is_dev_msix(dev))
return(ALREADY_MSIX);
else
return(MSIX_NOT_AVAILABLE);
if (!__pci_enable_msix(dev))
pci_enable_msi(dev);
}That way noone needs to explicitly turn off msi as it's done automatically instead and the device will after this call always be in either MSIX, MSI or NORMAL IRQ mode, and always in the "best" mode the device, motherboard, bios, NB, whatever combination is available.
// Stefan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)
iD8DBQFCqNWGBrn2kJu9P78RAjvUAJsFS2yj15j34CWKyh3CJ9iSDMydpACfc2a5 MA7LncSpbj6tQHigOFmFmrQ=
=4bMK
-----END PGP SIGNATURE-----