> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, September 09, 2022 09:16
> To: Ceraolo Spurio, Daniele <daniele.ceraolospurio@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> Winkler, Tomas <tomas.winkler@xxxxxxxxx>; Lubart, Vitaly
> <vitaly.lubart@xxxxxxxxx>; Teres Alexis, Alan Previn
> <alan.previn.teres.alexis@xxxxxxxxx>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Daniele Ceraolo Spurio
> > <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Vitaly Lubart <vitaly.lubart@xxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/
>
> > 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
