On Thu, May 22, 2014 at 07:56:31AM +1000, Benjamin Herrenschmidt wrote:

On Wed, 2014-05-21 at 15:07 +0200, Alexander Graf wrote:

quoted

quoted

+#ifdef CONFIG_VFIO_PCI_EEH
+int eeh_vfio_open(struct pci_dev *pdev)

Why vfio? Also that config option will not be set if vfio is compiled as 
a module.

quoted

+{
+	struct eeh_dev *edev;
+
+	/* No PCI device ? */
+	if (!pdev)
+		return -ENODEV;
+
+	/* No EEH device ? */
+	edev = pci_dev_to_eeh_dev(pdev);
+	if (!edev || !edev->pe)
+		return -ENODEV;
+
+	eeh_dev_set_passed(edev, true);
+	eeh_pe_set_passed(edev->pe, true);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(eeh_vfio_open);

Additionally, shouldn't we have some locking here ? (and in release too)

I don't like relying on the caller locking (if it does it at all).

Ok. I'll add one mutex for open() and release() in next revision.
Thanks for the comment.

quoted

quoted

+	/* Device existing ? */
+	ret = eeh_vfio_check_dev(pdev, &edev, &pe);
+	if (ret) {
+		pr_debug("%s: Cannot find device %s\n",
+			__func__, pdev ? pci_name(pdev) : "NULL");
+		*retval = -7;

What are these? Please use proper kernel internal return values for 
errors. I don't want to see anything even remotely tied to RTAS in any 
of these patches.

Hint: -ENODEV

In next revision, Those exported functions will have return value as:

= 0:	carrried information to caller.

<  0:   error number.

Thanks,
Gavin