LKML Archive mirror
 help / color / mirror / Atom feed
* [Bug] altera_cvp registers a PCI device (Altera/Intel FPGA) without verifying that it supports CVP
@ 2018-10-19 15:53 Andreas Puhm
  2018-10-22 10:33 ` Moritz Fischer
  0 siblings, 1 reply; 2+ messages in thread
From: Andreas Puhm @ 2018-10-19 15:53 UTC (permalink / raw
  To: Alan Tull, Moritz Fischer
  Cc: linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org

Hello,

I hope the following information is descriptive enough.
If this is no the case, I will provide further details.

--------------------------------------------------------------------
Full description:
The altera_cvp probe function only checks, 
if the Altera/Intel PCI device configuration space contains a vendor specific entry (VSEC Capability Header 0x000b) at offset 0x200.
 But the probe function does not verify, if the PCI device (and further the FPGA), 
for which it has been called, actually supports the Configure-via-Protocol feature.

The PCI device (FPGA) can explicitly disable the Configur-via-Protocol (CvP) feature by setting the CVP_EN bit, index 20 of CVP_STATUS register, to '0'. 
As the altera_cvp probe function does not check this it registers the device in any way. 
At this point, the altera_cvp module cannot be used to program this device via CvP. 
In addition no other module can use the device, as it is still registered by altera_cvp.

Keywords: altera_cvp module, PCI, Configure-via-Protocol

Kernel version: problem occured with v4.15, should occur from 4.14+

Instructions to reproduce: 
Proper hardware is necessary to reproduce this, i.e., FPGA with instantiated Altera/Intel PCIe IP Core with disabled CvP feature.

Workaround:
It is possible to circumvent this problem by manually unloading or blacklisting the altera_cvp module.

Suggested Patch:
This patch was successfully build and tested for 4.15 and 4.18

Subject: [PATCH] register device only, if it supports CvP operation

This patch is based on: git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tag/?h=v4.18

Signed-off-by: Andreas Puhm <puhm@oregano.at>
---
 drivers/fpga/altera-cvp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 7fa793672a7a..838abcfca0fb 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -403,6 +403,7 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 	struct altera_cvp_conf *conf;
 	struct fpga_manager *mgr;
 	u16 cmd, val;
+	u32 val32;
 	int ret;
 
 	/*
@@ -416,6 +417,14 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 		return -ENODEV;
 	}
 
+	pci_read_config_dword(pdev, VSE_CVP_STATUS, &val32);
+	if (!(val32 & VSE_CVP_STATUS_CVP_EN)) {
+		dev_err(&pdev->dev,
+			"CVP is disabled for this device: CVP_STATUS Reg 0x%x\n",
+			val32);
+		return -ENODEV;
+	}
+
 	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
 	if (!conf)
 		return -ENOMEM;
--

With best regards,
Andreas Puhm <puhm@oregano.at>

^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [Bug] altera_cvp registers a PCI device (Altera/Intel FPGA) without verifying that it supports CVP
  2018-10-19 15:53 [Bug] altera_cvp registers a PCI device (Altera/Intel FPGA) without verifying that it supports CVP Andreas Puhm
@ 2018-10-22 10:33 ` Moritz Fischer
  0 siblings, 0 replies; 2+ messages in thread
From: Moritz Fischer @ 2018-10-22 10:33 UTC (permalink / raw
  To: Andreas Puhm
  Cc: Alan Tull, Moritz Fischer, linux-fpga@vger.kernel.org,
	linux-kernel@vger.kernel.org

Hi Andreas,

On Fri, Oct 19, 2018 at 03:53:02PM +0000, Andreas Puhm wrote:
> Hello,
> 
> I hope the following information is descriptive enough.
> If this is no the case, I will provide further details.
> 
> --------------------------------------------------------------------
> Full description:
> The altera_cvp probe function only checks, 
> if the Altera/Intel PCI device configuration space contains a vendor specific entry (VSEC Capability Header 0x000b) at offset 0x200.
>  But the probe function does not verify, if the PCI device (and further the FPGA), 
> for which it has been called, actually supports the Configure-via-Protocol feature.
> 
> The PCI device (FPGA) can explicitly disable the Configur-via-Protocol (CvP) feature by setting the CVP_EN bit, index 20 of CVP_STATUS register, to '0'. 
> As the altera_cvp probe function does not check this it registers the device in any way. 
> At this point, the altera_cvp module cannot be used to program this device via CvP. 
> In addition no other module can use the device, as it is still registered by altera_cvp.
> 
> Keywords: altera_cvp module, PCI, Configure-via-Protocol
> 
> Kernel version: problem occured with v4.15, should occur from 4.14+
> 
> Instructions to reproduce: 
> Proper hardware is necessary to reproduce this, i.e., FPGA with instantiated Altera/Intel PCIe IP Core with disabled CvP feature.
> 
> Workaround:
> It is possible to circumvent this problem by manually unloading or blacklisting the altera_cvp module.
> 
> Suggested Patch:
> This patch was successfully build and tested for 4.15 and 4.18
> 
> Subject: [PATCH] register device only, if it supports CvP operation

Could you make this: 'fpga: altera_cvp: Conditionalize registration' or
something along those lines?
> 
> This patch is based on: git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tag/?h=v4.18

Put some of the extensive description you made above in the git commit
message, please, such that future us will know why the change was made
:)

Thanks for your patch,

Moritz

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-10-22 10:33 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-19 15:53 [Bug] altera_cvp registers a PCI device (Altera/Intel FPGA) without verifying that it supports CVP Andreas Puhm
2018-10-22 10:33 ` Moritz Fischer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).