RE: [Penance PATCH] PCI: clean up the MSI code a bit

RE: [Penance PATCH] PCI: clean up the MSI code a bit

[Nguyen, Tom L]

On Thursday, June 09, 2005 4:49 PM Stefan Smietanowski wrote:
> 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.

Your logic does not work because existing MSI/MSI-X code does not allow
a driver to switch back and forth between MSI mode and MSI-X mode. A
driver can switch interrupt mode between NORMAL IRQ mode and MSI mode or
between NORMAL IRQ mode and MSI-X mode but NOT between MSI mode and
MSI-X mode. A device driver should know well which MSI mode or MSI-X
mode it wants to run when its device supports both MSI and MSI-X
capability structures. Please read MSI-HOWTO before any attempt. If you
like to continue this path, then think of a better policy of how to
manage vector sources for MSI and MSI-X allocation before making
changes.

Thanks,
Long

Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Stefan Smietanowski]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Tom.

>>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.
> 
> 
> Your logic does not work because existing MSI/MSI-X code does not allow
> a driver to switch back and forth between MSI mode and MSI-X mode. A
> driver can switch interrupt mode between NORMAL IRQ mode and MSI mode or
> between NORMAL IRQ mode and MSI-X mode but NOT between MSI mode and
> MSI-X mode. A device driver should know well which MSI mode or MSI-X
> mode it wants to run when its device supports both MSI and MSI-X
> capability structures. Please read MSI-HOWTO before any attempt. If you
> like to continue this path, then think of a better policy of how to
> manage vector sources for MSI and MSI-X allocation before making
> changes.

I know about the different capability structures, but the function
as listed above really "only" makes a few changes.

Currently, with legacy irq mode, the driver calls
pci_enable_msix(dev, .., ..);
and that's it.

If we default to MSI mode, then that is replaced by
pci_disable_msi(dev); pci_enable_msix(dev, .., ..);
with any error checking required, etc.

If we implement an error code for each of the cases, so that
the driver KNOWS which mode it's in after pci_enable_msix() is called
I don't see a difference. I'm not an expert on the subject and likely
missing things but ..

If we could successfully change from msi to msix mode we get a normal
zero return code from pci_enable_msix(), however if we ended up in
msi mode we could have a new return code indicating that we're now in
msi mode and if we ended up with legacy IRQ mode we have another one
telling us that. The problem as I see it is that we then don't get the
other error codes from pci_enable_msix(). Takes some thinking I guess.
Only solution I see, if we want to be able to pass the error code as
well as which mode we ended up in (if not msix), is either another
argument to pci_enable_msix, changing
int pci_enable_msix(struct pci_dev *dev, u32 *entries, int nvec)
to
int pci_enable_msix(struct pci_dev *dev, u32 *entries, int nvec, int *mode)
where mode is for instance 0 for msix, 1 for msi and 2 for legacy irq.
We could also have "mode" be a mask of what we support at all.

for instance the device driver would do:

mode = MSIX|LEGACY_IRQ;
pci_enable_msix(dev, entries, nvec, &mode);
where mode would be overwritten with the mode we ended up in.

Tell me I'm missing something here. This doesn't feel like too different
to the logic the driver must ALREADY have, since it ALREADY must know
how to fall back to legacy irq mode.

if (pci_enable_msix(dev, entries, nvec) < 0) {
  if (pci_enable_msi(dev) < 0)
   ...
  ...
}
This is all changed to

if (pci_enable_msix(dev, entries, nvec, mode) < 0) {
  if (mode==MSI)
    ..
  else if (mode==LEGACY_IRQ)
    ..
}

Or am I really not thinking straight?

The logic should be already there in the driver beforehand, so it's just
slightly different way of knowing it.

// Stefan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)

iD8DBQFCqbgMBrn2kJu9P78RArWwAKCPk+xZcqhMfFybHztNUqwvSrOJagCgn0O2
EhWG6p+wb/b1cKtPLJU1vzc=
=AgYC
-----END PGP SIGNATURE-----

RE: [Penance PATCH] PCI: clean up the MSI code a bit

[Nguyen, Tom L]

On Friday, June 10, 2005 8:56 AM Stefan Smietanowski wrote:
> If we default to MSI mode, then that is replaced by
> pci_disable_msi(dev); pci_enable_msix(dev, .., ..);
> with any error checking required, etc.
>
> If we implement an error code for each of the cases, so that
> the driver KNOWS which mode it's in after pci_enable_msix() is called
> I don't see a difference. I'm not an expert on the subject and likely
> missing things but ..

You have a good point. Please save your discussion and bring it back
when Linux Community's inputs agree that we should default to MSI mode.

Thanks,
Long

RE: [Penance PATCH] PCI: clean up the MSI code a bit

[Nguyen, Tom L]

On Wednesday, June 08, 2005 10:15 AM Grant Grundler wrote:
> > 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.

MSI-X will outpace MSI in future. In my opinion, enabling MSI by default
is a short-term solution. Again, this is just my opinion.

Thanks,
Long

[Penance PATCH] PCI: clean up the MSI code a bit

[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.

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.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/pci/msi.c |   88 ++++++++++++++++++++----------------------------------
 drivers/pci/msi.h |    9 ++---
 2 files changed, 37 insertions(+), 60 deletions(-)

--- gregkh-2.6.orig/drivers/pci/msi.c	2005-06-07 22:38:01.000000000 -0700
+++ gregkh-2.6/drivers/pci/msi.c	2005-06-07 23:12:29.000000000 -0700
@@ -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
--- gregkh-2.6.orig/drivers/pci/msi.h	2005-03-01 23:38:13.000000000 -0800
+++ gregkh-2.6/drivers/pci/msi.h	2005-06-07 23:10:43.000000000 -0700
@@ -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

Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Andi Kleen]

On Tue, Jun 07, 2005 at 11:35:59PM -0700, Greg KH wrote:
> 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

Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Grant Grundler]

On Wed, Jun 08, 2005 at 03:41:09PM +0200, Andi Kleen wrote:
> 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

Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Greg KH]

On Wed, Jun 08, 2005 at 11:14:40AM -0600, Grant Grundler wrote:
> On Wed, Jun 08, 2005 at 03:41:09PM +0200, Andi Kleen wrote:
> > 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

Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Andi Kleen]

On Wed, Jun 08, 2005 at 10:45:48AM -0700, Greg KH wrote:
> On Wed, Jun 08, 2005 at 11:14:40AM -0600, Grant Grundler wrote:
> > On Wed, Jun 08, 2005 at 03:41:09PM +0200, Andi Kleen wrote:
> > > 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.

> 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.

> 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

Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Stefan Smietanowski]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi.

> 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-----