diff mbox

WARNING: CPU: 4 PID: 863 at include/drm/drm_crtc.h:1577 drm_helper_choose_encoder_dpms+0x88/0x90() - evildoer found and neutralized

Message ID 20150930173619.GA3826@pd.tnic
State New, archived
Headers show

Commit Message

Borislav Petkov Sept. 30, 2015, 5:36 p.m. UTC
On Thu, Oct 01, 2015 at 01:00:44AM +0800, Jiang Liu wrote:
> Thanks Joerg, that makes sense. If some driver tries to binding to
> the IOMMU device, it will trigger the scenario as you described. For
> example, Xen backend driver will try to probe all PCI devices if
> enabled. I will do more investigation tomorrow.

Right, so this fixes the issue on my box, courtesy of Joerg. WE
basically don't disable the IRQ on MSI-enabled devices. The AMD IOMMU
uses a barebones PCI device but not a PCI driver, which would be an
overkill.

---

Comments

Joerg Roedel Sept. 30, 2015, 6:07 p.m. UTC | #1
On Wed, Sep 30, 2015 at 07:36:19PM +0200, Borislav Petkov wrote:
> Right, so this fixes the issue on my box, courtesy of Joerg. WE
> basically don't disable the IRQ on MSI-enabled devices. The AMD IOMMU
> uses a barebones PCI device but not a PCI driver, which would be an
> overkill.

Well, not only overkill, but actually harmful. As I just wrote to
Jiang, a device can be forcibly unbound from its driver, which is
something we don't want for the IOMMU.


	Joerg
Jiang Liu Oct. 3, 2015, 7:36 a.m. UTC | #2
On 2015/10/1 1:36, Borislav Petkov wrote:
> On Thu, Oct 01, 2015 at 01:00:44AM +0800, Jiang Liu wrote:
>> Thanks Joerg, that makes sense. If some driver tries to binding to
>> the IOMMU device, it will trigger the scenario as you described. For
>> example, Xen backend driver will try to probe all PCI devices if
>> enabled. I will do more investigation tomorrow.
> 
> Right, so this fixes the issue on my box, courtesy of Joerg. WE
> basically don't disable the IRQ on MSI-enabled devices. The AMD IOMMU
> uses a barebones PCI device but not a PCI driver, which would be an
> overkill.
> 
> ---
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 09d3afc..29ec2eb 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -674,12 +674,15 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  int pcibios_alloc_irq(struct pci_dev *dev)
>  {
> +	if (pci_dev_msi_enabled(dev))
> +		return 0;
We may return -EBUSY here to reject the probe operation. It
doesn't make sense to continue the probe if MSI is already enabled,
tt also helps to avoid calling pcibios_free_irq() in function
pci_device_probe().

> +
>  	return pcibios_enable_irq(dev);
>  }
>  
>  void pcibios_free_irq(struct pci_dev *dev)
>  {
> -	if (pcibios_disable_irq)
> +	if (!pci_dev_msi_enabled(dev) && pcibios_disable_irq)
The above change is not needed, pcibios_disable_irq() will
first check !pci_has_managed_irq(dev) before actually freeing
PCI irq. pci_has_managed_irq(dev) only returns true if
pcibios_alloc_irq() succeeds.

So to summary, I think we only need following change to fix the
regression:
 int pcibios_alloc_irq(struct pci_dev *dev)
 {
+	if (pci_dev_msi_enabled(dev))
+		return -EBUSY;

What do you think?
Thanks!
Gerry

>  		pcibios_disable_irq(dev);
>  }
> --
>
Joerg Roedel Oct. 5, 2015, 10:03 a.m. UTC | #3
Hi Jiang,

On Sat, Oct 03, 2015 at 03:36:35PM +0800, Jiang Liu wrote:
> So to summary, I think we only need following change to fix the
> regression:
>  int pcibios_alloc_irq(struct pci_dev *dev)
>  {
> +	if (pci_dev_msi_enabled(dev))
> +		return -EBUSY;
> 
> What do you think?

Yes, that works too and has the added benefit that no driver can attach
to the iommu device and get in the way of the driver.

Will you send the patch for this change or should I do it?



	Joerg
Jiang Liu Oct. 6, 2015, 1:13 p.m. UTC | #4
On 2015/10/5 18:03, Joerg Roedel wrote:
> Hi Jiang,
> 
> On Sat, Oct 03, 2015 at 03:36:35PM +0800, Jiang Liu wrote:
>> So to summary, I think we only need following change to fix the
>> regression:
>>  int pcibios_alloc_irq(struct pci_dev *dev)
>>  {
>> +	if (pci_dev_msi_enabled(dev))
>> +		return -EBUSY;
>>
>> What do you think?
> 
> Yes, that works too and has the added benefit that no driver can attach
> to the iommu device and get in the way of the driver.
> 
> Will you send the patch for this change or should I do it?
Hi Joerg,
	We are on leave for Chinese National Holiday and has limited
access to my working environment. It would be appreciated if you could
help to send out a patch for it. Otherwise I will send out a patch
within 2-3 days.
Thanks!
Gerry
Joerg Roedel Oct. 9, 2015, 10:24 a.m. UTC | #5
On Tue, Oct 06, 2015 at 09:13:11PM +0800, Jiang Liu wrote:
> 	We are on leave for Chinese National Holiday and has limited
> access to my working environment. It would be appreciated if you could
> help to send out a patch for it. Otherwise I will send out a patch
> within 2-3 days.

Okay, I just sent the patch.


	Joerg
diff mbox

Patch

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 09d3afc..29ec2eb 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -674,12 +674,15 @@  int pcibios_add_device(struct pci_dev *dev)
 
 int pcibios_alloc_irq(struct pci_dev *dev)
 {
+	if (pci_dev_msi_enabled(dev))
+		return 0;
+
 	return pcibios_enable_irq(dev);
 }
 
 void pcibios_free_irq(struct pci_dev *dev)
 {
-	if (pcibios_disable_irq)
+	if (!pci_dev_msi_enabled(dev) && pcibios_disable_irq)
 		pcibios_disable_irq(dev);
 }
--