RE: [PATCH v4 06/15] mei: pxp: support matching with a gfx discrete card

["Winkler, Tomas" <tomas.winkler@intel.com>]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Friday, September 09, 2022 09:16
> To: Ceraolo Spurio, Daniele <daniele.ceraolospurio@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> Winkler, Tomas <tomas.winkler@intel.com>; Lubart, Vitaly
> <vitaly.lubart@intel.com>; Teres Alexis, Alan Previn
> <alan.previn.teres.alexis@intel.com>
> Subject: Re: [PATCH v4 06/15] mei: pxp: support matching with a gfx discrete
> card
> 
> On Thu, Sep 08, 2022 at 05:16:03PM -0700, Daniele Ceraolo Spurio wrote:
> > From: Tomas Winkler <tomas.winkler@intel.com>
> >
> > With on-boards graphics card, both i915 and MEI are in the same device
> > hierarchy with the same parent, while for discrete gfx card the MEI is
> > its child device.
> > Adjust the match function for that scenario by matching MEI parent
> > device with i915.
> >
> > V2:
> >  1. More detailed commit message
> >  2. Check for dev is not null before it is accessed.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > Signed-off-by: Daniele Ceraolo Spurio
> > <daniele.ceraolospurio@intel.com>
> > Cc: Vitaly Lubart <vitaly.lubart@intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >  drivers/misc/mei/pxp/mei_pxp.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/misc/mei/pxp/mei_pxp.c
> > b/drivers/misc/mei/pxp/mei_pxp.c index 17c5d201603f..afc047627800
> > 100644
> > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > @@ -159,17 +159,24 @@ static int mei_pxp_component_match(struct
> device
> > *dev, int subcomponent,  {
> >  	struct device *base = data;
> >
> > +	if (!dev)
> > +		return 0;
> 
> How can that happen?
> 
> > +
> >  	if (!dev->driver || strcmp(dev->driver->name, "i915") ||
> 
> That's crazy to assume, but whatever :(
Explained here:
https://lore.kernel.org/all/20220418175932.1809770-2-wonchung@google.com/

> 
> >  	    subcomponent != I915_COMPONENT_PXP)
> >  		return 0;
> >
> >  	base = base->parent;
> > -	if (!base)
> > +	if (!base) /* mei device */
> 
> Why does this mean that?
> 
> Where is that documented?
> 
> >  		return 0;
> >
> > -	base = base->parent;
> > -	dev = dev->parent;
> > +	base = base->parent; /* pci device */
> 
> Again, why is this the case?
> 
> > +	/* for dgfx */
> > +	if (base && dev == base)
> > +		return 1;
> >
> > +	/* for pch */
> > +	dev = dev->parent;
> 
> You are digging through a random device tree and assuming that you "know"
> what the topology is going to be, that feels very very fragile and ripe for
> problems going forward.

I don't think it is random.

> How do you ensure that this really is they way the tree is for ALL systems?

Yes we take the topology assumption in PCI hierarchy.
There is a case where both GFX and MEI are in PCH and you cannot stick additional PCI extender or anything else there. 
And case where MEI is child on a standalone graphics card this is set in software so topology is not going to change unless we rewrite
everything.  Be happy to hear your insights. 
Thanks
Tomas