RE: [PATCH v2 2/5] mei: add support for graphics system controller (gsc) devices

From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Usyskin, Alexander" <alexander.usyskin@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	"Vivi, Rodrigo" <rodrigo.vivi@intel.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	"Winkler, Tomas" <tomas.winkler@intel.com>,
	"Lubart, Vitaly" <vitaly.lubart@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 2/5] mei: add support for graphics system controller (gsc) devices
Date: Thu, 27 Jan 2022 11:12:03 +0200	[thread overview]
Message-ID: <878rv1y23g.fsf@intel.com> (raw)
In-Reply-To: <MW3PR11MB4651EAD45375963B6A43CF8AED219@MW3PR11MB4651.namprd11.prod.outlook.com>

On Thu, 27 Jan 2022, "Usyskin, Alexander" <alexander.usyskin@intel.com> wrote:
>> -----Original Message-----
>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Sent: Wednesday, January 26, 2022 20:06
>> To: Usyskin, Alexander <alexander.usyskin@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>; Joonas Lahtinen
>> <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
>> David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Winkler,
>> Tomas <tomas.winkler@intel.com>; Lubart, Vitaly <vitaly.lubart@intel.com>;
>> intel-gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 2/5] mei: add support for graphics system controller
>> (gsc) devices
>> 
>> On Wed, Jan 19, 2022 at 05:58:04PM +0200, Alexander Usyskin wrote:
>> > From: Tomas Winkler <tomas.winkler@intel.com>
>> >
>> > GSC is a graphics system controller, based on CSE, it provides
>> > a chassis controller for graphics discrete cards, as well as it
>> > supports media protection on selected devices.
>> >
>> > mei_gsc binds to a auxiliary devices exposed by Intel discrete
>> > driver i915.
>> >
>> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
>> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>> > ---
>> >  drivers/misc/mei/Kconfig  |  14 +++
>> >  drivers/misc/mei/Makefile |   3 +
>> >  drivers/misc/mei/gsc-me.c | 192
>> ++++++++++++++++++++++++++++++++++++++
>> >  drivers/misc/mei/hw-me.c  |  27 +++++-
>> >  drivers/misc/mei/hw-me.h  |   2 +
>> >  5 files changed, 236 insertions(+), 2 deletions(-)
>> >  create mode 100644 drivers/misc/mei/gsc-me.c
>> >
>> > diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
>> > index 0e0bcd0da852..ec119bb98251 100644
>> > --- a/drivers/misc/mei/Kconfig
>> > +++ b/drivers/misc/mei/Kconfig
>> > @@ -46,6 +46,20 @@ config INTEL_MEI_TXE
>> >  	  Supported SoCs:
>> >  	  Intel Bay Trail
>> >
>> > +config INTEL_MEI_GSC
>> > +	tristate "Intel MEI GSC embedded device"
>> > +	select INTEL_MEI
>> > +	select INTEL_MEI_ME
>> 
>> Please don't select, why not just depend on?
>
> These are hard dependencies. If user wants to have INTEL_GSC,
> user should enable INTEL_MEI and INTEL_MEI_ME anyway.

Isn't that exactly what depends on conveys?

> INTEL_MEI_ME selects INTEL_MEI in this file and it was taken as example.
>
> What is wrong with select? Why to avoid it use?

Documentation/kbuild/kconfig-language.rst:

  Note:
	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

If we followed that rule, we'd have a lot less kconfig dependency
issues.

If I had the time, I'd add a lint/verbose mode to scripts/kconfig tool
to warn about selecting symbols that are visible or have dependencies.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center