Re: [RFC PATCH 1/7] vfio: Include optional device match in vfio_device_ops callbacks

From: Cornelia Huck
Date: Fri Feb 07 2020 - 04:33:52 EST


On Thu, 6 Feb 2020 11:18:42 -0700
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:

> On Thu, 6 Feb 2020 12:14:19 +0100
> Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
>
> > On Tue, 04 Feb 2020 16:05:43 -0700
> > Alex Williamson <alex.williamson@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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.