| Message ID | 20190409144509.19da00c4@endymion (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: OCC drivers are PowerPC-only | expand |
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 > >
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 > > > > >
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?
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
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? >
--- 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
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(-)