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:
>
>> > +#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.
>>
>> > +{
>> > + 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.
>> > + /* 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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
