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?)
I guess "legacy virtio" devices need not implement custom PCIe
capabilities, but "modern virtio" devices must implement them?
Capitalize "Endpoint framework" consistently; sometimes it's
"Endpoint", other times it's "endpoint".
> 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.
> + 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.
> +#define VIRTIO_PCI_LEGACY_CFG_BAR 0
What makes this a "legacy cfg BAR"? Is there some spec that covers
virtio BAR usage?
> + * 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.
> + 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.
> + 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?
> +#include <linux/pci-epf.h>
> +#include <linux/pci-epc.h>
> +#include <linux/vringh.h>
> +#include <linux/dmaengine.h>
Alpha order if possible
> + /* 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
