[PATCH] hwmon: OCC drivers are PowerPC-only

[PATCH] hwmon: OCC drivers are PowerPC-only

[Jean Delvare]

Don't propose PowerPC-only drivers on other architectures, unless
build-testing.

Also drop configuration symbol SENSORS_OCC which serves no purpose
that I can see.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Eddie James <eajames@linux.ibm.com>
Cc: Guenter Roeck <linux@roeck-us.net>
---
SENSORS_OCC *would* serve a purpose if the common code between the
POWER8 driver and the POWER9 driver would go in a separate, shared
module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
specific code. This would avoid packaging the same code twice in 2
separate modules, therefore saving some storage space for ppc
distributions.

As far as I can see, this would simply require exporting 2 functions
(occ_setup and occ_shutdown). Is there any reason why things were not
done that way in the first place? This would look cleaner to me.

 drivers/hwmon/Makefile    |    2 +-
 drivers/hwmon/occ/Kconfig |    8 ++------
 2 files changed, 3 insertions(+), 7 deletions(-)

--- linux-5.0.orig/drivers/hwmon/occ/Kconfig	2019-03-04 00:21:29.000000000 +0100
+++ linux-5.0/drivers/hwmon/occ/Kconfig	2019-04-09 14:08:41.316551071 +0200
@@ -5,7 +5,7 @@
 config SENSORS_OCC_P8_I2C
 	tristate "POWER8 OCC through I2C"
 	depends on I2C
-	select SENSORS_OCC
+	depends on POWERPC || COMPILE_TEST
 	help
 	 This option enables support for monitoring sensors provided by the
 	 On-Chip Controller (OCC) on a POWER8 processor. Communications with
@@ -17,7 +17,7 @@ config SENSORS_OCC_P8_I2C
 config SENSORS_OCC_P9_SBE
 	tristate "POWER9 OCC through SBE"
 	depends on FSI_OCC
-	select SENSORS_OCC
+	depends on POWERPC || COMPILE_TEST
 	help
 	 This option enables support for monitoring sensors provided by the
 	 On-Chip Controller (OCC) on a POWER9 processor. Communications with
@@ -25,7 +25,3 @@ config SENSORS_OCC_P9_SBE
 
 	 This driver can also be built as a module. If so, the module will be
 	 called occ-p9-hwmon.
-
-config SENSORS_OCC
-	bool "POWER On-Chip Controller"
-	depends on SENSORS_OCC_P8_I2C || SENSORS_OCC_P9_SBE
--- linux-5.0.orig/drivers/hwmon/Makefile	2019-03-04 00:21:29.000000000 +0100
+++ linux-5.0/drivers/hwmon/Makefile	2019-04-09 14:33:49.605510047 +0200
@@ -178,7 +178,7 @@ obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-h
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
 obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
 
-obj-$(CONFIG_SENSORS_OCC)	+= occ/
+obj-y				+= occ/
 obj-$(CONFIG_PMBUS)		+= pmbus/
 
 ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] hwmon: OCC drivers are PowerPC-only

[Eddie James]

On 4/9/19 7:45 AM, Jean Delvare wrote:
> Don't propose PowerPC-only drivers on other architectures, unless
> build-testing.


This driver does NOT only run on PowerPC; rather it runs on a BMC 
processor connected to a PowerPC processor. BMC will most likely be ARM, 
but shouldn't be restricted to that arch only.


>
> Also drop configuration symbol SENSORS_OCC which serves no purpose
> that I can see.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Eddie James <eajames@linux.ibm.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> ---
> SENSORS_OCC *would* serve a purpose if the common code between the
> POWER8 driver and the POWER9 driver would go in a separate, shared
> module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
> specific code. This would avoid packaging the same code twice in 2
> separate modules, therefore saving some storage space for ppc
> distributions.


Well you'd never have both P8 and P9 enabled at once, so space shouldn't 
be an issue. I agree this could be cleaner but I think I was getting 
duplicate symbol errors for the compile test and so I did it this way. 
If this doesn't lead to errors in the compile test, I'm fine with this 
(without the change for PPC only though).


Thanks,

Eddie

>
> As far as I can see, this would simply require exporting 2 functions
> (occ_setup and occ_shutdown). Is there any reason why things were not
> done that way in the first place? This would look cleaner to me.
>
>   drivers/hwmon/Makefile    |    2 +-
>   drivers/hwmon/occ/Kconfig |    8 ++------
>   2 files changed, 3 insertions(+), 7 deletions(-)
>
> --- linux-5.0.orig/drivers/hwmon/occ/Kconfig	2019-03-04 00:21:29.000000000 +0100
> +++ linux-5.0/drivers/hwmon/occ/Kconfig	2019-04-09 14:08:41.316551071 +0200
> @@ -5,7 +5,7 @@
>   config SENSORS_OCC_P8_I2C
>   	tristate "POWER8 OCC through I2C"
>   	depends on I2C
> -	select SENSORS_OCC
> +	depends on POWERPC || COMPILE_TEST
>   	help
>   	 This option enables support for monitoring sensors provided by the
>   	 On-Chip Controller (OCC) on a POWER8 processor. Communications with
> @@ -17,7 +17,7 @@ config SENSORS_OCC_P8_I2C
>   config SENSORS_OCC_P9_SBE
>   	tristate "POWER9 OCC through SBE"
>   	depends on FSI_OCC
> -	select SENSORS_OCC
> +	depends on POWERPC || COMPILE_TEST
>   	help
>   	 This option enables support for monitoring sensors provided by the
>   	 On-Chip Controller (OCC) on a POWER9 processor. Communications with
> @@ -25,7 +25,3 @@ config SENSORS_OCC_P9_SBE
>   
>   	 This driver can also be built as a module. If so, the module will be
>   	 called occ-p9-hwmon.
> -
> -config SENSORS_OCC
> -	bool "POWER On-Chip Controller"
> -	depends on SENSORS_OCC_P8_I2C || SENSORS_OCC_P9_SBE
> --- linux-5.0.orig/drivers/hwmon/Makefile	2019-03-04 00:21:29.000000000 +0100
> +++ linux-5.0/drivers/hwmon/Makefile	2019-04-09 14:33:49.605510047 +0200
> @@ -178,7 +178,7 @@ obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-h
>   obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
>   obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
>   
> -obj-$(CONFIG_SENSORS_OCC)	+= occ/
> +obj-y				+= occ/
>   obj-$(CONFIG_PMBUS)		+= pmbus/
>   
>   ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>
>

Re: [PATCH] hwmon: OCC drivers are PowerPC-only

[Guenter Roeck]

On Tue, Apr 09, 2019 at 10:20:22AM -0500, Eddie James wrote:
> 
> On 4/9/19 7:45 AM, Jean Delvare wrote:
> >Don't propose PowerPC-only drivers on other architectures, unless
> >build-testing.
> 
> 
> This driver does NOT only run on PowerPC; rather it runs on a BMC processor
> connected to a PowerPC processor. BMC will most likely be ARM, but shouldn't
> be restricted to that arch only.
> 
> 
> >
> >Also drop configuration symbol SENSORS_OCC which serves no purpose
> >that I can see.
> >
> >Signed-off-by: Jean Delvare <jdelvare@suse.de>
> >Cc: Eddie James <eajames@linux.ibm.com>
> >Cc: Guenter Roeck <linux@roeck-us.net>
> >---
> >SENSORS_OCC *would* serve a purpose if the common code between the
> >POWER8 driver and the POWER9 driver would go in a separate, shared
> >module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
> >specific code. This would avoid packaging the same code twice in 2
> >separate modules, therefore saving some storage space for ppc
> >distributions.
> 
> 
> Well you'd never have both P8 and P9 enabled at once, so space shouldn't be
> an issue. I agree this could be cleaner but I think I was getting duplicate
> symbol errors for the compile test and so I did it this way. If this doesn't
> lead to errors in the compile test, I'm fine with this (without the change
> for PPC only though).
>

Any common code would have to be in a separate module to avoid duplicate
symbols. You'd bundle common.c and sysfs.c into one module and export
the functions called from p8/p9 specific code.

Something like

occ-common-objs := common.o sysfs.o

obj-$(CONFIG_SENSORS_OCC) += occ-common.o
obj-$(CONFIG_SENSORS_OCC_P8_I2C) += p8_i2c.o
obj-$(CONFIG_SENSORS_OCC_P9_SBE) += p9-sbe.o

Guenter

> 
> Thanks,
> 
> Eddie
> 
> >
> >As far as I can see, this would simply require exporting 2 functions
> >(occ_setup and occ_shutdown). Is there any reason why things were not
> >done that way in the first place? This would look cleaner to me.
> >
> >  drivers/hwmon/Makefile    |    2 +-
> >  drivers/hwmon/occ/Kconfig |    8 ++------
> >  2 files changed, 3 insertions(+), 7 deletions(-)
> >
> >--- linux-5.0.orig/drivers/hwmon/occ/Kconfig	2019-03-04 00:21:29.000000000 +0100
> >+++ linux-5.0/drivers/hwmon/occ/Kconfig	2019-04-09 14:08:41.316551071 +0200
> >@@ -5,7 +5,7 @@
> >  config SENSORS_OCC_P8_I2C
> >  	tristate "POWER8 OCC through I2C"
> >  	depends on I2C
> >-	select SENSORS_OCC
> >+	depends on POWERPC || COMPILE_TEST
> >  	help
> >  	 This option enables support for monitoring sensors provided by the
> >  	 On-Chip Controller (OCC) on a POWER8 processor. Communications with
> >@@ -17,7 +17,7 @@ config SENSORS_OCC_P8_I2C
> >  config SENSORS_OCC_P9_SBE
> >  	tristate "POWER9 OCC through SBE"
> >  	depends on FSI_OCC
> >-	select SENSORS_OCC
> >+	depends on POWERPC || COMPILE_TEST
> >  	help
> >  	 This option enables support for monitoring sensors provided by the
> >  	 On-Chip Controller (OCC) on a POWER9 processor. Communications with
> >@@ -25,7 +25,3 @@ config SENSORS_OCC_P9_SBE
> >  	 This driver can also be built as a module. If so, the module will be
> >  	 called occ-p9-hwmon.
> >-
> >-config SENSORS_OCC
> >-	bool "POWER On-Chip Controller"
> >-	depends on SENSORS_OCC_P8_I2C || SENSORS_OCC_P9_SBE
> >--- linux-5.0.orig/drivers/hwmon/Makefile	2019-03-04 00:21:29.000000000 +0100
> >+++ linux-5.0/drivers/hwmon/Makefile	2019-04-09 14:33:49.605510047 +0200
> >@@ -178,7 +178,7 @@ obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-h
> >  obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
> >  obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
> >-obj-$(CONFIG_SENSORS_OCC)	+= occ/
> >+obj-y				+= occ/
> >  obj-$(CONFIG_PMBUS)		+= pmbus/
> >  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> >
> >
> 

Re: [PATCH] hwmon: OCC drivers are PowerPC-only

[Jean Delvare]

Hi Eddie,

Thanks for the quick answer.

On Tue, 9 Apr 2019 10:20:22 -0500, Eddie James wrote:
> On 4/9/19 7:45 AM, Jean Delvare wrote:
> > Don't propose PowerPC-only drivers on other architectures, unless
> > build-testing.  
> 
> This driver does NOT only run on PowerPC; rather it runs on a BMC 
> processor connected to a PowerPC processor. BMC will most likely be ARM, 

Thanks for clarifying. So, you have a server with one or more PowerPC
processors, running any operating system decided by the customer, and
in the same system, is a BMC, running a Linux operating system provided
by IBM? Do I understand it correctly?

> but shouldn't be restricted to that arch only.

Why not? Restricting drivers to the architectures or platforms where
they make sense significantly eases the work of the maintainers of
distribution kernels. Each new kernel version comes with several dozens
of new options. Without hints, they have no idea what is needed on
which architecture, and they either select everything, resulting in an
overweight kernel forever, or nothing, resulting in missing features
until someone complains.

Regardless of any dependency at the Kconfig level, the help text of
these drivers should explain exactly what you wrote above. At the
moment, the reader has no way to guess that the drivers only make sense
for an embedded Linux running on a BMC. As I understand it now, general
purpose distributions do not need these drivers, right? But for example
SUSE kernels included them on all architectures. I just restricted them
to ppc64, but apparently I was wrong and it should be disabled there
too.

A good Kconfig help text should help the user decide whether they need
the driver or not.

> > Also drop configuration symbol SENSORS_OCC which serves no purpose
> > that I can see.
> >
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Eddie James <eajames@linux.ibm.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > ---
> > SENSORS_OCC *would* serve a purpose if the common code between the
> > POWER8 driver and the POWER9 driver would go in a separate, shared
> > module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
> > specific code. This would avoid packaging the same code twice in 2
> > separate modules, therefore saving some storage space for ppc
> > distributions.  
> 
> Well you'd never have both P8 and P9 enabled at once, so space shouldn't 
> be an issue. I agree this could be cleaner but I think I was getting 

Where "you" is IBM, provider of the embedded Linux running on the BMC?

> duplicate symbol errors for the compile test and so I did it this way. 
> If this doesn't lead to errors in the compile test, I'm fine with this 
> (without the change for PPC only though).

Dropping SENSORS_OCC can't result in duplicate symbols because it is a
no-op. But while this is the easiest solution, it it not the cleanest
in my opinion. OTOH, if you are certain that both modules will never be
shipped at the same time, then it indeed doesn't matter that much. I
can provide a patch going in either direction. Guenter, any preference?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] hwmon: OCC drivers are PowerPC-only

[Guenter Roeck]

On Tue, Apr 09, 2019 at 09:44:33PM +0200, Jean Delvare wrote:
> Hi Eddie,
> 
> Thanks for the quick answer.
> 
> On Tue, 9 Apr 2019 10:20:22 -0500, Eddie James wrote:
> > On 4/9/19 7:45 AM, Jean Delvare wrote:
> > > Don't propose PowerPC-only drivers on other architectures, unless
> > > build-testing.  
> > 
> > This driver does NOT only run on PowerPC; rather it runs on a BMC 
> > processor connected to a PowerPC processor. BMC will most likely be ARM, 
> 
> Thanks for clarifying. So, you have a server with one or more PowerPC
> processors, running any operating system decided by the customer, and
> in the same system, is a BMC, running a Linux operating system provided
> by IBM? Do I understand it correctly?
> 
> > but shouldn't be restricted to that arch only.
> 
> Why not? Restricting drivers to the architectures or platforms where
> they make sense significantly eases the work of the maintainers of
> distribution kernels. Each new kernel version comes with several dozens
> of new options. Without hints, they have no idea what is needed on
> which architecture, and they either select everything, resulting in an
> overweight kernel forever, or nothing, resulting in missing features
> until someone complains.
> 
> Regardless of any dependency at the Kconfig level, the help text of
> these drivers should explain exactly what you wrote above. At the
> moment, the reader has no way to guess that the drivers only make sense
> for an embedded Linux running on a BMC. As I understand it now, general
> purpose distributions do not need these drivers, right? But for example
> SUSE kernels included them on all architectures. I just restricted them
> to ppc64, but apparently I was wrong and it should be disabled there
> too.
> 
> A good Kconfig help text should help the user decide whether they need
> the driver or not.
> 
> > > Also drop configuration symbol SENSORS_OCC which serves no purpose
> > > that I can see.
> > >
> > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > Cc: Eddie James <eajames@linux.ibm.com>
> > > Cc: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > SENSORS_OCC *would* serve a purpose if the common code between the
> > > POWER8 driver and the POWER9 driver would go in a separate, shared
> > > module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
> > > specific code. This would avoid packaging the same code twice in 2
> > > separate modules, therefore saving some storage space for ppc
> > > distributions.  
> > 
> > Well you'd never have both P8 and P9 enabled at once, so space shouldn't 
> > be an issue. I agree this could be cleaner but I think I was getting 
> 
> Where "you" is IBM, provider of the embedded Linux running on the BMC?
> 
> > duplicate symbol errors for the compile test and so I did it this way. 
> > If this doesn't lead to errors in the compile test, I'm fine with this 
> > (without the change for PPC only though).
> 
> Dropping SENSORS_OCC can't result in duplicate symbols because it is a
> no-op. But while this is the easiest solution, it it not the cleanest
> in my opinion. OTOH, if you are certain that both modules will never be

I suspect that referred to symbols in common.c and sysfs.c, but I may
be wrong.

> shipped at the same time, then it indeed doesn't matter that much. I
> can provide a patch going in either direction. Guenter, any preference?
> 
The one I suggested in my other in my reply. Of course, I didn't test it,
so it may not work as suggested and require some additional tweaks.

Guenter

Re: [PATCH] hwmon: OCC drivers are PowerPC-only

[Eddie James]

On 4/9/19 2:44 PM, Jean Delvare wrote:
> Hi Eddie,
>
> Thanks for the quick answer.
>
> On Tue, 9 Apr 2019 10:20:22 -0500, Eddie James wrote:
>> On 4/9/19 7:45 AM, Jean Delvare wrote:
>>> Don't propose PowerPC-only drivers on other architectures, unless
>>> build-testing.
>> This driver does NOT only run on PowerPC; rather it runs on a BMC
>> processor connected to a PowerPC processor. BMC will most likely be ARM,
> Thanks for clarifying. So, you have a server with one or more PowerPC
> processors, running any operating system decided by the customer, and
> in the same system, is a BMC, running a Linux operating system provided
> by IBM? Do I understand it correctly?


Exactly, though the operating system running on the BMC (OpenBMC) is 
used/developed by a number of other companies as well.


>
>> but shouldn't be restricted to that arch only.
> Why not? Restricting drivers to the architectures or platforms where
> they make sense significantly eases the work of the maintainers of
> distribution kernels. Each new kernel version comes with several dozens
> of new options. Without hints, they have no idea what is needed on
> which architecture, and they either select everything, resulting in an
> overweight kernel forever, or nothing, resulting in missing features
> until someone complains.


I see, that makes sense to restrict it then. It is possible someone will 
want to run OpenBMC managing a Power processor on something other than 
an ARM chip, but I don't think that configuration is needed right now.


>
> Regardless of any dependency at the Kconfig level, the help text of
> these drivers should explain exactly what you wrote above. At the
> moment, the reader has no way to guess that the drivers only make sense
> for an embedded Linux running on a BMC. As I understand it now, general
> purpose distributions do not need these drivers, right? But for example
> SUSE kernels included them on all architectures. I just restricted them
> to ppc64, but apparently I was wrong and it should be disabled there
> too.
>
> A good Kconfig help text should help the user decide whether they need
> the driver or not.


Yep, the Kconfig text can probably be improved as well. I'll look into that.


>
>>> Also drop configuration symbol SENSORS_OCC which serves no purpose
>>> that I can see.
>>>
>>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
>>> Cc: Eddie James <eajames@linux.ibm.com>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> SENSORS_OCC *would* serve a purpose if the common code between the
>>> POWER8 driver and the POWER9 driver would go in a separate, shared
>>> module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
>>> specific code. This would avoid packaging the same code twice in 2
>>> separate modules, therefore saving some storage space for ppc
>>> distributions.
>> Well you'd never have both P8 and P9 enabled at once, so space shouldn't
>> be an issue. I agree this could be cleaner but I think I was getting
> Where "you" is IBM, provider of the embedded Linux running on the BMC?


Yes, though again could be any distributor of OpenBMC.

Thanks for looking into this!

Eddie


>
>> duplicate symbol errors for the compile test and so I did it this way.
>> If this doesn't lead to errors in the compile test, I'm fine with this
>> (without the change for PPC only though).
> Dropping SENSORS_OCC can't result in duplicate symbols because it is a
> no-op. But while this is the easiest solution, it it not the cleanest
> in my opinion. OTOH, if you are certain that both modules will never be
> shipped at the same time, then it indeed doesn't matter that much. I
> can provide a patch going in either direction. Guenter, any preference?
>