linux-kernel - Re: [PATCH 2/2] hwmon: Allow to compile dell-smm-hwmon driver without /proc/i8k

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: <201503282304.51310@pali>
Date:	Sat, 28 Mar 2015 23:04:51 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Arnd Bergmann <arnd@...db.de>,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
	Jean Delvare <jdelvare@...e.de>,
	Steven Honeyman <stevenhoneyman@...il.com>,
	Valdis.Kletnieks@...edu,
	Jochen Eisinger <jochen@...guin-breeder.org>,
	Gabriele Mazzotta <gabriele.mzt@...il.com>,
	linux-kernel@...r.kernel.org, lm-sensors@...sensors.org
Subject: Re: [PATCH 2/2] hwmon: Allow to compile dell-smm-hwmon driver without /proc/i8k

On Saturday 28 March 2015 15:23:15 Guenter Roeck wrote:
> > +	---help---
> > +	  This hwmon driver adds support for reporting temperature
> > of different +	  sensors and controls the fans on Dell
> > laptops via System Management +	  Mode provided by Dell
> > BIOS.
> > +
> > +	  When option I8K is also enabled this driver provides
> > legacy /proc/i8k +	  userspace interface for i8kutils
> > package.
> > +
> 
> Please add this in alphabetic order.
> 

ok

> >   if ACPI
> >   
> >   comment "ACPI drivers"
> > 
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 1c3e458..9eec614 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -155,7 +155,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+=
> > w83l785ts.o
> > 
> >   obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
> >   obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
> >   obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
> > 
> > -obj-$(CONFIG_I8K)		+= dell-smm-hwmon.o
> > +obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
> 
> Same here.
> 

ok

> > -	proc_i8k = proc_create("i8k", 0, NULL, &i8k_fops);
> > -	if (!proc_i8k)
> > +	if (!proc_create("i8k", 0, NULL, &i8k_fops))
> > 
> >   		return -ENOENT;
> 
> I would prefer not to fail here but report a warning.
> This is no longer a fatal condition.
> 

ok, no problem

> 
> Can you move all the conditional functions and global
> variables together under a single #ifdef ? That should
> include functions to create the proc entries, and shim
> functions for the same if I8K is not configured.
> 

ok, I will move procfs code to one location.

> Also, the #ifdef would not cover the case where I8K is
> configured as module (there is no reason to force it to
> bool). You should use "#if IS_ENABLED(CONFIG_I8K)" instead.
> 

I think that setting CONFIG_I8K to tristate (Y/M/N) does not make 
sense... CONFIG_I8K just control if /proc/i8k will be compiled 
into dell-smm-hwmon or not.

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)