Re: [RFC PATCH v2 1/3] PCI: endpoint: introduce a helper to implement pci ep virtio function

[Bjorn Helgaas]

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