Re: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 10, 2021 at 4:58 AM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> On Tue, Mar 09, 2021 at 09:52:30PM -0800, Dan Williams wrote:
> > On Tue, Mar 9, 2021 at 1:39 PM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> > >
> > > This is the standard kernel pattern, the ops associated with a struct get
> > > the struct pointer in for typesafety. The expected design is to use
> > > container_of to cleanly go from the subsystem level type to the driver
> > > level type without having any type erasure in a void *.
> >
> > This patch alone is worth the price of admission.
>
> Right, this is how I got here as I was going to do the mdev stuff and
> we end up doing transformations like this:
>
> -static long mtty_ioctl(struct mdev_device *mdev, unsigned int cmd,
> +static long mtty_ioctl(struct vfio_device *vdev, unsigned int cmd,
>                         unsigned long arg)
>
> And if the 'struct vfio_device *vdev' was left a 'void *' then the
> compiler doesn't get to help any more :(
>
> > Seems like it would be worth adding
> > to_vfio_{pci,platform,fsl_mc}_device() helpers in this patch as well.
>
> I have mixed feelings on these one-liners. If people feel they are
> worthwhile I'll add them

It's only worthwhile in my opinion if it makes the diffstat more
favorable for lines removed.

>
> > I've sometimes added runtime type safety to to_* helpers for early
> > warning of mistakes that happen when refactoring...
> >
> > static inline struct vfio_pci_device *
> > to_vfio_pci_device(struct vfio_device *core_dev)
> > {
> >         if (dev_WARN_ONCE(core_dev->dev, core_dev->ops != &vfio_pci_ops,
> >                           "not a vfio_pci_device!\n"))
> >                 return NULL;
> >         return container_of(core_vdev, struct vfio_pci_device, vdev);
>
> In this case I don't think we need to worry as everything is tidy in a
> single module such that the compilation units can't see other
> container_of options anyhow and vfio core isn't going to accidently
> call an ops with the wrong type.

Sounds good.

You can add:

Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux