Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 Jan 2023 17:20:38 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     "Usyskin, Alexander" <alexander.usyskin@intel.com>
Cc:     "Winkler, Tomas" <tomas.winkler@intel.com>,
        "Lubart, Vitaly" <vitaly.lubart@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [char-misc-next] mei: gsc_proxy: add gsc proxy driver
Message-ID: <Y8lt1gmo9smOMXD7@kroah.com>
References: <20221222220214.3688774-1-tomas.winkler@intel.com>
 <Y6VV5d/V4MKDz2Te@kroah.com>
 <CY5PR11MB6366515D2A965EEDEC77AF63EDF29@CY5PR11MB6366.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CY5PR11MB6366515D2A965EEDEC77AF63EDF29@CY5PR11MB6366.namprd11.prod.outlook.com>
Precedence: bulk

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.

> > > +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