linux-kernel - RE: [char-misc-next] mei: gsc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CY5PR11MB6366515D2A965EEDEC77AF63EDF29@CY5PR11MB6366.namprd11.prod.outlook.com>
Date:   Wed, 28 Dec 2022 11:46:36 +0000
From:   "Usyskin, Alexander" <alexander.usyskin@...el.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Winkler, Tomas" <tomas.winkler@...el.com>
CC:     "Lubart, Vitaly" <vitaly.lubart@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [char-misc-next] mei: gsc_proxy: add gsc proxy driver

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

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

> 
> If it is, then pass in a REAL pci device structure please.
> 
> > +
> > +	dev = dev->parent;
> > +	return (base && dev && dev == base);
> 
> I do not understand this statement at all, what are you doing here?
> 
> confused,

Hope that scheme above explains the relations between devices.
> 
> greg k-h


-- 
Thanks,
Sasha