| Message ID | 20230422010642.60720-3-brett.creeley@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pds_vfio driver | expand |
On Fri, Apr 21, 2023 at 06:06:37PM -0700, Brett Creeley wrote: > +static int > +pds_vfio_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ This indenting scheme is not kernel style. I generally suggest people run their code through clang-format and go through and take most of the changes. Most of what it sugges for this series is good This GNU style of left aligning the function name should not be in the kernel. Jason
On Fri, Apr 21, 2023 at 06:06:37PM -0700, Brett Creeley wrote: > +static const struct vfio_device_ops > +pds_vfio_ops = { > + .name = "pds-vfio", > + .init = pds_vfio_init_device, > + .release = vfio_pci_core_release_dev, > + .open_device = pds_vfio_open_device, > + .close_device = vfio_pci_core_close_device, > + .ioctl = vfio_pci_core_ioctl, > + .device_feature = vfio_pci_core_ioctl_feature, > + .read = vfio_pci_core_read, > + .write = vfio_pci_core_write, > + .mmap = vfio_pci_core_mmap, > + .request = vfio_pci_core_request, > + .match = vfio_pci_core_match, > + .bind_iommufd = vfio_iommufd_physical_bind, > + .unbind_iommufd = vfio_iommufd_physical_unbind, > + .attach_ioas = vfio_iommufd_physical_attach_ioas, > +}; > + > +const struct vfio_device_ops * > +pds_vfio_ops_info(void) > +{ > + return &pds_vfio_ops; > +} No reason for a function like this It is a bit strange to split up the driver files so the registration is in a different file than the ops implementation. Jason
On Thu, 4 May 2023 14:26:43 -0300 Jason Gunthorpe wrote: > This indenting scheme is not kernel style. I generally suggest people > run their code through clang-format and go through and take most of > the changes. Most of what it sugges for this series is good > > This GNU style of left aligning the function name should not be > in the kernel. FTR that's not a kernel-wide rule. Please scope your coding style suggestions to your subsystem, you may confuse people.
On Thu, May 04, 2023 at 01:20:01PM -0700, Jakub Kicinski wrote: > On Thu, 4 May 2023 14:26:43 -0300 Jason Gunthorpe wrote: > > This indenting scheme is not kernel style. I generally suggest people > > run their code through clang-format and go through and take most of > > the changes. Most of what it sugges for this series is good > > > > This GNU style of left aligning the function name should not be > > in the kernel. > > FTR that's not a kernel-wide rule. Please scope your coding style > suggestions to your subsystem, you may confuse people. It is what Documentation/process/coding-style.rst expects. It is good advice for new submitters to follow the common style guide consistently. The places that want different can ask for their differences during the first review, we don't need to confuse people with the reality that everything is an exception to someone somewhere. Jason
On Thu, 4 May 2023 19:41:54 -0300 Jason Gunthorpe wrote: > On Thu, May 04, 2023 at 01:20:01PM -0700, Jakub Kicinski wrote: > > On Thu, 4 May 2023 14:26:43 -0300 Jason Gunthorpe wrote: > > > This GNU style of left aligning the function name should not be > > > in the kernel. > > > > FTR that's not a kernel-wide rule. Please scope your coding style > > suggestions to your subsystem, you may confuse people. > > It is what Documentation/process/coding-style.rst expects. A reference to the section? You mean the vague mentions of the GNU coding style? That's just a tiny part of the GNU style, the separation of arg types is probably the most offensive. If the function declaration does not fit in 80 chars breaking the type off as a separate line is often a very reasonable choice. Anyway, I shouldn't complain, networking still has its odd rules. Probably why people making up rules for no strong reason is on my mind. > It is good advice for new submitters to follow the common style guide > consistently. The places that want different can ask for their > differences during the first review, we don't need to confuse people > with the reality that everything is an exception to someone somewhere. Yeah.
On Thu, May 04, 2023 at 04:40:05PM -0700, Jakub Kicinski wrote: > On Thu, 4 May 2023 19:41:54 -0300 Jason Gunthorpe wrote: > > On Thu, May 04, 2023 at 01:20:01PM -0700, Jakub Kicinski wrote: > > > On Thu, 4 May 2023 14:26:43 -0300 Jason Gunthorpe wrote: > > > > This GNU style of left aligning the function name should not be > > > > in the kernel. > > > > > > FTR that's not a kernel-wide rule. Please scope your coding style > > > suggestions to your subsystem, you may confuse people. > > > > It is what Documentation/process/coding-style.rst expects. > > A reference to the section? You mean the vague mentions of the GNU > coding style? Here I was looking at the "left indent the function name" - that is a GNUism. IIRC the justification is it makes it easy to find the function with 'grep "^func"' Coding style says: Descendants are always substantially shorter than the parent and are placed substantially to the right. A very commonly used style is to align descendants to a function open parenthesis. So this patch had things like this: +static void +pds_vfio_pci_remove(struct pci_dev *pdev) The first line is shorter than the second and the second is left not right placed. It doesn't even need wrapping. I know some people like to do this in some parts of the tree regardless of coding-style. The GNU idea is sort of reasonable after all. > If the function declaration does not fit in 80 chars breaking the type > off as a separate line is often a very reasonable choice. Reasonable yes, but not "common" :) > Anyway, I shouldn't complain, networking still has its odd rules. > Probably why people making up rules for no strong reason is on my mind. I usually try to ignore most of the style details, but this one stood out. When I checked the series against clang-format it was pretty good otherwise. A few minor fine tunings on some line wrapping :) Jason
On 5/4/2023 10:26 AM, Jason Gunthorpe wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Fri, Apr 21, 2023 at 06:06:37PM -0700, Brett Creeley wrote: > >> +static int >> +pds_vfio_pci_probe(struct pci_dev *pdev, >> + const struct pci_device_id *id) >> +{ > > This indenting scheme is not kernel style. I generally suggest people > run their code through clang-format and go through and take most of > the changes. Most of what it sugges for this series is good > > This GNU style of left aligning the function name should not be > in the kernel. > > Jason I will align and fix it in the next revision. Thanks for the review. Brett
On 5/4/2023 10:31 AM, Jason Gunthorpe wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Fri, Apr 21, 2023 at 06:06:37PM -0700, Brett Creeley wrote: > >> +static const struct vfio_device_ops >> +pds_vfio_ops = { >> + .name = "pds-vfio", >> + .init = pds_vfio_init_device, >> + .release = vfio_pci_core_release_dev, >> + .open_device = pds_vfio_open_device, >> + .close_device = vfio_pci_core_close_device, >> + .ioctl = vfio_pci_core_ioctl, >> + .device_feature = vfio_pci_core_ioctl_feature, >> + .read = vfio_pci_core_read, >> + .write = vfio_pci_core_write, >> + .mmap = vfio_pci_core_mmap, >> + .request = vfio_pci_core_request, >> + .match = vfio_pci_core_match, >> + .bind_iommufd = vfio_iommufd_physical_bind, >> + .unbind_iommufd = vfio_iommufd_physical_unbind, >> + .attach_ioas = vfio_iommufd_physical_attach_ioas, >> +}; >> + >> +const struct vfio_device_ops * >> +pds_vfio_ops_info(void) >> +{ >> + return &pds_vfio_ops; >> +} > > No reason for a function like this > > It is a bit strange to split up the driver files so the registration is in a > different file than the ops implementation. > > Jason I will fix this in the next revision. Thanks for the review. Brett
On 5/4/2023 10:31 AM, Jason Gunthorpe wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Fri, Apr 21, 2023 at 06:06:37PM -0700, Brett Creeley wrote: > >> +static const struct vfio_device_ops >> +pds_vfio_ops = { >> + .name = "pds-vfio", >> + .init = pds_vfio_init_device, >> + .release = vfio_pci_core_release_dev, >> + .open_device = pds_vfio_open_device, >> + .close_device = vfio_pci_core_close_device, >> + .ioctl = vfio_pci_core_ioctl, >> + .device_feature = vfio_pci_core_ioctl_feature, >> + .read = vfio_pci_core_read, >> + .write = vfio_pci_core_write, >> + .mmap = vfio_pci_core_mmap, >> + .request = vfio_pci_core_request, >> + .match = vfio_pci_core_match, >> + .bind_iommufd = vfio_iommufd_physical_bind, >> + .unbind_iommufd = vfio_iommufd_physical_unbind, >> + .attach_ioas = vfio_iommufd_physical_attach_ioas, >> +}; >> + >> +const struct vfio_device_ops * >> +pds_vfio_ops_info(void) >> +{ >> + return &pds_vfio_ops; >> +} > > No reason for a function like this > > It is a bit strange to split up the driver files so the registration is in a > different file than the ops implementation. The reason I did this was to separate the pci functionality from the vfio device functionality. There are other similar examples of uses like this. I ended up not changing this for v10 because it was intentional due to the reason I stated above. > > Jason
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile index 24c524224da5..45167be462d8 100644 --- a/drivers/vfio/pci/Makefile +++ b/drivers/vfio/pci/Makefile @@ -11,3 +11,5 @@ obj-$(CONFIG_VFIO_PCI) += vfio-pci.o obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5/ obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/ + +obj-$(CONFIG_PDS_VFIO_PCI) += pds/ diff --git a/drivers/vfio/pci/pds/Makefile b/drivers/vfio/pci/pds/Makefile new file mode 100644 index 000000000000..e1a55ae0f079 --- /dev/null +++ b/drivers/vfio/pci/pds/Makefile @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2023 Advanced Micro Devices, Inc. + +obj-$(CONFIG_PDS_VFIO_PCI) += pds_vfio.o + +pds_vfio-y := \ + pci_drv.o \ + vfio_dev.o diff --git a/drivers/vfio/pci/pds/pci_drv.c b/drivers/vfio/pci/pds/pci_drv.c new file mode 100644 index 000000000000..3856a4e78c8d --- /dev/null +++ b/drivers/vfio/pci/pds/pci_drv.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/types.h> +#include <linux/vfio.h> + +#include <linux/pds/pds_core_if.h> + +#include "vfio_dev.h" + +#define PDS_VFIO_DRV_DESCRIPTION "AMD/Pensando VFIO Device Driver" +#define PCI_VENDOR_ID_PENSANDO 0x1dd8 + +static int +pds_vfio_pci_probe(struct pci_dev *pdev, + const struct pci_device_id *id) +{ + struct pds_vfio_pci_device *pds_vfio; + int err; + + pds_vfio = vfio_alloc_device(pds_vfio_pci_device, vfio_coredev.vdev, + &pdev->dev, pds_vfio_ops_info()); + if (IS_ERR(pds_vfio)) + return PTR_ERR(pds_vfio); + + dev_set_drvdata(&pdev->dev, &pds_vfio->vfio_coredev); + + err = vfio_pci_core_register_device(&pds_vfio->vfio_coredev); + if (err) + goto out_put_vdev; + + return 0; + +out_put_vdev: + vfio_put_device(&pds_vfio->vfio_coredev.vdev); + return err; +} + +static void +pds_vfio_pci_remove(struct pci_dev *pdev) +{ + struct pds_vfio_pci_device *pds_vfio = pds_vfio_pci_drvdata(pdev); + + vfio_pci_core_unregister_device(&pds_vfio->vfio_coredev); + vfio_put_device(&pds_vfio->vfio_coredev.vdev); +} + +static const struct pci_device_id +pds_vfio_pci_table[] = { + { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_VENDOR_ID_PENSANDO, 0x1003) }, /* Ethernet VF */ + { 0, } +}; +MODULE_DEVICE_TABLE(pci, pds_vfio_pci_table); + +static struct pci_driver +pds_vfio_pci_driver = { + .name = KBUILD_MODNAME, + .id_table = pds_vfio_pci_table, + .probe = pds_vfio_pci_probe, + .remove = pds_vfio_pci_remove, + .driver_managed_dma = true, +}; + +module_pci_driver(pds_vfio_pci_driver); + +MODULE_DESCRIPTION(PDS_VFIO_DRV_DESCRIPTION); +MODULE_AUTHOR("Advanced Micro Devices, Inc."); +MODULE_LICENSE("GPL"); diff --git a/drivers/vfio/pci/pds/vfio_dev.c b/drivers/vfio/pci/pds/vfio_dev.c new file mode 100644 index 000000000000..0f70efec01e1 --- /dev/null +++ b/drivers/vfio/pci/pds/vfio_dev.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */ + +#include <linux/vfio.h> +#include <linux/vfio_pci_core.h> + +#include "vfio_dev.h" + +struct pds_vfio_pci_device * +pds_vfio_pci_drvdata(struct pci_dev *pdev) +{ + struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev); + + return container_of(core_device, struct pds_vfio_pci_device, + vfio_coredev); +} + +static int +pds_vfio_init_device(struct vfio_device *vdev) +{ + struct pds_vfio_pci_device *pds_vfio = + container_of(vdev, struct pds_vfio_pci_device, + vfio_coredev.vdev); + struct pci_dev *pdev = to_pci_dev(vdev->dev); + int err; + + err = vfio_pci_core_init_dev(vdev); + if (err) + return err; + + pds_vfio->vf_id = pci_iov_vf_id(pdev); + pds_vfio->pci_id = PCI_DEVID(pdev->bus->number, pdev->devfn); + + return 0; +} + +static int +pds_vfio_open_device(struct vfio_device *vdev) +{ + struct pds_vfio_pci_device *pds_vfio = + container_of(vdev, struct pds_vfio_pci_device, + vfio_coredev.vdev); + int err; + + err = vfio_pci_core_enable(&pds_vfio->vfio_coredev); + if (err) + return err; + + vfio_pci_core_finish_enable(&pds_vfio->vfio_coredev); + + return 0; +} + +static const struct vfio_device_ops +pds_vfio_ops = { + .name = "pds-vfio", + .init = pds_vfio_init_device, + .release = vfio_pci_core_release_dev, + .open_device = pds_vfio_open_device, + .close_device = vfio_pci_core_close_device, + .ioctl = vfio_pci_core_ioctl, + .device_feature = vfio_pci_core_ioctl_feature, + .read = vfio_pci_core_read, + .write = vfio_pci_core_write, + .mmap = vfio_pci_core_mmap, + .request = vfio_pci_core_request, + .match = vfio_pci_core_match, + .bind_iommufd = vfio_iommufd_physical_bind, + .unbind_iommufd = vfio_iommufd_physical_unbind, + .attach_ioas = vfio_iommufd_physical_attach_ioas, +}; + +const struct vfio_device_ops * +pds_vfio_ops_info(void) +{ + return &pds_vfio_ops; +} diff --git a/drivers/vfio/pci/pds/vfio_dev.h b/drivers/vfio/pci/pds/vfio_dev.h new file mode 100644 index 000000000000..66cfcab5b5bf --- /dev/null +++ b/drivers/vfio/pci/pds/vfio_dev.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */ + +#ifndef _VFIO_DEV_H_ +#define _VFIO_DEV_H_ + +#include <linux/pci.h> +#include <linux/vfio_pci_core.h> + +struct pds_vfio_pci_device { + struct vfio_pci_core_device vfio_coredev; + + int vf_id; + int pci_id; +}; + +const struct vfio_device_ops *pds_vfio_ops_info(void); +struct pds_vfio_pci_device *pds_vfio_pci_drvdata(struct pci_dev *pdev); + +#endif /* _VFIO_DEV_H_ */