Message-ID: <CANXvt5oYq3CL9gPFZevAx30_Q=Ad1rsG2oCp7p9aNhNZaHvN+A@mail.gmail.com>
Date:   Wed, 10 May 2023 12:49:33 +0900
From:   Shunsuke Mie <mie@...l.co.jp>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Michael S. Tsirkin" <mst@...hat.com>, linux-pci@...r.kernel.org,
        Manivannan Sadhasivam <mani@...nel.org>,
        Frank Li <Frank.Li@....com>, linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        Ren Zhijie <renzhijie2@...wei.com>,
        Jon Mason <jdmason@...zu.us>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement
 pci ep virtio function

I'll fix the typo and some styles you mentioned.
In this e-mail, I reply to the other comments.
2023年4月28日(金) 3:28 Bjorn Helgaas <helgaas@...nel.org>:
>
> Simple typos, don't repost until there's more substantive feedback.
>
> On Thu, Apr 27, 2023 at 07:44:26PM +0900, Shunsuke Mie wrote:
> > The Linux PCIe Endpoint framework supports to implement PCIe endpoint
> > functions using a PCIe controller operating in endpoint mode.
> > It is possble to realize the behavior of PCIe device, such as virtio PCI
> > device. This patch introduces a setof helper functions and data structures
> > to implement a PCIe endpoint function that behaves as a virtio device.
>
> s/possble/possible/
> s/setof/set of/
>
> > Those functions enable the implementation PCIe endpoint function that
> > comply with the virtio lecacy specification. Because modern virtio
> > specifications require devices to implement custom PCIe capabilities, which
> > are not currently supported by either PCIe controllers/drivers or the PCIe
> > endpoint framework.
>
> s/implementation PCIe endpoint function/implementation of PCIe endpoint functions/
> s/lecacy/legacy/ (What does "legacy" mean?  Is there a spec for this?)
Yes, it is defined in spec.
> I guess "legacy virtio" devices need not implement custom PCIe
> capabilities, but "modern virtio" devices must implement them?
That I wanted to explain here.
> Capitalize "Endpoint framework" consistently; sometimes it's
> "Endpoint", other times it's "endpoint".
I'll take care to be consistent.
> > While this patch provides functions for negotiating with host drivers and
> > copying data, each PCIe function driver must impl ement operations that
> > depend on each specific device, such as network, block, etc.
>
> s/impl ement/implement/
>
> > +#include <linux/virtio_pci.h>
> > +#include <linux/virtio_config.h>
> > +#include <linux/kthread.h>
>
> Typically the header includes would be alphabetized if possible.
I'll try to reorder them.
> > +     vq_virt = pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_no,
> > +                                vq_pci_addr, vq_phys, vq_size);
> > +     if (IS_ERR(vq_virt)) {
> > +             pr_err("Failed to map virtuqueue to local");
>
> s/virtuqueue/virtqueue/
>
> I know you probably don't have any way to use dev_err(), but this
> message really needs some context, like a driver name and instance or
> something.
I'll try to use the dev_* function appropriately If possible.
> > +#define VIRTIO_PCI_LEGACY_CFG_BAR 0
>
> What makes this a "legacy cfg BAR"?  Is there some spec that covers
> virtio BAR usage?
Yes. Virtio Legacy interface defines the PCI BAR number to use.
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-14500010

> > + * epf_virtio_init - initialize struct epf_virtio and setup BAR for virtio
> > + * @evio: struct epf_virtio to initialize.
> > + * @hdr: pci configuration space to show remote host.
> > + * @bar_size: pci BAR size it depends on the virtio device type.
>
> s/pci/PCI/ (there were also a few more instances above in messages or
> comments)
>
> > + * epf_virtio_final - finalize struct epf_virtio. it frees bar and memories
> > + * @evio: struct epf_virtio to finalize.
>
> s/bar/BAR/
>
> > +static void epf_virtio_monitor_qnotify(struct epf_virtio *evio)
> > +{
> > +     const u16 qn_default = evio->nvq;
> > +     u16 tmp;
> > +
> > +     /* Since there is no way to synchronize between the host and EP functions,
> > +      * it is possible to miss multiple notifications.
>
> Multi-line comment style.
>
> > +     err = epf_virtio_negotiate_vq(evio);
> > +     if (err < 0) {
> > +             pr_err("failed to negoticate configs with driver\n");
>
> s/negoticate/negotiate/
>
> > + * epf_virtio_reset - reset virtio status
>
> Some of the function descriptions end with a period (".") and others
> don't.  Please figure out what the most common style is and use that
> consistently.
I agree. I'll fix to be consistent.
> > +                     dst = pci_epc_map_addr(epf->epc, epf->func_no,
> > +                                            epf->vfunc_no, dbase, &phys,
> > +                                            slen);
> > +                     if (IS_ERR(dst)) {
> > +                             pr_err("failed to map pci mmoery spact to local\n");
>
> s/pci/PCI/
> s/mmoery/memory/
> s/spact/space/ ?
>
> Also below.
>
> IIRC some previous messages started with a capital letter.  Please
> make them all consistent.
Sure.
> > +             if (dir == DMA_MEM_TO_DEV) {
> > +                     pci_epc_unmap_addr(epf->epc, epf->func_no,
> > +                                        epf->vfunc_no, phys, dst, slen);
> > +             } else {
> > +                     pci_epc_unmap_addr(epf->epc, epf->func_no,
> > +                                        epf->vfunc_no, phys, src, slen);
> > +             }
> > +     }
> > +
> > +     return 1;
>
> I guess this function returns either a negative error code or the
> value 1?  That seems sort of weird (I think "negative error code or
> *zero* is more typical), but maybe you're following some convention?
It has to be 0. It is my mistake.

Some vringh functions return 0 (data does not exist), 1 (it exists),
or error. But this functions is not related.
> > +#include <linux/pci-epf.h>
> > +#include <linux/pci-epc.h>
> > +#include <linux/vringh.h>
> > +#include <linux/dmaengine.h>
>
> Alpha order if possible
Yes.
> > +     /* Virtual address of pci configuration space */
>
> s/pci/PCI/
>
> > +     /* Callback function and parameter for queue notifcation
> > +      * Note: PCI EP function cannot detect qnotify accurately, therefore this
> > +      * callback function should check all of virtqueue's changes.
> > +      */
>
> Multi-line comment style.
>
> Bjorn

Best regards,
Shunsuke

Powered by blists - more mailing lists