pci_enable_msi() for everyone?

pci_enable_msi() for everyone?

[Greg KH]

In talking with a few people about the MSI kernel code, they asked why
we can't just do the pci_enable_msi() call for every pci device in the
system (at somewhere like pci_enable_device() time or so).  That would
let all drivers and devices get the MSI functionality without changing
their code, and probably make the api a whole lot simpler.

Now I know the e1000 driver would have to specifically disable MSI for
some of their broken versions, and possibly some other drivers might
need this, but the downside seems quite small.

Or am I missing something pretty obvious here?

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]

Greg KH wrote:
> In talking with a few people about the MSI kernel code, they asked why
> we can't just do the pci_enable_msi() call for every pci device in the
> system (at somewhere like pci_enable_device() time or so).  That would
> let all drivers and devices get the MSI functionality without changing
> their code, and probably make the api a whole lot simpler.

Agreed.

And now is a good time to make changes, before bunches of drivers start 
using pci_enable_msi().  I am preparing such a change for the AHCI 
driver (see attached), which will be the standard SATA interface on most 
new motherboards.


> Now I know the e1000 driver would have to specifically disable MSI for
> some of their broken versions, and possibly some other drivers might
> need this, but the downside seems quite small.

tg3 needs to deal with some broken system chipsets as well.  See
tg3_test_msi(), which I would eventually prefer to eliminate in favor of 
PCI quirks and such.

An API note of warning though...   IMO eventually different drivers are 
going to want different behavior from pci_enable_device().  IDE already 
hacks around this, as Alan was required to do a while ago (IDE has a 
weird PCI BAR setup sometimes, requiring care during enabling).

Longer term, I think we will need a

	pci_enable(info on what to enable)

so that drivers can specify ahead of time "don't enable PIO, only MMIO", 
"don't enable MMIO, only PIO", "don't use MSI", etc.  and add a 
pci_disable() to undo all of that.

The more we add singleton functions like pci_enable_msi(), 
pci_set_master(), etc. the more I wish for a single function that 
handled all those details at one atomic point.  There is a lot of 
standard patterns that are hand-coded into every PCI driver's probe 
functions.

	Jeff


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3831 bytes --]

diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -39,7 +39,7 @@
 #include <asm/io.h>
 
 #define DRV_NAME	"ahci"
-#define DRV_VERSION	"1.00"
+#define DRV_VERSION	"1.01"
 
 
 enum {
@@ -153,6 +153,7 @@ struct ahci_sg {
 
 struct ahci_host_priv {
 	unsigned long		flags;
+	unsigned int		have_msi; /* is PCI MSI enabled? */
 	u32			cap;	/* cache of HOST_CAP register */
 	u32			port_map; /* cache of HOST_PORTS_IMPL reg */
 };
@@ -183,6 +184,7 @@ static void ahci_qc_prep(struct ata_queu
 static u8 ahci_check_status(struct ata_port *ap);
 static u8 ahci_check_err(struct ata_port *ap);
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
+static void ahci_remove_one (struct pci_dev *pdev);
 
 static Scsi_Host_Template ahci_sht = {
 	.module			= THIS_MODULE,
@@ -272,7 +274,7 @@ static struct pci_driver ahci_pci_driver
 	.name			= DRV_NAME,
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= ahci_remove_one,
 };
 
 
@@ -879,15 +881,19 @@ static int ahci_host_init(struct ata_pro
 }
 
 /* move to PCI layer, integrate w/ MSI stuff */
-static void pci_enable_intx(struct pci_dev *pdev)
+static void pci_intx(struct pci_dev *pdev, int enable)
 {
-	u16 pci_command;
+	u16 pci_command, new;
 
 	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
-	if (pci_command & PCI_COMMAND_INTX_DISABLE) {
-		pci_command &= ~PCI_COMMAND_INTX_DISABLE;
+
+	if (enable)
+		new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
+	else
+		new = pci_command | PCI_COMMAND_INTX_DISABLE;
+
+	if (new != pci_command)
 		pci_write_config_word(pdev, PCI_COMMAND, pci_command);
-	}
 }
 
 static void ahci_print_info(struct ata_probe_ent *probe_ent)
@@ -969,7 +975,7 @@ static int ahci_init_one (struct pci_dev
 	unsigned long base;
 	void *mmio_base;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
-	int pci_dev_busy = 0;
+	int have_msi, pci_dev_busy = 0;
 	int rc;
 
 	VPRINTK("ENTER\n");
@@ -987,12 +993,17 @@ static int ahci_init_one (struct pci_dev
 		goto err_out;
 	}
 
-	pci_enable_intx(pdev);
+	if (pci_enable_msi(pdev) == 0)
+		have_msi = 1;
+	else {
+		pci_intx(pdev, 1);
+		have_msi = 0;
+	}
 
 	probe_ent = kmalloc(sizeof(*probe_ent), GFP_KERNEL);
 	if (probe_ent == NULL) {
 		rc = -ENOMEM;
-		goto err_out_regions;
+		goto err_out_msi;
 	}
 
 	memset(probe_ent, 0, sizeof(*probe_ent));
@@ -1025,6 +1036,8 @@ static int ahci_init_one (struct pci_dev
 	probe_ent->mmio_base = mmio_base;
 	probe_ent->private_data = hpriv;
 
+	hpriv->have_msi = have_msi;
+
 	/* initialize adapter */
 	rc = ahci_host_init(probe_ent);
 	if (rc)
@@ -1044,7 +1057,11 @@ err_out_iounmap:
 	iounmap(mmio_base);
 err_out_free_ent:
 	kfree(probe_ent);
-err_out_regions:
+err_out_msi:
+	if (have_msi)
+		pci_disable_msi(pdev);
+	else
+		pci_intx(pdev, 0);
 	pci_release_regions(pdev);
 err_out:
 	if (!pci_dev_busy)
@@ -1052,6 +1069,42 @@ err_out:
 	return rc;
 }
 
+static void ahci_remove_one (struct pci_dev *pdev)
+{
+	struct device *dev = pci_dev_to_dev(pdev);
+	struct ata_host_set *host_set = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host_set->private_data;
+	struct ata_port *ap;
+	unsigned int i;
+	int have_msi;
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+
+		scsi_remove_host(ap->host);
+	}
+
+	have_msi = hpriv->have_msi;
+	free_irq(host_set->irq, host_set);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		ap = host_set->ports[i];
+
+		ata_scsi_release(ap->host);
+		scsi_host_put(ap->host);
+	}
+
+	host_set->ops->host_stop(host_set);
+	kfree(host_set);
+
+	if (have_msi)
+		pci_disable_msi(pdev);
+	else
+		pci_intx(pdev, 0);
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+	dev_set_drvdata(dev, NULL);
+}
 
 static int __init ahci_init(void)
 {

Re: pci_enable_msi() for everyone?

[Benjamin Herrenschmidt]

> 	pci_enable(info on what to enable)
> 
> so that drivers can specify ahead of time "don't enable PIO, only MMIO", 
> "don't enable MMIO, only PIO", "don't use MSI", etc.  and add a 
> pci_disable() to undo all of that.
> 
> The more we add singleton functions like pci_enable_msi(), 
> pci_set_master(), etc. the more I wish for a single function that 
> handled all those details at one atomic point.  There is a lot of 
> standard patterns that are hand-coded into every PCI driver's probe 
> functions.

Agreed, with the proper arch hook to deal with arch brokenness of
course.

That could be a bitmap. What I'm not 100% confident at this point is
wether we want a bit per BAR or an "IO" bit and an "MMIO" bit. I think
I'd rather go for the first one.

Ben.

Re: pci_enable_msi() for everyone?

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
>>	pci_enable(info on what to enable)
>>
>>so that drivers can specify ahead of time "don't enable PIO, only MMIO", 
>>"don't enable MMIO, only PIO", "don't use MSI", etc.  and add a 
>>pci_disable() to undo all of that.
>>
>>The more we add singleton functions like pci_enable_msi(), 
>>pci_set_master(), etc. the more I wish for a single function that 
>>handled all those details at one atomic point.  There is a lot of 
>>standard patterns that are hand-coded into every PCI driver's probe 
>>functions.
> 
> 
> Agreed, with the proper arch hook to deal with arch brokenness of
> course.
> 
> That could be a bitmap. What I'm not 100% confident at this point is
> wether we want a bit per BAR or an "IO" bit and an "MMIO" bit. I think
> I'd rather go for the first one.

A bitmap is what I would start with.  But I would implement it as

	struct pci_enable_info {
		unsigned long flags;
	};

because I guarantee we'll want more flexibility as time goes on.

Honestly I can think of situations where one driver would want a bit per 
BAR, and many others would just need a single MMIO bit.  Don't forget 
legacy decoding too:  with -only- a bit per BAR, the driver cannot tell 
the PCI layer that disabling IO means disabling a legacy ISA region 
that's not listed in the PCI BARs.

	Jeff

Re: pci_enable_msi() for everyone?

[Benjamin Herrenschmidt]

> Honestly I can think of situations where one driver would want a bit per 
> BAR, and many others would just need a single MMIO bit.  Don't forget 
> legacy decoding too:  with -only- a bit per BAR, the driver cannot tell 
> the PCI layer that disabling IO means disabling a legacy ISA region 
> that's not listed in the PCI BARs.

VGA is too much of a special case here. I'm currently working on a VGA
arbitrer but it will need a separate API (along with a userland
interface). Maybe the kernel side of this API could be folded in that
pci_enable() thing though, I'll have to give it a though...

Ben.

Re: pci_enable_msi() for everyone?

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
>>Honestly I can think of situations where one driver would want a bit per 
>>BAR, and many others would just need a single MMIO bit.  Don't forget 
>>legacy decoding too:  with -only- a bit per BAR, the driver cannot tell 
>>the PCI layer that disabling IO means disabling a legacy ISA region 
>>that's not listed in the PCI BARs.
> 
> 
> VGA is too much of a special case here. I'm currently working on a VGA
> arbitrer but it will need a separate API (along with a userland
> interface). Maybe the kernel side of this API could be folded in that
> pci_enable() thing though, I'll have to give it a though...

I was in fact thinking of IDE not VGA :)

Let's keep it simple.

Just need to make sure that, if we use an enable-by-PCI-BAR bitmap, it 
is still possible to let the driver make decisions about enable/disable 
of the IO and MMIO bits.  You could have a PCI BAR bitmap and then two 
additional "don't touch <xxx>" bits, for example.

	Jeff

Re: pci_enable_msi() for everyone?

[Greg KH]

On Fri, Jun 03, 2005 at 07:16:04PM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> >In talking with a few people about the MSI kernel code, they asked why
> >we can't just do the pci_enable_msi() call for every pci device in the
> >system (at somewhere like pci_enable_device() time or so).  That would
> >let all drivers and devices get the MSI functionality without changing
> >their code, and probably make the api a whole lot simpler.
> 
> Agreed.
> 
> And now is a good time to make changes, before bunches of drivers start 
> using pci_enable_msi().  I am preparing such a change for the AHCI 
> driver (see attached), which will be the standard SATA interface on most 
> new motherboards.

Yes, roll that pci_intx() into the core and have it do the
pci_disable_msi() also if needed would be what I am thinking of.

> >Now I know the e1000 driver would have to specifically disable MSI for
> >some of their broken versions, and possibly some other drivers might
> >need this, but the downside seems quite small.
> 
> tg3 needs to deal with some broken system chipsets as well.  See
> tg3_test_msi(), which I would eventually prefer to eliminate in favor of 
> PCI quirks and such.

Ok, that's fine.

> An API note of warning though...   IMO eventually different drivers are 
> going to want different behavior from pci_enable_device().  IDE already 
> hacks around this, as Alan was required to do a while ago (IDE has a 
> weird PCI BAR setup sometimes, requiring care during enabling).
> 
> Longer term, I think we will need a
> 
> 	pci_enable(info on what to enable)
> 
> so that drivers can specify ahead of time "don't enable PIO, only MMIO", 
> "don't enable MMIO, only PIO", "don't use MSI", etc.  and add a 
> pci_disable() to undo all of that.

Yes, I agree, but let's start with baby steps :)

> The more we add singleton functions like pci_enable_msi(), 
> pci_set_master(), etc. the more I wish for a single function that 
> handled all those details at one atomic point.  There is a lot of 
> standard patterns that are hand-coded into every PCI driver's probe 
> functions.

Also agreed.

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Roland Dreier]

    Greg> In talking with a few people about the MSI kernel code, they
    Greg> asked why we can't just do the pci_enable_msi() call for
    Greg> every pci device in the system (at somewhere like
    Greg> pci_enable_device() time or so).  That would let all drivers
    Greg> and devices get the MSI functionality without changing their
    Greg> code, and probably make the api a whole lot simpler.

    Greg> Now I know the e1000 driver would have to specifically
    Greg> disable MSI for some of their broken versions, and possibly
    Greg> some other drivers might need this, but the downside seems
    Greg> quite small.

This was discussed the first time around when MSI patches were first
posted, and the consensus then was that it should be an "opt in"
system for drivers.  However, perhaps things has matured enough now
with PCI Express catching on, etc.

I think the number of devices truly compliant with the MSI spec is
quite tiny.  Probably just about every driver for a device that
actually has an MSI capability in its PCI header will need code to
work around some breakage, or will just end up disabling MSI entirely
because it never works.  Also I don't know how many PCI host bridges
implement MSI correctly.  For example we have a quirk for AMD 8131,
but who knows how many other chipsets are broken (and some bugs may be
much more subtle than the way the AMD 8131 breaks, which is to never
deliver interrupts).

Also, there needs to be a way for drivers to ask for multiple MSI-X
vectors.  For example the mthca InfiniBand driver gets a nice
performance boost by using separate interrupts for different types of
events.  I'm also planning on adding support for having one completion
interrupt per CPU, to help SMP scalability.

With all that said this might be a good idea, as long as there's a way
for drivers to enable MSI-X cleanly and a way for people to disable
the whole thing (eg a boot option).

 - R.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Fri, Jun 03, 2005 at 04:36:17PM -0700, Roland Dreier wrote:
>     Greg> In talking with a few people about the MSI kernel code, they
>     Greg> asked why we can't just do the pci_enable_msi() call for
>     Greg> every pci device in the system (at somewhere like
>     Greg> pci_enable_device() time or so).  That would let all drivers
>     Greg> and devices get the MSI functionality without changing their
>     Greg> code, and probably make the api a whole lot simpler.
> 
>     Greg> Now I know the e1000 driver would have to specifically
>     Greg> disable MSI for some of their broken versions, and possibly
>     Greg> some other drivers might need this, but the downside seems
>     Greg> quite small.
> 
> This was discussed the first time around when MSI patches were first
> posted, and the consensus then was that it should be an "opt in"
> system for drivers.  However, perhaps things has matured enough now
> with PCI Express catching on, etc.

Yeah, that's what I'm trying to find out.

> I think the number of devices truly compliant with the MSI spec is
> quite tiny.  Probably just about every driver for a device that
> actually has an MSI capability in its PCI header will need code to
> work around some breakage, or will just end up disabling MSI entirely
> because it never works.  Also I don't know how many PCI host bridges
> implement MSI correctly.  For example we have a quirk for AMD 8131,
> but who knows how many other chipsets are broken (and some bugs may be
> much more subtle than the way the AMD 8131 breaks, which is to never
> deliver interrupts).

Motherboard quirks are one thing.  Broken devices are a totally
different thing.  If there are too many of them, then the current
situation is acceptable to me.  Does ib have devices that will break
with MSI?

> Also, there needs to be a way for drivers to ask for multiple MSI-X
> vectors.  For example the mthca InfiniBand driver gets a nice
> performance boost by using separate interrupts for different types of
> events.  I'm also planning on adding support for having one completion
> interrupt per CPU, to help SMP scalability.

In looking at that, I don't see a way to get rid of the msix stuff.  So
that's probably just going to stay the same.

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Roland Dreier]

    Greg> Motherboard quirks are one thing.  Broken devices are a
    Greg> totally different thing.  If there are too many of them,
    Greg> then the current situation is acceptable to me.  Does ib
    Greg> have devices that will break with MSI?

Yes, I believe some versions of the firmware for Mellanox HCAs have
problems with MSI.

    Greg> In looking at that, I don't see a way to get rid of the msix
    Greg> stuff.  So that's probably just going to stay the same.

OK -- we'll just have to make sure that the switch from MSI mode to
MSI-X mode is implementated correctly.

 - R.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 05:23:17PM -0700, Roland Dreier wrote:
>     Greg> Motherboard quirks are one thing.  Broken devices are a
>     Greg> totally different thing.  If there are too many of them,
>     Greg> then the current situation is acceptable to me.  Does ib
>     Greg> have devices that will break with MSI?
> 
> Yes, I believe some versions of the firmware for Mellanox HCAs have
> problems with MSI.

Ick ick ick, and people think writing drivers is "easy"...

>     Greg> In looking at that, I don't see a way to get rid of the msix
>     Greg> stuff.  So that's probably just going to stay the same.
> 
> OK -- we'll just have to make sure that the switch from MSI mode to
> MSI-X mode is implementated correctly.

Hm good point, I think I got that wrong in my patch, let me go fix that
up...

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Grant Grundler]

On Fri, Jun 03, 2005 at 03:45:51PM -0700, Greg KH wrote:
> In talking with a few people about the MSI kernel code, they asked why
> we can't just do the pci_enable_msi() call for every pci device in the
> system (at somewhere like pci_enable_device() time or so).  That would
> let all drivers and devices get the MSI functionality without changing
> their code, and probably make the api a whole lot simpler.

One complication is some drivers will want to register a different
IRQ handler depending on if MSI is enabled or not.
If MSI is enabled (and usable), then some MMIO reads can be omitted.
I've posted a patch for tg3 driver:
	ftp://ftp.parisc-linux.org/patches/diff-2.6.10-tg3_MSI-03

(Just an example! It was not accepted because of buggy HW
 though it worked great on the HW I have access to.)

drivers/infiniband/hw/mthca driver is another example.

> Now I know the e1000 driver would have to specifically disable MSI for
> some of their broken versions, and possibly some other drivers might
> need this, but the downside seems quite small.
> 
> Or am I missing something pretty obvious here?

How can the driver know which IRQ handlers to register?

grant

Re: pci_enable_msi() for everyone?

[Greg KH]

On Fri, Jun 03, 2005 at 07:31:12PM -0600, Grant Grundler wrote:
> On Fri, Jun 03, 2005 at 03:45:51PM -0700, Greg KH wrote:
> > In talking with a few people about the MSI kernel code, they asked why
> > we can't just do the pci_enable_msi() call for every pci device in the
> > system (at somewhere like pci_enable_device() time or so).  That would
> > let all drivers and devices get the MSI functionality without changing
> > their code, and probably make the api a whole lot simpler.
> 
> One complication is some drivers will want to register a different
> IRQ handler depending on if MSI is enabled or not.

That's fine, they can always check the device capabilities and do that.

> If MSI is enabled (and usable), then some MMIO reads can be omitted.
> I've posted a patch for tg3 driver:
> 	ftp://ftp.parisc-linux.org/patches/diff-2.6.10-tg3_MSI-03
> 
> (Just an example! It was not accepted because of buggy HW
>  though it worked great on the HW I have access to.)
> 
> drivers/infiniband/hw/mthca driver is another example.

But it doesn't do that yet either ;)

> > Now I know the e1000 driver would have to specifically disable MSI for
> > some of their broken versions, and possibly some other drivers might
> > need this, but the downside seems quite small.
> > 
> > Or am I missing something pretty obvious here?
> 
> How can the driver know which IRQ handlers to register?

Same as always, use the dev->irq field like they do today.

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Grant Grundler]

On Fri, Jun 03, 2005 at 11:48:21PM -0700, Greg KH wrote:
> > One complication is some drivers will want to register a different
> > IRQ handler depending on if MSI is enabled or not.
> 
> That's fine, they can always check the device capabilities and do that.

Can you be more specific?
Maybe a short chunk of psuedo code?

> > If MSI is enabled (and usable), then some MMIO reads can be omitted.
> > I've posted a patch for tg3 driver:
> > 	ftp://ftp.parisc-linux.org/patches/diff-2.6.10-tg3_MSI-03
> > 
> > (Just an example! It was not accepted because of buggy HW
> >  though it worked great on the HW I have access to.)
> > 
> > drivers/infiniband/hw/mthca driver is another example.
> 
> But it doesn't do that yet either ;)

Sorry - only uses different IRQ handlers for MSI-X support.
But it could do something different for MSI IRQ handlers as well.

> > How can the driver know which IRQ handlers to register?
> 
> Same as always, use the dev->irq field like they do today.

I think you misunderstood my question.
The driver uses dev->irq as a "token" to register *some* IRQ handler.
If the driver wants to register "tg3_irq_nommioread()" for the
MSI case and "tg3_irq()" for Line Based IRQ case, how would the
driver know which IRQ handler it should register?

The arch IRQ support knows the difference and currently returns
that status in the pci_msi_enable() call.

thanks,
grant

Re: pci_enable_msi() for everyone?

[Greg KH]

On Sat, Jun 04, 2005 at 01:05:37AM -0600, Grant Grundler wrote:
> On Fri, Jun 03, 2005 at 11:48:21PM -0700, Greg KH wrote:
> > > One complication is some drivers will want to register a different
> > > IRQ handler depending on if MSI is enabled or not.
> > 
> > That's fine, they can always check the device capabilities and do that.
> 
> Can you be more specific?
> Maybe a short chunk of psuedo code?

Hm, here's a possible function to do it (typed into my email client, not
compiled, no warranties, etc...):

/* returns 1 if device is in MSI mode, 0 otherwise */
int pci_in_msi_mode(struct pci_dev *dev)
{
	int pos;
	u16 control;

	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
	if (!pos)
		return 0;
	pci_read_config_word(dev, msi_control_reg(pos), &control);
	if (control & PCI_MSI_FLAGS_ENABLE);
		return 1;
	return 0;
}

> > > If MSI is enabled (and usable), then some MMIO reads can be omitted.
> > > I've posted a patch for tg3 driver:
> > > 	ftp://ftp.parisc-linux.org/patches/diff-2.6.10-tg3_MSI-03
> > > 
> > > (Just an example! It was not accepted because of buggy HW
> > >  though it worked great on the HW I have access to.)
> > > 
> > > drivers/infiniband/hw/mthca driver is another example.
> > 
> > But it doesn't do that yet either ;)
> 
> Sorry - only uses different IRQ handlers for MSI-X support.
> But it could do something different for MSI IRQ handlers as well.

Sure.

> > > How can the driver know which IRQ handlers to register?
> > 
> > Same as always, use the dev->irq field like they do today.
> 
> I think you misunderstood my question.
> The driver uses dev->irq as a "token" to register *some* IRQ handler.
> If the driver wants to register "tg3_irq_nommioread()" for the
> MSI case and "tg3_irq()" for Line Based IRQ case, how would the
> driver know which IRQ handler it should register?
> 
> The arch IRQ support knows the difference and currently returns
> that status in the pci_msi_enable() call.

If you use the above function, then you can tell the difference and
register different irq handlers if you wish.

The main point being is that the pci_enable_msi() function would not
have to be explicitly called by your driver, it would have already been
taken care of earlier by the PCI core.  That's what I want to do and am
wondering if there would be any bad side affects to it.

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Dave Jones]

On Sat, Jun 04, 2005 at 12:18:03AM -0700, Greg KH wrote:
 > On Sat, Jun 04, 2005 at 01:05:37AM -0600, Grant Grundler wrote:
 > > On Fri, Jun 03, 2005 at 11:48:21PM -0700, Greg KH wrote:
 > > > > One complication is some drivers will want to register a different
 > > > > IRQ handler depending on if MSI is enabled or not.
 > > > 
 > > > That's fine, they can always check the device capabilities and do that.
 > > 
 > > Can you be more specific?
 > > Maybe a short chunk of psuedo code?
 > 
 > Hm, here's a possible function to do it (typed into my email client, not
 > compiled, no warranties, etc...):
 > 
 > /* returns 1 if device is in MSI mode, 0 otherwise */
 > int pci_in_msi_mode(struct pci_dev *dev)
 > {
 > 	int pos;
 > 	u16 control;
 > 
 > 	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
 > 	if (!pos)
 > 		return 0;
 > 	pci_read_config_word(dev, msi_control_reg(pos), &control);
 > 	if (control & PCI_MSI_FLAGS_ENABLE);
 > 		return 1;
 > 	return 0;
 > }

What if MSI support has been disabled in the bridge due to some quirk
(like the recent AMD 8111 quirk) ?   Maybe the above function
should check pci_msi_enable as well ?

		Dave

Re: pci_enable_msi() for everyone?

[Roland Dreier]

    Dave> What if MSI support has been disabled in the bridge due to
    Dave> some quirk (like the recent AMD 8111 quirk) ?  Maybe the
    Dave> above function should check pci_msi_enable as well ?

You can't disable MSI at a bridge -- according to the PCI spec, as
soon as a device has the MSI enable turned on, it must start using MSI
for interrupts and must not ever assert an interrupt pine.  The issue
with AMD 8131 is that it doesn't have any MSI support and just
silently throws away MSI messages, and so the host never gets
interrupts from devices in MSI mode.  Which means any device below
such a host bridge better not have MSI or MSI-X enabled.

 - R.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Sat, Jun 04, 2005 at 03:23:48AM -0400, Dave Jones wrote:
> What if MSI support has been disabled in the bridge due to some quirk
> (like the recent AMD 8111 quirk) ?   Maybe the above function
> should check pci_msi_enable as well ?

Yes, you are correct.  I said it wasn't tested :)

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Roland Dreier]

    davej> What if MSI support has been disabled in the bridge due to
    davej> some quirk (like the recent AMD 8111 quirk) ?  Maybe the
    davej> above function should check pci_msi_enable as well ?

    Greg> Yes, you are correct.  I said it wasn't tested :)

Huh?  If a host bridge doesn't support MSI, and a device below it has
its MSI capability enabled, we're in big trouble.  Because that device
is going to send interrupt messages whether the bridge likes it or
not.

 - R.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 05:26:32PM -0700, Roland Dreier wrote:
> 
>     davej> What if MSI support has been disabled in the bridge due to
>     davej> some quirk (like the recent AMD 8111 quirk) ?  Maybe the
>     davej> above function should check pci_msi_enable as well ?
> 
>     Greg> Yes, you are correct.  I said it wasn't tested :)
> 
> Huh?  If a host bridge doesn't support MSI, and a device below it has
> its MSI capability enabled, we're in big trouble.  Because that device
> is going to send interrupt messages whether the bridge likes it or
> not.

No, that device would never get MSI enabled on it.  See the patch I
posted to make sure I didn't get it wrong...

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Adam Belay]

On Mon, Jun 06, 2005 at 10:22:03PM -0700, Greg KH wrote:
> On Mon, Jun 06, 2005 at 05:26:32PM -0700, Roland Dreier wrote:
> > 
> >     davej> What if MSI support has been disabled in the bridge due to
> >     davej> some quirk (like the recent AMD 8111 quirk) ?  Maybe the
> >     davej> above function should check pci_msi_enable as well ?
> > 
> >     Greg> Yes, you are correct.  I said it wasn't tested :)
> > 
> > Huh?  If a host bridge doesn't support MSI, and a device below it has
> > its MSI capability enabled, we're in big trouble.  Because that device
> > is going to send interrupt messages whether the bridge likes it or
> > not.
> 
> No, that device would never get MSI enabled on it.  See the patch I
> posted to make sure I didn't get it wrong...
> 
> thanks,
> 
> greg k-h

How are we handling the case where a device has multiple MSI messages.
Is any driver interaction needed for that?  Will this change affect it?
I haven't had a chance to look through the MSI code yet.

Thanks,
Adam

Re: pci_enable_msi() for everyone?

[Luben Tuikov]

On 06/07/05 01:46, Adam Belay wrote:
> On Mon, Jun 06, 2005 at 10:22:03PM -0700, Greg KH wrote:
> 
>>On Mon, Jun 06, 2005 at 05:26:32PM -0700, Roland Dreier wrote:
>>
>>>    davej> What if MSI support has been disabled in the bridge due to
>>>    davej> some quirk (like the recent AMD 8111 quirk) ?  Maybe the
>>>    davej> above function should check pci_msi_enable as well ?
>>>
>>>    Greg> Yes, you are correct.  I said it wasn't tested :)
>>>
>>>Huh?  If a host bridge doesn't support MSI, and a device below it has
>>>its MSI capability enabled, we're in big trouble.  Because that device
>>>is going to send interrupt messages whether the bridge likes it or
>>>not.
>>
>>No, that device would never get MSI enabled on it.  See the patch I
>>posted to make sure I didn't get it wrong...
>>
>>thanks,
>>
>>greg k-h
> 
> 
> How are we handling the case where a device has multiple MSI messages.
> Is any driver interaction needed for that?  Will this change affect it?
> I haven't had a chance to look through the MSI code yet.
> 
> Thanks,
> Adam

Yes, this is a very good point (PCI MSI-X).

All in all, given all the hardware quirks of both
PCI bridges and PCI devices, I'd leave PCI MSI
control to the PCI LLDD.

	Luben

Re: pci_enable_msi() for everyone?

[Benjamin Herrenschmidt]

> Hm, here's a possible function to do it (typed into my email client, not
> compiled, no warranties, etc...):
> 
> /* returns 1 if device is in MSI mode, 0 otherwise */
> int pci_in_msi_mode(struct pci_dev *dev)
> {
> 	int pos;
> 	u16 control;
> 
> 	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> 	if (!pos)
> 		return 0;
> 	pci_read_config_word(dev, msi_control_reg(pos), &control);
> 	if (control & PCI_MSI_FLAGS_ENABLE);
> 		return 1;
> 	return 0;
> }

That would assume the architecture/slot/hw_setup always support MSI.
What if you put an SMI capable card in a machine that doesn't do MSI ?

> If you use the above function, then you can tell the difference and
> register different irq handlers if you wish.

No you can't because you lack the result code from pci_enable_msi()
which can fail (because it's veto'd by the arch for example)

> The main point being is that the pci_enable_msi() function would not
> have to be explicitly called by your driver, it would have already been
> taken care of earlier by the PCI core.  That's what I want to do and am
> wondering if there would be any bad side affects to it.

Disagreed.

Ben

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 08:00:26AM +1000, Benjamin Herrenschmidt wrote:
> 
> > Hm, here's a possible function to do it (typed into my email client, not
> > compiled, no warranties, etc...):
> > 
> > /* returns 1 if device is in MSI mode, 0 otherwise */
> > int pci_in_msi_mode(struct pci_dev *dev)
> > {
> > 	int pos;
> > 	u16 control;
> > 
> > 	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> > 	if (!pos)
> > 		return 0;
> > 	pci_read_config_word(dev, msi_control_reg(pos), &control);
> > 	if (control & PCI_MSI_FLAGS_ENABLE);
> > 		return 1;
> > 	return 0;
> > }
> 
> That would assume the architecture/slot/hw_setup always support MSI.
> What if you put an SMI capable card in a machine that doesn't do MSI ?

The ENABLE flag would not have been set by the current pci_enable_msi()
function.

> > If you use the above function, then you can tell the difference and
> > register different irq handlers if you wish.
> 
> No you can't because you lack the result code from pci_enable_msi()
> which can fail (because it's veto'd by the arch for example)

That's what the above function is for.  To call before setting up the
irq handlers.

> > The main point being is that the pci_enable_msi() function would not
> > have to be explicitly called by your driver, it would have already been
> > taken care of earlier by the PCI core.  That's what I want to do and am
> > wondering if there would be any bad side affects to it.
> 
> Disagreed.

Disagreed in what way?  What's the bad side affects?

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Benjamin Herrenschmidt]

> > 
> > Disagreed.
> 
> Disagreed in what way?  What's the bad side affects?

I'd rather have the driver decide wether to enable it or not
explicitely ... but heh, that's just my taste ...

Ben.

Re: pci_enable_msi() for everyone?

[David S. Miller]

From: Greg KH <gregkh@suse.de>
Date: Fri, 3 Jun 2005 15:45:51 -0700

> Now I know the e1000 driver would have to specifically disable MSI for
> some of their broken versions, and possibly some other drivers might
> need this, but the downside seems quite small.
> 
> Or am I missing something pretty obvious here?

This is totally undesirable.  We don't want the device sending
out MSI messages unless the driver for it explicitly knows
that it is operating the device in this mode.

TG3 will disable MSI for several chip variants as well.  It will
also disable MSI if it's internal self-test of MSI functionality
fails.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Sun, Jun 05, 2005 at 12:46:12PM -0700, David S. Miller wrote:
> From: Greg KH <gregkh@suse.de>
> Date: Fri, 3 Jun 2005 15:45:51 -0700
> 
> > Now I know the e1000 driver would have to specifically disable MSI for
> > some of their broken versions, and possibly some other drivers might
> > need this, but the downside seems quite small.
> > 
> > Or am I missing something pretty obvious here?
> 
> This is totally undesirable.  We don't want the device sending
> out MSI messages unless the driver for it explicitly knows
> that it is operating the device in this mode.

Why would it matter?  The driver shouldn't care if the interrupts come
in via the standard interrupt way, or through MSI, right?  And if it
does, it could always use a function like the one I proposed to set up a
different irq handler.

> TG3 will disable MSI for several chip variants as well.  It will
> also disable MSI if it's internal self-test of MSI functionality
> fails.

That's fine to disable msi, I don't have an issue with that.  I'm just
getting pushback from some vendors as to why MSI isn't explicitly
enabled by default and I don't have any solid answers.

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[David S. Miller]

From: Greg KH <gregkh@suse.de>
Date: Mon, 6 Jun 2005 15:55:49 -0700

> Why would it matter?  The driver shouldn't care if the interrupts come
> in via the standard interrupt way, or through MSI, right?  And if it
> does, it could always use a function like the one I proposed to set up a
> different irq handler.

It matters because MSI totally changes the DMA vs. interrupt delivery
guarentees.

With MSI, you can be assured that any DMA sent from the device, before
the interrupt, has reached global visibility before the MSI arrives at
the cpu.  There is no such guarentee with pre-MSI interrupt delivery.

Drivers optimize this, so they have to know if MSI is being used or
not.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 03:59:54PM -0700, David S. Miller wrote:
> From: Greg KH <gregkh@suse.de>
> Date: Mon, 6 Jun 2005 15:55:49 -0700
> 
> > Why would it matter?  The driver shouldn't care if the interrupts come
> > in via the standard interrupt way, or through MSI, right?  And if it
> > does, it could always use a function like the one I proposed to set up a
> > different irq handler.
> 
> It matters because MSI totally changes the DMA vs. interrupt delivery
> guarentees.
> 
> With MSI, you can be assured that any DMA sent from the device, before
> the interrupt, has reached global visibility before the MSI arrives at
> the cpu.  There is no such guarentee with pre-MSI interrupt delivery.
> 
> Drivers optimize this, so they have to know if MSI is being used or
> not.

Ok, I buy that argument, but currently for all of the drivers in the
tree using MSI (all 7), only the tg3 driver does such an optimizaion
(well, the mthca does some other MSI-X stuff too.)  And I'm still saying
that if you want to provide such a functionality, you can.  The function
to see if MSI is being used or not will be present, which drivers can
call if they wish to to do such optimizations.

So, the tg3 driver would need a small tweak, but the other drivers would
get code removed from them...

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Jeff Garzik]

Greg KH wrote:
> Why would it matter?  The driver shouldn't care if the interrupts come
> in via the standard interrupt way, or through MSI, right?  And if it

It matters.

Not only the differences DaveM mentioned, but also simply that you may 
assume your interrupt is not shared with anyone else.

	Jeff

Re: pci_enable_msi() for everyone?

[David S. Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Mon, 06 Jun 2005 19:08:33 -0400

> Not only the differences DaveM mentioned, but also simply that you may 
> assume your interrupt is not shared with anyone else.

I had forgotten this fundamental different, good catch
Jeff.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 07:08:33PM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> >Why would it matter?  The driver shouldn't care if the interrupts come
> >in via the standard interrupt way, or through MSI, right?  And if it
> 
> It matters.
> 
> Not only the differences DaveM mentioned, but also simply that you may 
> assume your interrupt is not shared with anyone else.

Ok, and again, how would the call, pci_in_msi_mode(struct pci_dev *dev)
not allow for the driver to determine this?

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[Jeff Garzik]

Greg KH wrote:
> On Mon, Jun 06, 2005 at 07:08:33PM -0400, Jeff Garzik wrote:
> 
>>Greg KH wrote:
>>
>>>Why would it matter?  The driver shouldn't care if the interrupts come
>>>in via the standard interrupt way, or through MSI, right?  And if it
>>
>>It matters.
>>
>>Not only the differences DaveM mentioned, but also simply that you may 
>>assume your interrupt is not shared with anyone else.
> 
> 
> Ok, and again, how would the call, pci_in_msi_mode(struct pci_dev *dev)
> not allow for the driver to determine this?

Let me see if I understand this correctly :)

A technology (MSI) allows one to more efficiently call interrupt 
handlers, with fewer bus reads...  and you want to add a test to each 
interrupt handler -- a test which adds several bus reads to the hot path 
of every MSI driver?

We want to -decrease- the overhead involved with an interrupt, but 
pci_in_msi_mode() increases it.

	Jeff

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 07:53:55PM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> >On Mon, Jun 06, 2005 at 07:08:33PM -0400, Jeff Garzik wrote:
> >
> >>Greg KH wrote:
> >>
> >>>Why would it matter?  The driver shouldn't care if the interrupts come
> >>>in via the standard interrupt way, or through MSI, right?  And if it
> >>
> >>It matters.
> >>
> >>Not only the differences DaveM mentioned, but also simply that you may 
> >>assume your interrupt is not shared with anyone else.
> >
> >
> >Ok, and again, how would the call, pci_in_msi_mode(struct pci_dev *dev)
> >not allow for the driver to determine this?
> 
> Let me see if I understand this correctly :)
> 
> A technology (MSI) allows one to more efficiently call interrupt 
> handlers, with fewer bus reads...  and you want to add a test to each 
> interrupt handler -- a test which adds several bus reads to the hot path 
> of every MSI driver?

hell no.

> We want to -decrease- the overhead involved with an interrupt, but 
> pci_in_msi_mode() increases it.

No, just call pci_in_msi_mode() where you were previously calling
pci_enable_msi() to test to see if we are in msi mode.

Patches in a bit to hopefully better show what I am talking about...

thanks,

greg k-h

Re: pci_enable_msi() for everyone?

[David S. Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Mon, 06 Jun 2005 19:53:55 -0400

> A technology (MSI) allows one to more efficiently call interrupt 
> handlers, with fewer bus reads...  and you want to add a test to each 
> interrupt handler -- a test which adds several bus reads to the hot path 
> of every MSI driver?

I think he's saying something different.

He is saying this test goes in the spot where you select
which interrupt handler to register, in place of the current
logic which call pci_enable_msi() and checks the return value.

MSI unaware drivers are OK because they can only assume
looser semantics (shared interrupts possible, no DMA ordering
guarentees wrt. interrupt delivery etc.) in their interrupt
handlers.

So I guess what Greg is proposing is not a bad idea afterall.

Re: pci_enable_msi() for everyone?

[Grant Grundler]

On Mon, Jun 06, 2005 at 07:08:33PM -0400, Jeff Garzik wrote:
> Not only the differences DaveM mentioned, but also simply that you may 
> assume your interrupt is not shared with anyone else.

I had the impression the CPU vector could be shared.
But PCI 2.3 spec says:
| An MSI is by definition a non-shared interrupt that enforces data
| consistency (ensures the iterrupt service routine accesses the most
| recent data). The system guarantees that any data written by the
| device prior to sending the MSI has reached its final destination
| before the interrupt service routine accesses that data. Therefore,
| a device driver is not required to read its device before servicing
| its MSI.


I guess that's pretty clear....

grant

Re: pci_enable_msi() for everyone?

[Roland Dreier]

    Greg> That's fine to disable msi, I don't have an issue with that.
    Greg> I'm just getting pushback from some vendors as to why MSI
    Greg> isn't explicitly enabled by default and I don't have any
    Greg> solid answers.

Why don't those vendors push back on their own behalf, on public
mailing lists?

 - R.

Re: pci_enable_msi() for everyone?

[Greg KH]

On Mon, Jun 06, 2005 at 05:18:49PM -0700, Roland Dreier wrote:
>     Greg> That's fine to disable msi, I don't have an issue with that.
>     Greg> I'm just getting pushback from some vendors as to why MSI
>     Greg> isn't explicitly enabled by default and I don't have any
>     Greg> solid answers.
> 
> Why don't those vendors push back on their own behalf, on public
> mailing lists?

Heh, that's always the question...  They are on these lists, and should
probably speak up.

Oh yeah, Tom, any opinions on this topic?  (note, Intel is not the
vendor pushing, just to squash any rumors...)

thanks,

greg k-h

RE: pci_enable_msi() for everyone?

[Nguyen, Tom L]

Monday, June 06, 2005 10:22 PM Greg KH wrote:
>> 
>> Huh?  If a host bridge doesn't support MSI, and a device below it has
>> its MSI capability enabled, we're in big trouble.  Because that
device
>> is going to send interrupt messages whether the bridge likes it or
>> not.
>
>No, that device would never get MSI enabled on it.  See the patch I
>posted to make sure I didn't get it wrong...

You're correct. If a host bridge doesn't support MSI, MSI quirk is set
to indicate that do not enable MSI/MSI-X on any device.

Thanks,
Long