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>
