2022-03-29 17:23:09

by Geert Uytterhoeven

[permalink] [raw]
Subject: [PATCH] peci: PECI should depend on ARCH_ASPEED

The Platform Environment Control Interface (PECI) is only available on
Baseboard Management Controllers (BMC) for Intel processors. Currently
the only supported BMCs are ASpeed BMC SoCs. Hence add a dependency on
ARCH_ASPEED, to prevent asking the user about the PECI subsystem when
configuring a kernel without ASpeed SoC support.

Fixes: 6523d3b2ffa238ac ("peci: Add core infrastructure")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
drivers/peci/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
index 89872ad833201510..0d3ef8ba0998d649 100644
--- a/drivers/peci/Kconfig
+++ b/drivers/peci/Kconfig
@@ -2,6 +2,7 @@

menuconfig PECI
tristate "PECI support"
+ depends on ARCH_ASPEED || COMPILE_TEST
help
The Platform Environment Control Interface (PECI) is an interface
that provides a communication channel to Intel processors and
--
2.25.1


2022-03-30 09:42:36

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] peci: PECI should depend on ARCH_ASPEED

On Tue, Mar 29, 2022 at 11:21:37AM +0200, Geert Uytterhoeven wrote:
> The Platform Environment Control Interface (PECI) is only available on
> Baseboard Management Controllers (BMC) for Intel processors. Currently
> the only supported BMCs are ASpeed BMC SoCs. Hence add a dependency on
> ARCH_ASPEED, to prevent asking the user about the PECI subsystem when
> configuring a kernel without ASpeed SoC support.
>
> Fixes: 6523d3b2ffa238ac ("peci: Add core infrastructure")
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> drivers/peci/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> index 89872ad833201510..0d3ef8ba0998d649 100644
> --- a/drivers/peci/Kconfig
> +++ b/drivers/peci/Kconfig
> @@ -2,6 +2,7 @@
>
> menuconfig PECI
> tristate "PECI support"
> + depends on ARCH_ASPEED || COMPILE_TEST

I hate ARCH_ dependencies as there is nothing specific with that one
platform that means that this driver subsystem will only work on that
one.

I'm all for fixing build dependancies, but it should be fine to build
all drivers for all arches.

So sorry, I don't like this change.

greg k-h

2022-03-30 09:44:32

by Patrick Williams

[permalink] [raw]
Subject: Re: [PATCH] peci: PECI should depend on ARCH_ASPEED

On Tue, Mar 29, 2022 at 12:33:14PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Mar 29, 2022 at 11:21:37AM +0200, Geert Uytterhoeven wrote:
> > The Platform Environment Control Interface (PECI) is only available on
> > Baseboard Management Controllers (BMC) for Intel processors. Currently
> > the only supported BMCs are ASpeed BMC SoCs. Hence add a dependency on
> > ARCH_ASPEED, to prevent asking the user about the PECI subsystem when
> > configuring a kernel without ASpeed SoC support.
> >
> > Fixes: 6523d3b2ffa238ac ("peci: Add core infrastructure")
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > ---
> > drivers/peci/Kconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> > index 89872ad833201510..0d3ef8ba0998d649 100644
> > --- a/drivers/peci/Kconfig
> > +++ b/drivers/peci/Kconfig
> > @@ -2,6 +2,7 @@
> >
> > menuconfig PECI
> > tristate "PECI support"
> > + depends on ARCH_ASPEED || COMPILE_TEST
>
> I hate ARCH_ dependencies as there is nothing specific with that one
> platform that means that this driver subsystem will only work on that
> one.

The motivation in the commit message isn't even accurate because the chips
under ARCH_NPCM are usually used as a BMC as well and PECI is just as important
for them. HPE also has a custom chip they use as BMC and they've started the
process for upstreaming that support.

> I'm all for fixing build dependancies, but it should be fine to build
> all drivers for all arches.

There are a few drivers, like PECI and FSI, that are likely only useful
when being used in the BMC space. Is it worth having a "config BMC" for
drivers which are likely only useful in a BMC environment and that we can
"if BMC" around these drivers so they get hidden from the menuconfig for
typical use cases?

--
Patrick Williams


Attachments:
(No filename) (1.86 kB)
signature.asc (849.00 B)
Download all attachments

2022-03-30 16:44:21

by Winiarska, Iwona

[permalink] [raw]
Subject: Re: [PATCH] peci: PECI should depend on ARCH_ASPEED

On Tue, 2022-03-29 at 12:08 -0500, Patrick Williams wrote:
> On Tue, Mar 29, 2022 at 12:33:14PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Mar 29, 2022 at 11:21:37AM +0200, Geert Uytterhoeven wrote:
> > > The Platform Environment Control Interface (PECI) is only available on
> > > Baseboard Management Controllers (BMC) for Intel processors.  Currently
> > > the only supported BMCs are ASpeed BMC SoCs.  Hence add a dependency on
> > > ARCH_ASPEED, to prevent asking the user about the PECI subsystem when
> > > configuring a kernel without ASpeed SoC support.
> > >
> > > Fixes: 6523d3b2ffa238ac ("peci: Add core infrastructure")
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > ---
> > >  drivers/peci/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> > > index 89872ad833201510..0d3ef8ba0998d649 100644
> > > --- a/drivers/peci/Kconfig
> > > +++ b/drivers/peci/Kconfig
> > > @@ -2,6 +2,7 @@
> > >  
> > >  menuconfig PECI
> > >         tristate "PECI support"
> > > +       depends on ARCH_ASPEED || COMPILE_TEST
> >
> > I hate ARCH_ dependencies as there is nothing specific with that one
> > platform that means that this driver subsystem will only work on that
> > one.
>
> The motivation in the commit message isn't even accurate because the chips
> under ARCH_NPCM are usually used as a BMC as well and PECI is just as important
> for them.  HPE also has a custom chip they use as BMC and they've started the
> process for upstreaming that support.

"Currently the only supported BMCs are ASpeed BMC SoCs."
From PECI subsystem perspective (not BMC support in general), it is technically
true for now - but I agree with Greg and Patrick, this is just artificially
introducing build-time dependencies where in fact there are none. And yes - we
would then have to add the "depends on ARCH_YET_ANOTHER_ARCH" to generic
subsystem anytime we add a new PECI controller. This belongs in the controller
(and the ASPEED one depends on ARCH_ASPEED).

>
> > I'm all for fixing build dependancies, but it should be fine to build
> > all drivers for all arches.
>
> There are a few drivers, like PECI and FSI, that are likely only useful
> when being used in the BMC space.  Is it worth having a "config BMC" for
> drivers which are likely only useful in a BMC environment and that we can
> "if BMC" around these drivers so they get hidden from the menuconfig for
> typical use cases?

We don't have "config SERVER/config DESKTOP/config IOT".
I don't think there's anything special about BMCs to benefit from introducing
that (ultimately, this would be yet another "artificial build-time dependency").

Thanks
-Iwona

>

2022-03-31 03:27:15

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] peci: PECI should depend on ARCH_ASPEED

Hi Iwona,

On Wed, Mar 30, 2022 at 12:37 PM Winiarska, Iwona
<[email protected]> wrote:
> On Tue, 2022-03-29 at 12:08 -0500, Patrick Williams wrote:
> > On Tue, Mar 29, 2022 at 12:33:14PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Mar 29, 2022 at 11:21:37AM +0200, Geert Uytterhoeven wrote:
> > > > The Platform Environment Control Interface (PECI) is only available on
> > > > Baseboard Management Controllers (BMC) for Intel processors. Currently
> > > > the only supported BMCs are ASpeed BMC SoCs. Hence add a dependency on
> > > > ARCH_ASPEED, to prevent asking the user about the PECI subsystem when
> > > > configuring a kernel without ASpeed SoC support.
> > > >
> > > > Fixes: 6523d3b2ffa238ac ("peci: Add core infrastructure")
> > > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > > ---
> > > > drivers/peci/Kconfig | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/peci/Kconfig b/drivers/peci/Kconfig
> > > > index 89872ad833201510..0d3ef8ba0998d649 100644
> > > > --- a/drivers/peci/Kconfig
> > > > +++ b/drivers/peci/Kconfig
> > > > @@ -2,6 +2,7 @@
> > > >
> > > > menuconfig PECI
> > > > tristate "PECI support"
> > > > + depends on ARCH_ASPEED || COMPILE_TEST
> > >
> > > I hate ARCH_ dependencies as there is nothing specific with that one
> > > platform that means that this driver subsystem will only work on that
> > > one.
> >
> > The motivation in the commit message isn't even accurate because the chips
> > under ARCH_NPCM are usually used as a BMC as well and PECI is just as important
> > for them. HPE also has a custom chip they use as BMC and they've started the
> > process for upstreaming that support.
>
> "Currently the only supported BMCs are ASpeed BMC SoCs."
> From PECI subsystem perspective (not BMC support in general), it is technically
> true for now - but I agree with Greg and Patrick, this is just artificially
> introducing build-time dependencies where in fact there are none. And yes - we
> would then have to add the "depends on ARCH_YET_ANOTHER_ARCH" to generic
> subsystem anytime we add a new PECI controller. This belongs in the controller
> (and the ASPEED one depends on ARCH_ASPEED).

OK, let's keep it as-is.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds