| Message ID | 158085754299.9445.4389176548645142886.stgit@gimli.home (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | vfio/pci: SR-IOV support | expand |
On Tue, 04 Feb 2020 16:05:43 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > Allow bus drivers to provide their own callback to match a device to > the user provided string. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/vfio/vfio.c | 19 +++++++++++++++---- > include/linux/vfio.h | 3 +++ > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 388597930b64..dda1726adda8 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -875,11 +875,22 @@ EXPORT_SYMBOL_GPL(vfio_device_get_from_dev); > static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group, > char *buf) > { > - struct vfio_device *it, *device = NULL; > + struct vfio_device *it, *device = ERR_PTR(-ENODEV); > > mutex_lock(&group->device_lock); > list_for_each_entry(it, &group->device_list, group_next) { > - if (!strcmp(dev_name(it->dev), buf)) { > + int ret; > + > + if (it->ops->match) { > + ret = it->ops->match(it->device_data, buf); > + if (ret < 0 && ret != -ENODEV) { > + device = ERR_PTR(ret); > + break; > + } > + } else > + ret = strcmp(dev_name(it->dev), buf); The asymmetric braces look a bit odd. > + > + if (!ret) { > device = it; > vfio_device_get(device); > break; > @@ -1441,8 +1452,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) > return -EPERM; > > device = vfio_device_get_from_name(group, buf); > - if (!device) > - return -ENODEV; > + if (IS_ERR(device)) > + return PTR_ERR(device); > > ret = device->ops->open(device->device_data); > if (ret) { > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index e42a711a2800..755e0f0e2900 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -26,6 +26,8 @@ > * operations documented below > * @mmap: Perform mmap(2) on a region of the device file descriptor > * @request: Request for the bus driver to release the device > + * @match: Optional device name match callback (return: 0 for match, -ENODEV > + * (or >0) for no match and continue, other -errno: no match and stop) I'm wondering about these conventions. If you basically want a tri-state return (match, don't match/continue, don't match/stop), putting -ENODEV and >0 together feels odd. I would rather expect either - < 0 == don't match/stop, 0 == don't match/continue, > 0 == match, or - 0 == match, -ENODEV (or some other defined error) == don't match/continue, all other values == don't match/abort? > */ > struct vfio_device_ops { > char *name; > @@ -39,6 +41,7 @@ struct vfio_device_ops { > unsigned long arg); > int (*mmap)(void *device_data, struct vm_area_struct *vma); > void (*request)(void *device_data, unsigned int count); > + int (*match)(void *device_data, char *buf); > }; > > extern struct iommu_group *vfio_iommu_group_get(struct device *dev); >
On Thu, 6 Feb 2020 12:14:19 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, 04 Feb 2020 16:05:43 -0700 > Alex Williamson <alex.williamson@redhat.com> wrote: > > > Allow bus drivers to provide their own callback to match a device to > > the user provided string. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > drivers/vfio/vfio.c | 19 +++++++++++++++---- > > include/linux/vfio.h | 3 +++ > > 2 files changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index 388597930b64..dda1726adda8 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -875,11 +875,22 @@ EXPORT_SYMBOL_GPL(vfio_device_get_from_dev); > > static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group, > > char *buf) > > { > > - struct vfio_device *it, *device = NULL; > > + struct vfio_device *it, *device = ERR_PTR(-ENODEV); > > > > mutex_lock(&group->device_lock); > > list_for_each_entry(it, &group->device_list, group_next) { > > - if (!strcmp(dev_name(it->dev), buf)) { > > + int ret; > > + > > + if (it->ops->match) { > > + ret = it->ops->match(it->device_data, buf); > > + if (ret < 0 && ret != -ENODEV) { > > + device = ERR_PTR(ret); > > + break; > > + } > > + } else > > + ret = strcmp(dev_name(it->dev), buf); > > The asymmetric braces look a bit odd. Ok > > + > > + if (!ret) { > > device = it; > > vfio_device_get(device); > > break; > > @@ -1441,8 +1452,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) > > return -EPERM; > > > > device = vfio_device_get_from_name(group, buf); > > - if (!device) > > - return -ENODEV; > > + if (IS_ERR(device)) > > + return PTR_ERR(device); > > > > ret = device->ops->open(device->device_data); > > if (ret) { > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index e42a711a2800..755e0f0e2900 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -26,6 +26,8 @@ > > * operations documented below > > * @mmap: Perform mmap(2) on a region of the device file descriptor > > * @request: Request for the bus driver to release the device > > + * @match: Optional device name match callback (return: 0 for match, -ENODEV > > + * (or >0) for no match and continue, other -errno: no match and stop) > > I'm wondering about these conventions. > > If you basically want a tri-state return (match, don't match/continue, > don't match/stop), putting -ENODEV and >0 together feels odd. I would > rather expect either > - < 0 == don't match/stop, 0 == don't match/continue, > 0 == match, or So sort of a bool + errno. I shied away from this because returning zero for success, or match, is such a common semantic, especially when we're replacing a simple strcmp(). I suppose it's logically just !strcmp() though, which avoids the abort case for a simple implementation like patch 2/7. > - 0 == match, -ENODEV (or some other defined error) == don't > match/continue, all other values == don't match/abort? This is closest to what I arrived at in this version, but I found it necessary to exclude positive values from the no-match/abort and consider them as no-match/continue because I didn't want to assume the errno for that case. I think with your first option we arrive at something like this: diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index dda1726adda8..b5609a411139 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -883,14 +883,15 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group, if (it->ops->match) { ret = it->ops->match(it->device_data, buf); - if (ret < 0 && ret != -ENODEV) { + if (ret < 0) { device = ERR_PTR(ret); break; } - } else - ret = strcmp(dev_name(it->dev), buf); + } else { + ret = !strcmp(dev_name(it->dev), buf); + } - if (!ret) { + if (ret) { device = it; vfio_device_get(device); break; diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 755e0f0e2900..029694b977f2 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -26,8 +26,9 @@ * operations documented below * @mmap: Perform mmap(2) on a region of the device file descriptor * @request: Request for the bus driver to release the device - * @match: Optional device name match callback (return: 0 for match, -ENODEV - * (or >0) for no match and continue, other -errno: no match and stop) + * @match: Optional device name match callback (return: 0 for no-match, >0 for + * match, -errno for abort (ex. match with insufficient or incorrect + * additional args) */ struct vfio_device_ops { char *name; I like that. Thanks, Alex
On Thu, 6 Feb 2020 11:18:42 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > On Thu, 6 Feb 2020 12:14:19 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Tue, 04 Feb 2020 16:05:43 -0700 > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > > > Allow bus drivers to provide their own callback to match a device to > > > the user provided string. > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > drivers/vfio/vfio.c | 19 +++++++++++++++---- > > > include/linux/vfio.h | 3 +++ > I think with your first option we arrive at something like this: > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index dda1726adda8..b5609a411139 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -883,14 +883,15 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group, > > if (it->ops->match) { > ret = it->ops->match(it->device_data, buf); > - if (ret < 0 && ret != -ENODEV) { > + if (ret < 0) { > device = ERR_PTR(ret); > break; > } > - } else > - ret = strcmp(dev_name(it->dev), buf); > + } else { > + ret = !strcmp(dev_name(it->dev), buf); > + } > > - if (!ret) { > + if (ret) { > device = it; > vfio_device_get(device); > break; > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 755e0f0e2900..029694b977f2 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -26,8 +26,9 @@ > * operations documented below > * @mmap: Perform mmap(2) on a region of the device file descriptor > * @request: Request for the bus driver to release the device > - * @match: Optional device name match callback (return: 0 for match, -ENODEV > - * (or >0) for no match and continue, other -errno: no match and stop) > + * @match: Optional device name match callback (return: 0 for no-match, >0 for > + * match, -errno for abort (ex. match with insufficient or incorrect > + * additional args) > */ > struct vfio_device_ops { > char *name; > > > I like that. Thanks, > > Alex Looks good to me.
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 388597930b64..dda1726adda8 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -875,11 +875,22 @@ EXPORT_SYMBOL_GPL(vfio_device_get_from_dev); static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group, char *buf) { - struct vfio_device *it, *device = NULL; + struct vfio_device *it, *device = ERR_PTR(-ENODEV); mutex_lock(&group->device_lock); list_for_each_entry(it, &group->device_list, group_next) { - if (!strcmp(dev_name(it->dev), buf)) { + int ret; + + if (it->ops->match) { + ret = it->ops->match(it->device_data, buf); + if (ret < 0 && ret != -ENODEV) { + device = ERR_PTR(ret); + break; + } + } else + ret = strcmp(dev_name(it->dev), buf); + + if (!ret) { device = it; vfio_device_get(device); break; @@ -1441,8 +1452,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) return -EPERM; device = vfio_device_get_from_name(group, buf); - if (!device) - return -ENODEV; + if (IS_ERR(device)) + return PTR_ERR(device); ret = device->ops->open(device->device_data); if (ret) { diff --git a/include/linux/vfio.h b/include/linux/vfio.h index e42a711a2800..755e0f0e2900 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -26,6 +26,8 @@ * operations documented below * @mmap: Perform mmap(2) on a region of the device file descriptor * @request: Request for the bus driver to release the device + * @match: Optional device name match callback (return: 0 for match, -ENODEV + * (or >0) for no match and continue, other -errno: no match and stop) */ struct vfio_device_ops { char *name; @@ -39,6 +41,7 @@ struct vfio_device_ops { unsigned long arg); int (*mmap)(void *device_data, struct vm_area_struct *vma); void (*request)(void *device_data, unsigned int count); + int (*match)(void *device_data, char *buf); }; extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
Allow bus drivers to provide their own callback to match a device to the user provided string. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/vfio/vfio.c | 19 +++++++++++++++---- include/linux/vfio.h | 3 +++ 2 files changed, 18 insertions(+), 4 deletions(-)