> -----Original Message-----
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Friday, September 09, 2022 09:16
> To: Ceraolo Spurio, Daniele <[email protected]>
> Cc: [email protected]; [email protected];
> Winkler, Tomas <[email protected]>; Lubart, Vitaly
> <[email protected]>; Teres Alexis, Alan Previn
> <[email protected]>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Daniele Ceraolo Spurio
> > <[email protected]>
> > Cc: Vitaly Lubart <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Reviewed-by: Alan Previn <[email protected]>
> > ---
> > 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/[email protected]/
>
> > 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
