Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230427104428.862643-2-mie@igel.co.jp> <20230427182812.GA262399@bhelgaas>
In-Reply-To: <20230427182812.GA262399@bhelgaas>
From:   Shunsuke Mie <mie@igel.co.jp>
Date:   Wed, 10 May 2023 12:49:33 +0900
Message-ID: <CANXvt5oYq3CL9gPFZevAx30_Q=Ad1rsG2oCp7p9aNhNZaHvN+A@mail.gmail.com>
Subject: Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement
 pci ep virtio function
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        "Michael S. Tsirkin" <mst@redhat.com>, linux-pci@vger.kernel.org,
        Manivannan Sadhasivam <mani@kernel.org>,
        Frank Li <Frank.Li@nxp.com>, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        Ren Zhijie <renzhijie2@huawei.com>,
        Jon Mason <jdmason@kudzu.us>,
        Bjorn Helgaas <bhelgaas@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

I'll fix the typo and some styles you mentioned.
In this e-mail, I reply to the other comments.
2023=E5=B9=B44=E6=9C=8828=E6=97=A5(=E9=87=91) 3:28 Bjorn Helgaas <helgaas@k=
ernel.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 PC=
I
> > device. This patch introduces a setof helper functions and data structu=
res
> > 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, w=
hich
> > are not currently supported by either PCIe controllers/drivers or the P=
CIe
> > endpoint framework.
>
> s/implementation PCIe endpoint function/implementation of PCIe endpoint f=
unctions/
> 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 a=
nd
> > 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 =3D pci_epc_map_addr(epf->epc, epf->func_no, epf->vfunc_n=
o,
> > +                                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 vi=
rtio
> > + * @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 mem=
ories
> > + * @evio: struct epf_virtio to finalize.
>
> s/bar/BAR/
>
> > +static void epf_virtio_monitor_qnotify(struct epf_virtio *evio)
> > +{
> > +     const u16 qn_default =3D evio->nvq;
> > +     u16 tmp;
> > +
> > +     /* Since there is no way to synchronize between the host and EP f=
unctions,
> > +      * it is possible to miss multiple notifications.
>
> Multi-line comment style.
>
> > +     err =3D 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 =3D pci_epc_map_addr(epf->epc, epf->func_no,
> > +                                            epf->vfunc_no, dbase, &phy=
s,
> > +                                            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 =3D=3D 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, theref=
ore this
> > +      * callback function should check all of virtqueue's changes.
> > +      */
>
> Multi-line comment style.
>
> Bjorn

Best regards,
Shunsuke