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@xxxxxxxxxx>:
>
> 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
