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

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

i guess we are settled with this patch...

On Thu, 2023-04-20 at 15:04 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/18/2023 11:57 PM, Teres Alexis, Alan Previn wrote:
> > 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).
> 
> Will change
> 
alan: saw your reply on better alternative for both 'i' and 'd'

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.
> 
> AFAICS the lower level of the mei code do allow for size 0 for both send 
> and recv. Also, this is the same check as what we do for the PXP component.
alan: agreed - thus the nit as per my earlier email.

> > 
alan:snip

> > > +	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.
> 
> Those should always both be non-NULL, since both the mei and the GFX 
> device have the PCI bus as parent (and the previous check on pdev 
> ensures those are the 2 devices we're handling at this point).
alan: sounds good.



> > > +#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? 
[snip]
> AFAICS all other mei components use UUID_LE as well. The code was 
> updated from GUID to UUID_LE in:
> https://lore.kernel.org/all/20221228160558.21311-1-andriy.shevchenko@linux.intel.com/
alan: sounds good- thanks for the URL.