Re: [RFC PATCH v2 3/3] PCI: endpoint: Add EP function driver to provide virtio-console functionality
From: Shunsuke Mie
Date: Tue May 09 2023 - 21:25:41 EST
Hi Bjorn,
Thanks for the many comments. I will fix the mannerisms and typos as noted.
2023年4月28日(金) 3:09 Bjorn Helgaas <helgaas@xxxxxxxxxx>:
>
> Random typos and trivial things. No need to repost until somebody
> does a more substantive review.
>
> On Thu, Apr 27, 2023 at 07:44:28PM +0900, Shunsuke Mie wrote:
> > Add a new PCIe endpoint function driver that works as a pci virtio-console
> > device. The console connect to endpoint side console. It enables to
> > communicate PCIe host and endpoint.
>
> s/pci/PCI/
>
> > Architecture is following:
> >
> > ┌────────────┐ ┌──────────────────────┬────────────┐
> > │virtioe │ │ │virtio │
> > │console drv │ ├───────────────┐ │console drv │
> > ├────────────┤ │(virtio console│ ├────────────┤
> > │ virtio bus │ │ device) │◄────►│ virtio bus │
> > ├────────────┤ ├---------------┤ └────────────┤
> > │ │ │ pci ep virtio │ │
> > │ pci bus │ │ console drv │ │
> > │ │ pcie ├───────────────┤ │
> > │ │ ◄─────► │ pci ep Bus │ │
> > └────────────┘ └───────────────┴───────────────────┘
> > PCIe Root PCIe Endpoint
>
> s/virtioe/virtio/
> s/pci/PCI/
> s/pcie/PCIe/
> s/ep/EP/
>
> > +config PCI_EPF_VCON
> > + tristate "PCI Endpoint virito-console driver"
>
> s/virito/virtio/
>
> > + depends on PCI_ENDPOINT
> > + select VHOST_RING
> > + select PCI_EPF_VIRTIO
> > + help
> > + PCIe Endpoint virtio-console function implementatino. This module
> > + enables to show the virtio-console as pci device to PCIe host side, and
> > + another virtual virtio-console device registers to endpoint system.
> > + Those devices are connected virtually and can communicate each other.
>
> s/implementatino/implementation/
> s/pci/PCI/
> s/can communicate/can communicate with/
>
> > + * PCI Endpoint function driver to impliment virtio-console device
> > + * functionality.
>
> s/impliment/implement/
>
> > +static int virtio_queue_size = 0x100;
> > +module_param(virtio_queue_size, int, 0444);
> > +MODULE_PARM_DESC(virtio_queue_size, "A length of virtqueue");
>
> When and why would users need this parameter? Where is it documented?
>
> > + /* To access virtqueus of local host driver */
>
> s/virtqueus/virtqueues/
>
> > + /* To show a status whether this driver is ready and the remote is connected */
>
> Make this fit in 80 columns.
>
> > + /* This is a minimum implementation. Doesn't support any features of
> > + * virtio console. It means driver and device use just 2 virtuque for tx
> > + * and rx.
> > + */
>
> Use common multi-line comment style:
>
> /*
> * This is ...
> */
I'll follow the style.
> s/virtuque/virtqueues/
>
> > +static void epf_vcon_raise_irq_handler(struct work_struct *work)
> > +{
> > + struct epf_vcon *vcon =
> > + container_of(work, struct epf_vcon, raise_irq_work);
>
> Rewrap.
>
> > +static int epf_vcon_setup_common(struct epf_vcon *vcon)
> > +{
> > + vcon->features = 0;
> > + vcon->connected = false;
> > +
> > + vcon->task_wq =
> > + alloc_workqueue("pci-epf-vcon/task-wq",
>
> Looks like this would fit on the previous line?
>
> > + WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0);
>
> > +static void epf_vcon_initialize_complete(void *param)
> > +{
> > + struct epf_vcon *vcon = param;
> > +
> > + pr_debug("Remote host has connected\n");
>
> Is there any device info you could include here, e.g., with dev_dbg()?
> It's nice if users have a little context.
I see. I'll use it.
> > +static int epf_vcon_setup_ep_func(struct epf_vcon *vcon, struct pci_epf *epf)
> > +{
> > + int err;
> > + struct epf_virtio *evio = &vcon->evio;
> > + unsigned int nvq = epf_vcon_get_nvq(vcon);
> > +
> > + vcon->rdev_iovs =
> > + kmalloc_array(nvq, sizeof(vcon->rdev_iovs[0]), GFP_KERNEL);
>
> Move the function name and as many parameters as fit in 80 columns to
> the previous line to match prevailing style.
I've just applied clang-format... Ok, I'll fix it manually.
> > + /* There is no config for virtio console because this console device
> > + * doesn't any support features
> > + */
>
> Multi-line comment style.
>
> s/doesn't any support/doesn't support any/? Is that what you mean?
>
> > + /* Do nothing because this console device doesn't any support features */
>
> Same.
>
> > +static void epf_vcon_vdev_set_status(struct virtio_device *vdev, u8 status)
> > +{
> > + if (status & VIRTIO_CONFIG_S_FAILED)
> > + pr_debug("driver failed to setup this device\n");
>
> dev_dbg() if possible.
>
> > + err = vringh_init_kern(&vcon->vdev_vrhs[i], vcon->features,
> > + virtio_queue_size, false, vring->desc,
> > + vring->avail, vring->used);
> > + if (err) {
> > + pr_err("failed to init vringh for vring %d\n", i);
>
> dev_err() if possible.
>
> > +static int epf_vcon_setup_vdev(struct epf_vcon *vcon, struct device *parent)
> > +{
> > + int err;
> > + struct virtio_device *vdev = &vcon->vdev;
> > + const unsigned int nvq = epf_vcon_get_nvq(vcon);
> > +
> > + vcon->vdev_vrhs =
> > + kmalloc_array(nvq, sizeof(vcon->vdev_vrhs[0]), GFP_KERNEL);
>
> Rewrap.
>
> > + vcon->vdev_iovs =
> > + kmalloc_array(nvq, sizeof(vcon->vdev_iovs[0]), GFP_KERNEL);
>
> Rewrap.
>
> > + vcon->vdev_vqs =
> > + kmalloc_array(nvq, sizeof(vcon->vdev_vrhs[0]), GFP_KERNEL);
>
> Rewrap.
>
> > +static void epf_vcon_cleanup_vdev(struct epf_vcon *vcon)
> > +{
> > + unregister_virtio_device(&vcon->vdev);
> > + /* Cleanup struct virtio_device that has kobject, otherwise error occures when
> > + * reregister the virtio device.
> > + */
>
> Multi-line style and rewrap to fit in 80 columns.
>
> > +static int __init epf_vcon_init(void)
> > +{
> > + int err;
> > +
> > + err = pci_epf_register_driver(&epf_vcon_drv);
> > + if (err)
> > + pr_err("Failed to register PCI EP virtio-console function\n");
>
> dev_err() if possible (doesn't look like it *is* possible).
>
> Looks like this registers a *driver*, so maybe change the message from
> "function" to "driver"?
It should be "driver".
https://docs.kernel.org/PCI/endpoint/pci-endpoint.html#epc-apis-for-the-pci-endpoint-function-driver
> Bjorn
Best regards,
Shunsuke