| Message ID | 1430968374-29286-3-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, 7 May 2015, Jiang Liu wrote: > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate > when disabling MSI/MSI-X. This is a bit odd. With your proposed change we'll have: alloc_legacy_irq() msi[x]_enable() free_legacy_irq() msi[x]_disable() alloc_legacy_irq() And after that we shut down the device which will free the legacy irq again. Shouldn't we allocate the legacy irq only if we really need it? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Michael] On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote: > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate > when disabling MSI/MSI-X. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > drivers/pci/msi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index c3e7dfcf9ff5..47cf72c669f0 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) > msi_set_enable(dev, 1); > dev->msi_enabled = 1; > > + pcibios_free_irq(dev); From the recent discussion about disabling MSI at shutdown, I seem to recall that turning off bus mastering also effectively disables MSI and causes at least some devices to revert to using INTx. So with this patch, do we now have a problem in the following scenario? pci_enable_msi(dev); /* now frees INTx IRQ */ pci_clear_master(dev); <device interrupts> /* uses INTx since bus mastering disabled */ > dev->irq = entry->irq; > return 0; > } > @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev, > /* Set MSI-X enabled bits and unmask the function */ > pci_intx_for_msi(dev, 0); > dev->msix_enabled = 1; > - > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); > > + pcibios_free_irq(dev); > + > return 0; > > out_avail: > @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev) > > /* Restore dev->irq to its default pin-assertion irq */ > dev->irq = desc->msi_attrib.default_irq; > + pcibios_alloc_irq(dev); > } > > void pci_disable_msi(struct pci_dev *dev) > @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev) > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); > pci_intx_for_msi(dev, 1); > dev->msix_enabled = 0; > + pcibios_alloc_irq(dev); > } > > void pci_disable_msix(struct pci_dev *dev) > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 19, 2015 at 10:08:38AM -0500, Bjorn Helgaas wrote: > [+cc Michael] > > On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote: > > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't > > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. > > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate > > when disabling MSI/MSI-X. > > > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > > --- > > drivers/pci/msi.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > > index c3e7dfcf9ff5..47cf72c669f0 100644 > > --- a/drivers/pci/msi.c > > +++ b/drivers/pci/msi.c > > @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) > > msi_set_enable(dev, 1); > > dev->msi_enabled = 1; > > > > + pcibios_free_irq(dev); > > >From the recent discussion about disabling MSI at shutdown, I seem to > recall that turning off bus mastering also effectively disables MSI and > causes at least some devices to revert to using INTx. > > So with this patch, do we now have a problem in the following scenario? Can't say, I didn't follow the discussion. But I'll address the comment below: > pci_enable_msi(dev); /* now frees INTx IRQ */ > pci_clear_master(dev); > <device interrupts> /* uses INTx since bus mastering disabled */ Not really, this does not use INTx. Disabling bus master shuts down MSI but does not enable INTx, you effectively get no interrupts at all: MSI disabled INTx and bus master disabled MSI. > > dev->irq = entry->irq; > > return 0; > > } > > @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev, > > /* Set MSI-X enabled bits and unmask the function */ > > pci_intx_for_msi(dev, 0); > > dev->msix_enabled = 1; > > - > > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); > > > > + pcibios_free_irq(dev); > > + > > return 0; > > > > out_avail: > > @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev) > > > > /* Restore dev->irq to its default pin-assertion irq */ > > dev->irq = desc->msi_attrib.default_irq; > > + pcibios_alloc_irq(dev); > > } > > > > void pci_disable_msi(struct pci_dev *dev) > > @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev) > > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); > > pci_intx_for_msi(dev, 1); > > dev->msix_enabled = 0; > > + pcibios_alloc_irq(dev); > > } > > > > void pci_disable_msix(struct pci_dev *dev) > > -- > > 1.7.10.4 > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 19, 2015 at 05:16:54PM +0200, Michael S. Tsirkin wrote: > On Tue, May 19, 2015 at 10:08:38AM -0500, Bjorn Helgaas wrote: > > [+cc Michael] > > > > On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote: > > > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't > > > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. > > > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate > > > when disabling MSI/MSI-X. > > > > > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > > > --- > > > drivers/pci/msi.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > > > index c3e7dfcf9ff5..47cf72c669f0 100644 > > > --- a/drivers/pci/msi.c > > > +++ b/drivers/pci/msi.c > > > @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) > > > msi_set_enable(dev, 1); > > > dev->msi_enabled = 1; > > > > > > + pcibios_free_irq(dev); > > > > >From the recent discussion about disabling MSI at shutdown, I seem to > > recall that turning off bus mastering also effectively disables MSI and > > causes at least some devices to revert to using INTx. So to stress this point, turning off bus mastering also effectively disables MSI without making devices revert to using INTx. This is why clearing bus master is the right thing to do if you don't want any interrupts at all. > > So with this patch, do we now have a problem in the following scenario? > > Can't say, I didn't follow the discussion. But I'll address the > comment below: > > > pci_enable_msi(dev); /* now frees INTx IRQ */ > > pci_clear_master(dev); > > <device interrupts> /* uses INTx since bus mastering disabled */ > > Not really, this does not use INTx. Disabling bus master shuts down MSI > but does not enable INTx, you effectively get no interrupts at all: MSI > disabled INTx and bus master disabled MSI. > > > > > dev->irq = entry->irq; > > > return 0; > > > } > > > @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev, > > > /* Set MSI-X enabled bits and unmask the function */ > > > pci_intx_for_msi(dev, 0); > > > dev->msix_enabled = 1; > > > - > > > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); > > > > > > + pcibios_free_irq(dev); > > > + > > > return 0; > > > > > > out_avail: > > > @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev) > > > > > > /* Restore dev->irq to its default pin-assertion irq */ > > > dev->irq = desc->msi_attrib.default_irq; > > > + pcibios_alloc_irq(dev); > > > } > > > > > > void pci_disable_msi(struct pci_dev *dev) > > > @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev) > > > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); > > > pci_intx_for_msi(dev, 1); > > > dev->msix_enabled = 0; > > > + pcibios_alloc_irq(dev); > > > } > > > > > > void pci_disable_msix(struct pci_dev *dev) > > > -- > > > 1.7.10.4 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote: > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate > when disabling MSI/MSI-X. The rest of this series makes sense to me. If you want to remove an IOAPIC, you want to make sure all of the IRQs using that IOAPIC have been freed. But I'm trying to figure out this patch. Do you want to free the IRQ when enabling MSI because it enables you to remove the IOAPIC without removing the device? That wouldn't really make sense to me because then the device has no possibility of using INTx. Bjorn > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > drivers/pci/msi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index c3e7dfcf9ff5..47cf72c669f0 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) > msi_set_enable(dev, 1); > dev->msi_enabled = 1; > > + pcibios_free_irq(dev); > dev->irq = entry->irq; > return 0; > } > @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev, > /* Set MSI-X enabled bits and unmask the function */ > pci_intx_for_msi(dev, 0); > dev->msix_enabled = 1; > - > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); > > + pcibios_free_irq(dev); > + > return 0; > > out_avail: > @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev) > > /* Restore dev->irq to its default pin-assertion irq */ > dev->irq = desc->msi_attrib.default_irq; > + pcibios_alloc_irq(dev); > } > > void pci_disable_msi(struct pci_dev *dev) > @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev) > msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); > pci_intx_for_msi(dev, 1); > dev->msix_enabled = 0; > + pcibios_alloc_irq(dev); > } > > void pci_disable_msix(struct pci_dev *dev) > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/5/16 5:02, Thomas Gleixner wrote: > On Thu, 7 May 2015, Jiang Liu wrote: > >> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't >> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. >> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate >> when disabling MSI/MSI-X. > > This is a bit odd. With your proposed change we'll have: > > alloc_legacy_irq() > > msi[x]_enable() > free_legacy_irq() > > msi[x]_disable() > alloc_legacy_irq() Hi Thomas, It's for safety. I'm not sure whether the device driver will make use of legacy IRQ after calling msi[x]_disable(). I have concerns about following pattern in PCI device drivers: --------------------------------------------------- if (enable_msi() == SUCCESS) { if (allocate_resource_for_msi() == SUCCESS) return; disable_msi(); } use_legacy_irq() Thanks! Gerry > > And after that we shut down the device which will free the legacy irq > again. > > Shouldn't we allocate the legacy irq only if we really need it? > > Thanks, > > tglx > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/5/19 23:26, Michael S. Tsirkin wrote: > On Tue, May 19, 2015 at 05:16:54PM +0200, Michael S. Tsirkin wrote: >> On Tue, May 19, 2015 at 10:08:38AM -0500, Bjorn Helgaas wrote: >>> [+cc Michael] >>> >>> On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote: >>>> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't >>>> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. >>>> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate >>>> when disabling MSI/MSI-X. >>>> >>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >>>> --- >>>> drivers/pci/msi.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c >>>> index c3e7dfcf9ff5..47cf72c669f0 100644 >>>> --- a/drivers/pci/msi.c >>>> +++ b/drivers/pci/msi.c >>>> @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) >>>> msi_set_enable(dev, 1); >>>> dev->msi_enabled = 1; >>>> >>>> + pcibios_free_irq(dev); >>> >>> >From the recent discussion about disabling MSI at shutdown, I seem to >>> recall that turning off bus mastering also effectively disables MSI and >>> causes at least some devices to revert to using INTx. > > So to stress this point, turning off bus mastering also effectively > disables MSI without making devices revert to using INTx. > This is why clearing bus master is the right thing to do > if you don't want any interrupts at all. Thanks for explanation, Michael:) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/5/20 5:39, Bjorn Helgaas wrote: > On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote: >> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't >> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. >> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate >> when disabling MSI/MSI-X. > > The rest of this series makes sense to me. If you want to remove an > IOAPIC, you want to make sure all of the IRQs using that IOAPIC have been > freed. > > But I'm trying to figure out this patch. Do you want to free the IRQ when > enabling MSI because it enables you to remove the IOAPIC without removing > the device? That wouldn't really make sense to me because then the device > has no possibility of using INTx. Hi Bjorn Sorry for the confusion. This patch is not for IOAPIC hot-removal, but it's an effort to use IRQ resource precisely. The idea is to allocate IRQ resource on demand and release it when not used. We could skip this patch if it's not worth the cost. Thanks! Gerry -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 20 May 2015, Jiang Liu wrote: > On 2015/5/16 5:02, Thomas Gleixner wrote: > > This is a bit odd. With your proposed change we'll have: > > > > alloc_legacy_irq() > > > > msi[x]_enable() > > free_legacy_irq() > > > > msi[x]_disable() > > alloc_legacy_irq() > Hi Thomas, > It's for safety. I'm not sure whether the device driver will > make use of legacy IRQ after calling msi[x]_disable(). I have concerns > about following pattern in PCI device drivers: > --------------------------------------------------- > if (enable_msi() == SUCCESS) { > if (allocate_resource_for_msi() == SUCCESS) > return; > disable_msi(); > } > use_legacy_irq() Fair enough. tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index c3e7dfcf9ff5..47cf72c669f0 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) msi_set_enable(dev, 1); dev->msi_enabled = 1; + pcibios_free_irq(dev); dev->irq = entry->irq; return 0; } @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev, /* Set MSI-X enabled bits and unmask the function */ pci_intx_for_msi(dev, 0); dev->msix_enabled = 1; - msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0); + pcibios_free_irq(dev); + return 0; out_avail: @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev) /* Restore dev->irq to its default pin-assertion irq */ dev->irq = desc->msi_attrib.default_irq; + pcibios_alloc_irq(dev); } void pci_disable_msi(struct pci_dev *dev) @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev) msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0); pci_intx_for_msi(dev, 1); dev->msix_enabled = 0; + pcibios_alloc_irq(dev); } void pci_disable_msix(struct pci_dev *dev)
Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again. So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate when disabling MSI/MSI-X. Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- drivers/pci/msi.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)