On Tue, 2007-02-27 at 12:31 -0700, Eric W. Biederman wrote: > enable/disable_msi_mode have several side effects which keeps them from > being generally useful. So this patch replaces them with with two > much more targeted functions: msi_set_enable and msix_set_enable. > > This patch makes pci_dev->msi_enabled and pci_dev->msix_enabled the > definitive way to test if linux has enabled the msi capability, and > has the appropriate msi data structures set up. > > This patch ensures that while writing the msi messages in save/restore > and during device initialization we have the msi capability disabled > so we don't get into races. The pci spec requires that we do not have > the msi capability enabled and the msi messages unmasked while we write > the messages. Completely disabling the capability is overkill but it > is easy :) > > Care has been taken so we never have both a msi capability and intx > enabled simultaneously. We haven't run into a problem yet but better > safe then sorry.
Hi Eric, comments below ..
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index fd1068b..c43e7d2 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -38,6 +38,36 @@ static int msi_cache_init(void)
> return 0;
> }
>
> +static void msi_set_enable(struct pci_dev *dev, int enable)
> +{
> + int pos;
> + u16 control;
> +
> + pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> + if (pos) {
> + pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
> + control &= ~PCI_MSI_FLAGS_ENABLE;
> + if (enable)
> + control |= PCI_MSI_FLAGS_ENABLE;
> + pci_write_config_word(dev, pos + PCI_MSI_FLAGS, control);
> + }
> +}
> +
> +static void msix_set_enable(struct pci_dev *dev, int enable)
> +{
> + int pos;
> + u16 control;
> +
> + pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> + if (pos) {
> + pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
> + control &= ~PCI_MSIX_FLAGS_ENABLE;
> + if (enable)
> + control |= PCI_MSIX_FLAGS_ENABLE;
> + pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
> + }
> +}
> +
> static void msi_set_mask_bit(unsigned int irq, int flag)
> {
> struct msi_desc *entry;
> @@ -192,44 +222,6 @@ static struct msi_desc* alloc_msi_entry(void)
> return entry;
> }
>
> -static void enable_msi_mode(struct pci_dev *dev, int pos, int type)
> -{
> - u16 control;
> -
> - pci_read_config_word(dev, msi_control_reg(pos), &control);
> - if (type == PCI_CAP_ID_MSI) {
> - /* Set enabled bits to single MSI & enable MSI_enable bit */
> - msi_enable(control, 1);
> - pci_write_config_word(dev, msi_control_reg(pos), control);
> - dev->msi_enabled = 1;
> - } else {
> - msix_enable(control);
> - pci_write_config_word(dev, msi_control_reg(pos), control);
> - dev->msix_enabled = 1;
> - }
> -
> - pci_intx(dev, 0); /* disable intx */
> -}
> -
> -static void disable_msi_mode(struct pci_dev *dev, int pos, int type)
> -{
> - u16 control;
> -
> - pci_read_config_word(dev, msi_control_reg(pos), &control);
> - if (type == PCI_CAP_ID_MSI) {
> - /* Set enabled bits to single MSI & enable MSI_enable bit */
> - msi_disable(control);
> - pci_write_config_word(dev, msi_control_reg(pos), control);
> - dev->msi_enabled = 0;
> - } else {
> - msix_disable(control);
> - pci_write_config_word(dev, msi_control_reg(pos), control);
> - dev->msix_enabled = 0;
> - }
> -
> - pci_intx(dev, 1); /* enable intx */
> -}
> -
> #ifdef CONFIG_PM
> static int __pci_save_msi_state(struct pci_dev *dev)
> {
> @@ -238,12 +230,11 @@ static int __pci_save_msi_state(struct pci_dev *dev)
> struct pci_cap_saved_state *save_state;
> u32 *cap;
>
> - pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> - if (pos <= 0 || dev->no_msi)
> + if (!dev->msi_enabled)
> return 0;
>
> - pci_read_config_word(dev, msi_control_reg(pos), &control);
> - if (!(control & PCI_MSI_FLAGS_ENABLE))
> + pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> + if (pos <= 0)
> return 0;
>
> save_state = kzalloc(sizeof(struct pci_cap_saved_state) + sizeof(u32) *
> 5,
> @@ -276,13 +267,18 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
> struct pci_cap_saved_state *save_state;
> u32 *cap;
>
> + if (!dev->msi_enabled)
> + return;
> +
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_MSI);
> pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> if (!save_state || pos <= 0)
> return;
> cap = &save_state->data[0];
>
> + pci_intx(dev, 0); /* disable intx */
> control = cap[i++] >> 16;
> + msi_set_enable(dev, 0);
> pci_write_config_dword(dev, pos + PCI_MSI_ADDRESS_LO, cap[i++]);
> if (control & PCI_MSI_FLAGS_64BIT) {
> pci_write_config_dword(dev, pos + PCI_MSI_ADDRESS_HI, cap[i++]);
> @@ -292,7 +288,6 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
> if (control & PCI_MSI_FLAGS_MASKBIT)
> pci_write_config_dword(dev, pos + PCI_MSI_MASK_BIT, cap[i++]);
> pci_write_config_word(dev, pos + PCI_MSI_FLAGS, control);
> - enable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
> pci_remove_saved_cap(save_state);
> kfree(save_state);
> }
I get the reasoning for disabling MSI before we start writing back the
config space, but don't we want to re-enable MSI on the way out?
> @@ -308,13 +303,11 @@ static int __pci_save_msix_state(struct pci_dev *dev)
> return 0;
>
> pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> - if (pos <= 0 || dev->no_msi)
> + if (pos <= 0)
> return 0;
>
> /* save the capability */
> pci_read_config_word(dev, msi_control_reg(pos), &control);
> - if (!(control & PCI_MSIX_FLAGS_ENABLE))
> - return 0;
> save_state = kzalloc(sizeof(struct pci_cap_saved_state) + sizeof(u16),
> GFP_KERNEL);
> if (!save_state) {
> @@ -376,6 +369,8 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
> return;
>
> /* route the table */
> + pci_intx(dev, 0); /* disable intx */
> + msix_set_enable(dev, 0);
> irq = head = dev->first_msi_irq;
> while (head != tail) {
> entry = get_irq_msi(irq);
> @@ -386,7 +381,6 @@ static void __pci_restore_msix_state(struct pci_dev *dev)
> }
>
> pci_write_config_word(dev, msi_control_reg(pos), save);
> - enable_msi_mode(dev, pos, PCI_CAP_ID_MSIX);
> }
>
> void pci_restore_msi_state(struct pci_dev *dev)
Ditto here.
cheers
> @@ -411,6 +405,8 @@ static int msi_capability_init(struct pci_dev *dev)
> int pos, irq;
> u16 control;
>
> + msi_set_enable(dev, 0); /* Ensure msi is disabled as I set it up */
> +
> pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> pci_read_config_word(dev, msi_control_reg(pos), &control);
> /* MSI Entry Initialization */
> @@ -454,7 +450,9 @@ static int msi_capability_init(struct pci_dev *dev)
> set_irq_msi(irq, entry);
>
> /* Set MSI enabled bits */
> - enable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
> + pci_intx(dev, 0); /* disable intx */
> + msi_set_enable(dev, 1);
> + dev->msi_enabled = 1;
>
> dev->irq = irq;
> return 0;
> @@ -481,6 +479,8 @@ static int msix_capability_init(struct pci_dev *dev,
> u8 bir;
> void __iomem *base;
>
> + msix_set_enable(dev, 0);/* Ensure msix is disabled as I set it up */
> +
> pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> /* Request & Map MSI-X table region */
> pci_read_config_word(dev, msi_control_reg(pos), &control);
> @@ -549,7 +549,9 @@ static int msix_capability_init(struct pci_dev *dev,
> }
> dev->first_msi_irq = entries[0].vector;
> /* Set MSI-X enabled bits */
> - enable_msi_mode(dev, pos, PCI_CAP_ID_MSIX);
> + pci_intx(dev, 0); /* disable intx */
> + msix_set_enable(dev, 1);
> + dev->msix_enabled = 1;
>
> return 0;
> }
> @@ -611,12 +613,11 @@ int pci_enable_msi(struct pci_dev* dev)
> WARN_ON(!!dev->msi_enabled);
>
> /* Check whether driver already requested for MSI-X irqs */
> - pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> - if (pos > 0 && dev->msix_enabled) {
> - printk(KERN_INFO "PCI: %s: Can't enable MSI. "
> - "Device already has MSI-X enabled\n",
> - pci_name(dev));
> - return -EINVAL;
> + if (dev->msix_enabled) {
> + printk(KERN_INFO "PCI: %s: Can't enable MSI. "
> + "Device already has MSI-X enabled\n",
> + pci_name(dev));
> + return -EINVAL;
> }
> status = msi_capability_init(dev);
> return status;
> @@ -625,8 +626,7 @@ int pci_enable_msi(struct pci_dev* dev)
> void pci_disable_msi(struct pci_dev* dev)
> {
> struct msi_desc *entry;
> - int pos, default_irq;
> - u16 control;
> + int default_irq;
>
> if (!pci_msi_enable)
> return;
> @@ -636,16 +636,9 @@ void pci_disable_msi(struct pci_dev* dev)
> if (!dev->msi_enabled)
> return;
>
> - pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> - if (!pos)
> - return;
> -
> - pci_read_config_word(dev, msi_control_reg(pos), &control);
> - if (!(control & PCI_MSI_FLAGS_ENABLE))
> - return;
> -
> -
> - disable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
> + msi_set_enable(dev, 0);
> + pci_intx(dev, 1); /* enable intx */
> + dev->msi_enabled = 0;
>
> entry = get_irq_msi(dev->first_msi_irq);
> if (!entry || !entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI) {
> @@ -746,8 +739,7 @@ int pci_enable_msix(struct pci_dev* dev, struct
> msix_entry *entries, int nvec)
> WARN_ON(!!dev->msix_enabled);
>
> /* Check whether driver already requested for MSI irq */
> - if (pci_find_capability(dev, PCI_CAP_ID_MSI) > 0 &&
> - dev->msi_enabled) {
> + if (dev->msi_enabled) {
> printk(KERN_INFO "PCI: %s: Can't enable MSI-X. "
> "Device already has an MSI irq assigned\n",
> pci_name(dev));
> @@ -760,8 +752,6 @@ int pci_enable_msix(struct pci_dev* dev, struct
> msix_entry *entries, int nvec)
> void pci_disable_msix(struct pci_dev* dev)
> {
> int irq, head, tail = 0, warning = 0;
> - int pos;
> - u16 control;
>
> if (!pci_msi_enable)
> return;
> @@ -771,15 +761,9 @@ void pci_disable_msix(struct pci_dev* dev)
> if (!dev->msix_enabled)
> return;
>
> - pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> - if (!pos)
> - return;
> -
> - pci_read_config_word(dev, msi_control_reg(pos), &control);
> - if (!(control & PCI_MSIX_FLAGS_ENABLE))
> - return;
> -
> - disable_msi_mode(dev, pos, PCI_CAP_ID_MSIX);
> + msix_set_enable(dev, 0);
> + pci_intx(dev, 1); /* enable intx */
> + dev->msix_enabled = 0;
>
> irq = head = dev->first_msi_irq;
> while (head != tail) {
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
