RE: [char-misc-next] mei: gsc_proxy: add gsc proxy driver

[Winkler, Tomas]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, January 19, 2023 18:21
> To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>
> Cc: Winkler, Tomas <tomas.winkler@xxxxxxxxx>; Lubart, Vitaly
> <vitaly.lubart@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [char-misc-next] mei: gsc_proxy: add gsc proxy driver
> 
> On Wed, Dec 28, 2022 at 11:46:36AM +0000, Usyskin, Alexander wrote:
> > >
> > > Why a whole new subdirectory for a tiny 200 line file?
> > >
> > All drivers for devices on mei bus have private subdirectory.
> > This one just modelled on the existing examples.
> > If you say that this is not a good thing - can put it in the main mei directory.
> 
> Put it in the main mei directory, no need to split things up for no good
> reason.
> 

All mei sub drivers are in sperate directories, this driver is indeed tiny, but I do prefer consistency,
In my view it is easier to maintain that way. 

I believe all the bellow stuff you've already discussed in ths thread,.
https://lore.kernel.org/lkml/YkXdgQH1GWCitf0A@xxxxxxxxx/T/
I guess all that was explained there, so no need to repeat.

We'll try to see what can be done to make it more robust and your comments are more than valid,
but as the thread concludes probably the component framework needs to be addressed. 


> > > > +static int mei_gsc_proxy_component_match(struct device *dev, int
> > > subcomponent, void *data)
> > > > +{
> > > > +	struct device *base = data;
> > > > +
> > > > +	if (!dev || !dev->driver ||
> > > > +	    strcmp(dev->driver->name, "i915") ||
> > >
> > > I thought I had objected to this "let's poke around in a driver name
> > > for a magic value" logic in the past.  How do you know this is
> > > always going to work?
> >
> > All components that serve Intel graphics integrated into PCH should
> > check in their match that calling device is graphic card sitting on the same
> PCH.
> 
> And by looking at the driver name?  That does not work, sorry.  Get access to
> the driver pointer please, that's the only way you know for sure, right?  And
> even then you shouldn't be messing with things in a device you have no
> control over (i.e. a driver pointer or name.)
> 
> > The code below checks that i915 pci device and mei pci device
> > (grandparent of our device on mei bus) are children of the same
> > parent, but there is no way to know if caller is, indeed, graphic device.
> Easiest way is to check well-known device river name.
> 
> Again, that's a big abuse of the driver model, please do not do that.
> 
> You need to rely on the fact that you will NOT be called unless your parent is
> of the correct type.  That's all, no fancy layering violations please.
> 
> 
> > All i915 components doing this comparison.
> > This is a simplified scheme of relations between devices here:
> >            /--- MEI PCI --- MEI --- GSC_PROXY PCH ---
> >            \--- GRAPHIC PCI --- I915
> > >
> > > > +	    subcomponent != I915_COMPONENT_GSC_PROXY)
> > > > +		return 0;
> > > > +
> > > > +	base = base->parent;
> > > > +	if (!base) /* mei device */
> > > > +		return 0;
> > >
> > > How can a device not have a parent?
> >
> > This one should be proxy device on mei bus, so parent should be there
> always, can drop this check.
> >
> > >
> > > > +
> > > > +	base = base->parent; /* pci device */
> > >
> > > You don't know this is a pci device :(
> >
> > This is more, like, note to explain on what level in above scheme we are
> now.
> > It should be mei pci device for match to succeed.
> 
> Again, you can't know this and you should never poke around like this.
> You don't control the parent of anything (hint, you just saved a reference
> counted pointer without grabbing a reference count...)
> 
> Please don't abuse the driver model like this, it will cause long-term problems
> for keeping this code alive and working properly.
> 
> thanks,
> 
> greg k-h