diff mbox

[RFC,2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

Message ID 1430968374-29286-3-git-send-email-jiang.liu@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jiang Liu May 7, 2015, 3:12 a.m. UTC
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(-)

Comments

Thomas Gleixner May 15, 2015, 9:02 p.m. UTC | #1
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
Bjorn Helgaas May 19, 2015, 3:08 p.m. UTC | #2
[+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
Michael S. Tsirkin May 19, 2015, 3:16 p.m. UTC | #3
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
Michael S. Tsirkin May 19, 2015, 3:26 p.m. UTC | #4
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
Bjorn Helgaas May 19, 2015, 9:39 p.m. UTC | #5
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
Jiang Liu May 20, 2015, 3:06 a.m. UTC | #6
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
Jiang Liu May 20, 2015, 3:07 a.m. UTC | #7
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
Jiang Liu May 20, 2015, 3:12 a.m. UTC | #8
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
Thomas Gleixner May 20, 2015, 7:47 a.m. UTC | #9
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 mbox

Patch

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)