Re: [RFC PATCH v2 3/3] PCI: endpoint: Add EP function driver to provide virtio-console functionality

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

 



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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux