Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Add GSC proxy driver. It to allows messaging between GSC component
> on Intel on board graphics card and CSE device.
alan:nit: isn't "Intel integrated GPU" clearer than "Intel on-board graphics card"?
Same thing for the Kconfig description later (or am i missing something else here).

alan:snip



> +         MEI GSC proxy enables messaging between GSC service on
> +         Intel graphics on-board card and services on CSE (MEI)
alan:nit: same as above




> diff --git a/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
> new file mode 100644
> index 000000000000..953eda1a16fb
> --- /dev/null
> +++ b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022-2023 Intel Corporation
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/mei_cl_bus.h>
alan: [nit?] i thought we need to have the headers alphabetically ordered? (below too)
> +#include <linux/component.h>
> +#include <drm/drm_connector.h>
> +#include <drm/i915_component.h>
> +#include <drm/i915_gsc_proxy_mei_interface.h>

> +
> +/**
> + * mei_gsc_proxy_send - Sends a proxy message to ME FW.
> + * @dev: device corresponding to the mei_cl_device
> + * @buf: a message buffer to send
> + * @size: size of the message
> + * Return: bytes sent on Success, <0 on Failure
> + */
> +static int mei_gsc_proxy_send(struct device *dev, const void *buf, size_t size)
> +{
> +	ssize_t ret;
> +
> +	if (!dev || !buf)
alan: nit: not sure if we should be checking for !size here - i do see that next patch
is checking for this from i915 side of the interface... but just wasnt sure which is the prefered style
(in terms of where to check for this condition). Either way, its a nit for me since i traced down
all the way to mei_cl_alloc_cb and it looks like mei bus can tolerate zero sized messages.
> +		return -EINVAL;
alan:snip

> +static int mei_gsc_proxy_recv(struct device *dev, void *buf, size_t size)
> +{
> +	ssize_t ret;
> +
> +	if (!dev || !buf)
alan: nit: same as in the 'send' above,.. not sure if we should be checking for !size here...
or perhaps 0 sized recv is supported.

> +		return -EINVAL;
alan:snip

> +static int mei_gsc_proxy_component_match(struct device *dev, int subcomponent,
> +					 void *data)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!dev_is_pci(dev))
> +		return 0;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	if (pdev->class != (PCI_CLASS_DISPLAY_VGA << 8) ||
> +	    pdev->vendor != PCI_VENDOR_ID_INTEL)
> +		return 0;
> +
> +	if (subcomponent != I915_COMPONENT_GSC_PROXY)
> +		return 0;
> +
> +	return component_compare_dev(dev->parent, ((struct device *)data)->parent);
alan: do we care if both these parents are non-null? i notice in other mei component
drivers match functions we do check that.

> +}
> +
alan:snip

> +#define MEI_UUID_GSC_PROXY UUID_LE(0xf73db04, 0x97ab, 0x4125, \
> +				   0xb8, 0x93, 0xe9, 0x4, 0xad, 0xd, 0x54, 0x64)

alan: apologies for the newbie question, but why are we using UUID for the gsc_proxy
as opposed to GUID like the other mei components? i am not sure if i read the right
archived patch review but it sounded like GUID is for internal to kernel only whereas
UUID is for external too? (in which case why are we not using GUID for gsc-proxy?)
Consider this a non-blocking inquiry since i assume mei folks have reviewed this
prior and this is an explicit design decision that I'm just not versed on.

alan:snip