Re: [PATCH v3 4/4] drivers/vfio: Enable VFIO if EEH is not supported

Gavin Shan
2014-08-07T02:46:36Z

On Thu, Aug 07, 2014 at 12:10:07PM +1000, Gavin Shan wrote:
>On Wed, Aug 06, 2014 at 11:05:43PM +1000, Alexey Kardashevskiy wrote:
>>On 08/06/2014 10:50 PM, Alex Williamson wrote:
>>> On Wed, 2014-08-06 at 19:49 +1000, Gavin Shan wrote:
>>>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> The existing vfio_pci_open() fails upon error returned from
>>>> vfio_spapr_pci_eeh_open(), which breaks POWER7's P5IOC2 PHB
>>>> support which this patch brings back.
>>>>
>>>> The patch fixes the issue by dropping the return value of
>>>> vfio_spapr_pci_eeh_open().
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>> ---
>>>> v3: Drop return value of vfio_spapr_pci_eeh_open()
>>>> ---
>>>> drivers/vfio/pci/vfio_pci.c | 6 +-----
>>>> drivers/vfio/vfio_spapr_eeh.c | 4 ++--
>>>> include/linux/vfio.h | 5 ++---
>>>> 3 files changed, 5 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>>> index e2ee80f..32d69c8 100644
>>>> --- a/drivers/vfio/pci/vfio_pci.c
>>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>>> @@ -178,11 +178,7 @@ static int vfio_pci_open(void *device_data)
>>>> if (ret)
>>>> goto error;
>>>>
>>>> - ret = vfio_spapr_pci_eeh_open(vdev->pdev);
>>>> - if (ret) {
>>>> - vfio_pci_disable(vdev);
>>>> - goto error;
>>>> - }
>>>> + vfio_spapr_pci_eeh_open(vdev->pdev);
>>>> }
>>>>
>>>> return 0;
>>>> diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
>>>> index 4779cac..86dfceb 100644
>>>> --- a/drivers/vfio/vfio_spapr_eeh.c
>>>> +++ b/drivers/vfio/vfio_spapr_eeh.c
>>>> @@ -19,9 +19,9 @@
>>>> #define DRIVER_DESC "VFIO IOMMU SPAPR EEH"
>>>>
>>>> /* We might build address mapping here for "fast" path later */
>>>> -int vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
>>>> +void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
>>>> {
>>>> - return eeh_dev_open(pdev);
>>>> + eeh_dev_open(pdev);
>>>
>>> Wasn't there some intent to provide a warning message, that would now be
>>> done here? Has that idea been dropped?
>>
>>
>>Comrade Gavin just forgot it :)
>>
>>Gavin, please add it. Thanks!
>>
>
>Sure, I'll add following warning message in eeh_dev_open() in
>separate patch in v4 in case nobody objects.
>
>int eeh_dev_open(struct pci_dev *pdev)
>{
> :
> /* No EEH device or PE ? */
> edev = pci_dev_to_eeh_dev(pdev);
> if (!edev || !edev->pe) {
> pr_warn_once("%s: Device %s not supported\n",
> __func__, pci_name(pdev));
> goto out;
> }
> :
>}
>

Well, I added the warning message in eeh_dev_open() and sending "v4" out.

Thanks,
Gavin

>
>>
>>
>>>
>>>> }
>>>> EXPORT_SYMBOL_GPL(vfio_spapr_pci_eeh_open);
>>>>
>>>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>>>> index 224128a..d320411 100644
>>>> --- a/include/linux/vfio.h
>>>> +++ b/include/linux/vfio.h
>>>> @@ -100,15 +100,14 @@ extern long vfio_external_check_extension(struct vfio_group *group,
>>>>
>>>> struct pci_dev;
>>>> #ifdef CONFIG_EEH
>>>> -extern int vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>>> +extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>>> extern void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
>>>> extern long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
>>>> unsigned int cmd,
>>>> unsigned long arg);
>>>> #else
>>>> -static inline int vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
>>>> +static inline void vfio_spapr_pci_eeh_open(struct pci_dev *pdev)
>>>> {
>>>> - return 0;
>>>> }
>>>>
>>>> static inline void vfio_spapr_pci_eeh_release(struct pci_dev *pdev)
>>>
>>>
>>>
>>
>>
>>--
>>Alexey
>>