Message ID | 1479223805-22895-13-git-send-email-kwankhede@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
* Kirti Wankhede <kwankhede@nvidia.com> [2016-11-15 20:59:55 +0530]: Hi Kirti, [...] > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > index ffc36758cb84..4fc63db38829 100644 > --- a/drivers/vfio/mdev/vfio_mdev.c > +++ b/drivers/vfio/mdev/vfio_mdev.c > @@ -24,6 +24,15 @@ > #define DRIVER_AUTHOR "NVIDIA Corporation" > #define DRIVER_DESC "VFIO based driver for Mediated device" > > +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); > + struct parent_device *parent = mdev->parent; > + > + return parent->ops->notifier(mdev, action, data); > +} > + > static int vfio_mdev_open(void *device_data) > { > struct mdev_device *mdev = device_data; > @@ -36,9 +45,18 @@ static int vfio_mdev_open(void *device_data) > if (!try_module_get(THIS_MODULE)) > return -ENODEV; > > + if (likely(parent->ops->notifier)) { > + mdev->nb.notifier_call = vfio_mdev_notifier; > + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) > + pr_err("Failed to register notifier for mdev\n"); I think we should just return here if the error value is not -ENOTTY. > + } > + > ret = parent->ops->open(mdev); > - if (ret) > + if (ret) { > + if (likely(parent->ops->notifier)) > + vfio_unregister_notifier(&mdev->dev, &mdev->nb); > module_put(THIS_MODULE); > + } > > return ret; > } [...]
On 11/16/2016 12:07 PM, Dong Jia Shi wrote: > * Kirti Wankhede <kwankhede@nvidia.com> [2016-11-15 20:59:55 +0530]: > > Hi Kirti, > > [...] > >> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c >> index ffc36758cb84..4fc63db38829 100644 >> --- a/drivers/vfio/mdev/vfio_mdev.c >> +++ b/drivers/vfio/mdev/vfio_mdev.c >> @@ -24,6 +24,15 @@ >> #define DRIVER_AUTHOR "NVIDIA Corporation" >> #define DRIVER_DESC "VFIO based driver for Mediated device" >> >> +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, >> + void *data) >> +{ >> + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); >> + struct parent_device *parent = mdev->parent; >> + >> + return parent->ops->notifier(mdev, action, data); >> +} >> + >> static int vfio_mdev_open(void *device_data) >> { >> struct mdev_device *mdev = device_data; >> @@ -36,9 +45,18 @@ static int vfio_mdev_open(void *device_data) >> if (!try_module_get(THIS_MODULE)) >> return -ENODEV; >> >> + if (likely(parent->ops->notifier)) { >> + mdev->nb.notifier_call = vfio_mdev_notifier; >> + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) >> + pr_err("Failed to register notifier for mdev\n"); > I think we should just return here if the error value is not -ENOTTY. > It might be the case where iommu backend module might not support .register_notifier(). In that case vfio_register_notifier() returns -ENOTTY and that should not fail this open() call Changing it to: ret = vfio_register_notifier(&mdev->dev, &mdev->nb); if (ret && (ret != -ENOTTY)) { pr_err("Failed to register notifier for mdev\n"); module_put(THIS_MODULE); return ret; } Thanks, Kirti
* Kirti Wankhede <kwankhede@nvidia.com> [2016-11-16 20:47:18 +0530]: > > > On 11/16/2016 12:07 PM, Dong Jia Shi wrote: > > * Kirti Wankhede <kwankhede@nvidia.com> [2016-11-15 20:59:55 +0530]: > > > > Hi Kirti, > > > > [...] > > > >> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > >> index ffc36758cb84..4fc63db38829 100644 > >> --- a/drivers/vfio/mdev/vfio_mdev.c > >> +++ b/drivers/vfio/mdev/vfio_mdev.c > >> @@ -24,6 +24,15 @@ > >> #define DRIVER_AUTHOR "NVIDIA Corporation" > >> #define DRIVER_DESC "VFIO based driver for Mediated device" > >> > >> +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, > >> + void *data) > >> +{ > >> + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); > >> + struct parent_device *parent = mdev->parent; > >> + > >> + return parent->ops->notifier(mdev, action, data); > >> +} > >> + > >> static int vfio_mdev_open(void *device_data) > >> { > >> struct mdev_device *mdev = device_data; > >> @@ -36,9 +45,18 @@ static int vfio_mdev_open(void *device_data) > >> if (!try_module_get(THIS_MODULE)) > >> return -ENODEV; > >> > >> + if (likely(parent->ops->notifier)) { > >> + mdev->nb.notifier_call = vfio_mdev_notifier; > >> + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) > >> + pr_err("Failed to register notifier for mdev\n"); > > I think we should just return here if the error value is not -ENOTTY. > > > > It might be the case where iommu backend module might not support > .register_notifier(). In that case vfio_register_notifier() returns > -ENOTTY and that should not fail this open() call > Changing it to: > > ret = vfio_register_notifier(&mdev->dev, &mdev->nb); > if (ret && (ret != -ENOTTY)) { > pr_err("Failed to register notifier for mdev\n"); > module_put(THIS_MODULE); > return ret; > } Nod. And we need not call vfio_unregister_notifier once error occurs in open() in this case. > > Thanks, > Kirti >
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index ffc36758cb84..4fc63db38829 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -24,6 +24,15 @@ #define DRIVER_AUTHOR "NVIDIA Corporation" #define DRIVER_DESC "VFIO based driver for Mediated device" +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, + void *data) +{ + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); + struct parent_device *parent = mdev->parent; + + return parent->ops->notifier(mdev, action, data); +} + static int vfio_mdev_open(void *device_data) { struct mdev_device *mdev = device_data; @@ -36,9 +45,18 @@ static int vfio_mdev_open(void *device_data) if (!try_module_get(THIS_MODULE)) return -ENODEV; + if (likely(parent->ops->notifier)) { + mdev->nb.notifier_call = vfio_mdev_notifier; + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) + pr_err("Failed to register notifier for mdev\n"); + } + ret = parent->ops->open(mdev); - if (ret) + if (ret) { + if (likely(parent->ops->notifier)) + vfio_unregister_notifier(&mdev->dev, &mdev->nb); module_put(THIS_MODULE); + } return ret; } @@ -51,6 +69,11 @@ static void vfio_mdev_release(void *device_data) if (likely(parent->ops->release)) parent->ops->release(mdev); + if (likely(parent->ops->notifier)) { + if (vfio_unregister_notifier(&mdev->dev, &mdev->nb)) + pr_err("Failed to unregister notifier for mdev\n"); + } + module_put(THIS_MODULE); } diff --git a/include/linux/mdev.h b/include/linux/mdev.h index ec819e9a115a..94c43034c297 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -37,6 +37,7 @@ struct mdev_device { struct kref ref; struct list_head next; struct kobject *type_kobj; + struct notifier_block nb; }; /** @@ -85,6 +86,12 @@ struct mdev_device { * @mmap: mmap callback * @mdev: mediated device structure * @vma: vma structure + * @notifer: Notifier callback, currently only for + * VFIO_IOMMU_NOTIFY_DMA_UNMAP action notified duing + * DMA_UNMAP call on mapped iova range. + * @mdev: mediated device structure + * @action: Action for which notifier is called + * @data: Data associated with the notifier * Parent device that support mediated device should be registered with mdev * module with parent_ops structure. **/ @@ -106,6 +113,8 @@ struct parent_ops { ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd, unsigned long arg); int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); + int (*notifier)(struct mdev_device *mdev, unsigned long action, + void *data); }; /* interface for exporting mdev supported type attributes */