From:   "Winkler, Tomas" <tomas.winkler@intel.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Usyskin, Alexander" <alexander.usyskin@intel.com>
CC:     "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
Thread-Topic: [char-misc-next] mei: gsc_proxy: add gsc proxy driver
Thread-Index: AQHZFlEZJH6tjCVKFEmIILQgXlE1nK57EG6AgAgnAwCAIt/bAIAIJZ1w
Date:   Tue, 24 Jan 2023 21:05:19 +0000
Message-ID: <MN2PR11MB409305204A36872FEDAD2B82E5C99@MN2PR11MB4093.namprd11.prod.outlook.com>
References: <20221222220214.3688774-1-tomas.winkler@intel.com>
 <Y6VV5d/V4MKDz2Te@kroah.com>
 <CY5PR11MB6366515D2A965EEDEC77AF63EDF29@CY5PR11MB6366.namprd11.prod.outlook.com>
 <Y8lt1gmo9smOMXD7@kroah.com>
In-Reply-To: <Y8lt1gmo9smOMXD7@kroah.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?s4DvKll5NE0TFnmuOAstvIeVSBVygwaedKaAF44v2csaqHBJlkuxXmEbwxFH?=
 =?us-ascii?Q?vMo+pvvV6r/uk1oQAz6Zn17KJ+4FfFGK9xNm5TtiHdlRH14+mnwfp2motgcv?=
 =?us-ascii?Q?XX+mS61IfI6e0s4dXvOeSU+88rFXNQxvelhpYCwQtAoeBFIsyokmqVW1CWqv?=
 =?us-ascii?Q?KveQWxNEDqH5XrPjj/ej7o5kuHAooc548O4Y9O80RHbEHezIH8Rd5T+tjQSD?=
 =?us-ascii?Q?eIP1GnayP1yQ6cmPvmSfKDAe6rAdi+mnJmRU+X0+LHvGNONZFXqfvH2vjjh/?=
 =?us-ascii?Q?dMJ/cYX+k+Lw9PE+Qgn2MzGTCFswayNNOlDNSC5Pjk4G+tTUs55LDwt9/xx3?=
 =?us-ascii?Q?ZBHJg/+PnqdWD2boIHvyONWjs3z1BYmoWhmtXfaUHYxDS/LLkC4dgmSsUp2X?=
 =?us-ascii?Q?CS8HTKxn7djKElGpzcbrgOibSRKeg7MQa8gSx5qWbPm790Km5rB5SzG2TuE0?=
 =?us-ascii?Q?s8AWNczKzj5o5EJBKDoRGN/RkWg6egA3VcsD7J3daGq392xQicdKjB39iNHG?=
 =?us-ascii?Q?8irowjBSzNpb37SDEqtmyks/z0Mkc6gO+ItCzb/XUYVDssQ5Gk6iqvJEromR?=
 =?us-ascii?Q?lilMI7jyGmIwgUQuT/1Eb6mC/tpqkz/qGbf+2bvYu1iDXVO3U25I9RK5eIGP?=
 =?us-ascii?Q?kguaYyWp27Vl1NdHbKFAHUOwvrW0BzT5b1myESAppdyOzb9uyIo7PcqeAho5?=
 =?us-ascii?Q?+NHgTtCq2QEmgeY+J9ybs759i4HqAoVnIWDpq3Ob47ooHyiYTr6TmBPdYuyA?=
 =?us-ascii?Q?Cf2Gnm/Vasxt9h0CcdA6HBUrxaq8ab48h/Lt1dZKmxbBlVopJmLh9k5LL/dP?=
 =?us-ascii?Q?vdOzYdu17TRocSEVxcIo4ccOUTfvYQOI8lydTwvctYTT1NGtkzmVFjFJf6JJ?=
 =?us-ascii?Q?Fa6WbJU9iSitAZhriEIUggq57bkyXcBWVwiUeAo3MRl8+Cq/cQ+c9rEUvbZN?=
 =?us-ascii?Q?3DAreMXLQNBQhI8mlzfQmcuYUQxbWQUBaJY+B3j1ydTzZZNf/0XI2pzwdlv+?=
 =?us-ascii?Q?Wnc73mB6+f29Pochc3YNLECq/MHBi4YezvNembrRzYeaIY/Gp0cPvVB7PuHp?=
 =?us-ascii?Q?vzuUZhRg77C5ltEsOFaCGujOHk2sK65CoIJVDSpjoT2hpUcp8OWFbbN1DUI6?=
 =?us-ascii?Q?gGjzUc/SJ+dFL3sb5/tIOzDFXSZeDplwIMznnB4EyKARSQPEzdNOuN4k7eTg?=
 =?us-ascii?Q?h0liKt9utVcAvmqv/+D2q9HgLLB03hKHj9AlXMpXq+a/2anzh5YTFgUhh7yd?=
 =?us-ascii?Q?oWPNL7UBwJg6SPeRFNaHxz8+7SFBd4FCqOL712l5kKwxu9//oE5JufM6oXxi?=
 =?us-ascii?Q?OL0DtPdEaAtx+oXjkPn8dXHH0v9boi8BJ3LQRdnxedAd/CUkDLfR+4ZNBxkM?=
 =?us-ascii?Q?QnqyE7b57nQ6XAPwox1ShvFVXVtitsoebeTQjp+Q7YRekcwKt5M2jcZ5LVXr?=
 =?us-ascii?Q?LzSEhwSsnQnoN/J0PWeD3TibsxMGcg9exKRkCUGIOli2opkzf+P1xtCUL4tz?=
 =?us-ascii?Q?zLFU12A9Hd8Re5D88/DpP1muesiFES0kb3xV7ywZ0LYI1p7dQ54hNmU3a0qD?=
 =?us-ascii?Q?G+q0WcyjS4uGD14pqm+Izm6S23kAUO/nXNLfz2ZQ?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MN2PR11MB4093.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: afaae480-b178-4385-5629-08dafe4eb36f
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jan 2023 21:05:19.7512
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: UodvT8CObg+VOoIWgZOX0NlLFRYBdEV/klMK89rh/6HwH7yaYmo4lK25ym0YMjPhzIuzGkSIljgdWl7/u4ttCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4996
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Thursday, January 19, 2023 18:21
> 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
> Subject: Re: [char-misc-next] mei: gsc_proxy: add gsc proxy driver
>=20
> 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 d=
irectory.
>=20
> Put it in the main mei directory, no need to split things up for no good
> reason.
>=20

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.=20

I believe all the bellow stuff you've already discussed in ths thread,.
https://lore.kernel.org/lkml/YkXdgQH1GWCitf0A@kroah.com/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 ad=
dressed.=20


> > > > +static int mei_gsc_proxy_component_match(struct device *dev, int
> > > subcomponent, void *data)
> > > > +{
> > > > +	struct device *base =3D 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.
>=20
> And by looking at the driver name?  That does not work, sorry.  Get acces=
s 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.)
>=20
> > 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 devic=
e.
> Easiest way is to check well-known device river name.
>=20
> Again, that's a big abuse of the driver model, please do not do that.
>=20
> You need to rely on the fact that you will NOT be called unless your pare=
nt is
> of the correct type.  That's all, no fancy layering violations please.
>=20
>=20
> > 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 !=3D I915_COMPONENT_GSC_PROXY)
> > > > +		return 0;
> > > > +
> > > > +	base =3D 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 =3D 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 ar=
e
> now.
> > It should be mei pci device for match to succeed.
>=20
> 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 referenc=
e
> counted pointer without grabbing a reference count...)
>=20
> Please don't abuse the driver model like this, it will cause long-term pr=
oblems
> for keeping this code alive and working properly.
>=20
> thanks,
>=20
> greg k-h