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 ...
*/
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.
> +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.
> + /* 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"?
Bjorn
