Re: [v6,1/1] hwmon: (nct7904) Fix incorrect temperature limitation register setting of LTD.
On Mon, 18 Jun 2085 15:57:19 +, amy.s...@advantech.com.tw wrote: > (...) I suggest you fix your system clock ;-) -- Jean Delvare SUSE L3 Support
[PATCH] hwmon: w83795: Fan control option isn't that dangerous
I have been using SENSORS_W83795_FANCTRL for several years and never had any problem. When the driver was added, I had not tested that part of the driver yet so I wanted to be super cautious, but time has shown that it works just fine. In the long run I even believe that we should drop the option and enable the feature unconditionally. It doesn't do anything until the user explicitly starts twiddling with sysfs attributes anyway. Signed-off-by: Jean Delvare --- drivers/hwmon/Kconfig |7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) --- linux-5.2.orig/drivers/hwmon/Kconfig2019-07-08 00:41:56.0 +0200 +++ linux-5.2/drivers/hwmon/Kconfig 2019-08-06 09:55:16.344547556 +0200 @@ -1834,17 +1834,12 @@ config SENSORS_W83795 will be called w83795. config SENSORS_W83795_FANCTRL - bool "Include automatic fan control support (DANGEROUS)" + bool "Include automatic fan control support" depends on SENSORS_W83795 help If you say yes here, support for automatic fan speed control will be included in the driver. - This part of the code wasn't carefully reviewed and tested yet, - so enabling this option is strongly discouraged on production - servers. Only developers and testers should enable it for the - time being. - Please also note that this option will create sysfs attribute files which may change in the future, so you shouldn't rely on them being stable. -- Jean Delvare SUSE L3 Support
Re: [PATCH] hwmon: Convert remaining drivers to use SPDX identifier
On Thu, 20 Jun 2019 06:19:46 -0700, Guenter Roeck wrote: > This gets rid of the unnecessary license boilerplate, and avoids > having to deal with individual patches one by one. > > No functional changes intended. > > Signed-off-by: Guenter Roeck > --- > drivers/hwmon/adm1029.c| 10 -- > drivers/hwmon/adt7411.c| 5 + > drivers/hwmon/adt7475.c| 5 + > drivers/hwmon/iio_hwmon.c | 5 + > drivers/hwmon/max197.c | 5 + > drivers/hwmon/scpi-hwmon.c | 10 +- > 6 files changed, 5 insertions(+), 35 deletions(-) > (...) Reviewed-by: Jean Delvare -- Jean Delvare SUSE L3 Support
Re: [PATCH] hwmon: (smsc47m1) fix outside array bounds warnings
Hi Masahiro,
On Tue, 21 May 2019 13:44:56 +0900, Masahiro Yamada wrote:
> Kbuild test robot reports outside array bounds warnings:
>
> CC [M] drivers/hwmon/smsc47m1.o
> drivers/hwmon/smsc47m1.c: In function 'fan_div_store':
> drivers/hwmon/smsc47m1.c:370:49: warning: array subscript [0, 2] is outside
> array bounds of 'u8[3]' {aka 'unsigned char[3]'} [-Warray-bounds]
> tmp = 192 - (old_div * (192 - data->fan_preload[nr])
> ~^~~~
> drivers/hwmon/smsc47m1.c:372:19: warning: array subscript [0, 2] is outside
> array bounds of 'u8[3]' {aka 'unsigned char[3]'} [-Warray-bounds]
> data->fan_preload[nr] = clamp_val(tmp, 0, 191);
> ~^~~~
> drivers/hwmon/smsc47m1.c:373:53: warning: array subscript [0, 2] is outside
> array bounds of 'const u8[3]' {aka 'const unsigned char[3]'} [-Warray-bounds]
> smsc47m1_write_value(data, SMSC47M1_REG_FAN_PRELOAD[nr],
> ^~~~
These messages are pretty confusing. Subscript [0, 2] would refer to a
bi-dimensional array, while these are 1-dimension arrays. If [0, 2]
means something else, I still don't get it, because both indexes 0 and
2 are perfectly within bounds of a 3-element array. So what do these
messages mean exactly? Looks like a bogus checker to me.
> The index field in the SENSOR_DEVICE_ATTR_R* defines is 0, 1, or 2.
> However, the compiler never knows the fact that the default in the
> switch statement is unreachable.
>
> Reported-by: kbuild test robot
> Signed-off-by: Masahiro Yamada
> ---
>
> drivers/hwmon/smsc47m1.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hwmon/smsc47m1.c b/drivers/hwmon/smsc47m1.c
> index 5f92eab24c62..e00102e05666 100644
> --- a/drivers/hwmon/smsc47m1.c
> +++ b/drivers/hwmon/smsc47m1.c
> @@ -364,6 +364,10 @@ static ssize_t fan_div_store(struct device *dev,
> tmp |= data->fan_div[2] << 4;
> smsc47m1_write_value(data, SMSC47M2_REG_FANDIV3, tmp);
> break;
> + default:
> + WARN_ON(1);
> + mutex_unlock(>update_lock);
> + return -EINVAL;
> }
So basically the code is fine, the checker (which checker, BTW?)
incorrectly thinks it isn't, and you propose to add dead code to make
the checker happy?
I disagree with this approach. Ideally the checker must be improved to
understand that the code is correct. If that's not possible, we should
be allowed to annotate the code to skip that specific check on these
specific lines, because it has been inspected by a knowledgeable human
and confirmed to be correct.
And if that it still not "possible", then the least intrusive fix would
be to make one of the valid cases the default. But adding new code
which will never be executed, but must still be compiled and stored,
no, thank you. Another code checker could legitimately complain about
that actually.
IMHO if code checkers return false positives then they are not helping
us and should not be used in the first place.
--
Jean Delvare
SUSE L3 Support
Re: [PATCH 2/2] docs: hwmon: remove the extension from .rst files
On Fri, 2019-04-19 at 07:30 -0300, Mauro Carvalho Chehab wrote: > On almost all places, we're including ReST files without the > extension. > > Let's remove the extension here as well, in order to use just > one standard. > Tab in the middle of a sentence. > Suggested-by: Jani Nikula The line above appears to be truncated. > Signed-off-by: Mauro Carvalho Chehab > --- > Documentation/hwmon/index.rst | 316 +- > 1 file changed, 158 insertions(+), 158 deletions(-) -- Jean Delvare SUSE L3 Support
Re: [PATCH v2 1/2] hwmon: (occ) Move common code to a separate module
On Wed, 10 Apr 2019 13:02:32 -0500, Eddie James wrote: > On 4/10/19 5:47 AM, Jean Delvare wrote: > > Instead of duplicating the common code into the 2 (binary) drivers, > > move the common code to a separate module. This is cleaner. > > > > Signed-off-by: Jean Delvare > > Cc: Eddie James > > Cc: Guenter Roeck > > --- > > Eddie, can you please give it a try and confirm it works? > > > Yes, this works well. > > Reviewed-by: Eddie James > > Tested-by: Eddie James Thanks. Can you please send a patch on top of that updating the help texts in Kconfig to explain that the drivers are for the BMC and not for the PowerPC host OS, as discussed before? Thanks, -- Jean Delvare SUSE L3 Support
[PATCH v2 2/2] hwmon: OCC drivers are ARM-only
These drivers are for a BMC inside PowerPC servers. The BMC runs on ARM hardware, so only propose the drivers on this architecture, unless build-testing. Signed-off-by: Jean Delvare Cc: Eddie James Cc: Guenter Roeck --- If PowerPC BMCs are ever based on another architecture and these drivers are compatible with them, then the list can be extended. drivers/hwmon/occ/Kconfig |2 ++ 1 file changed, 2 insertions(+) --- linux-5.0.orig/drivers/hwmon/occ/Kconfig2019-04-10 11:54:07.014895111 +0200 +++ linux-5.0/drivers/hwmon/occ/Kconfig 2019-04-10 12:12:27.379725640 +0200 @@ -5,6 +5,7 @@ config SENSORS_OCC_P8_I2C tristate "POWER8 OCC through I2C" depends on I2C + depends on ARM || ARM64 || COMPILE_TEST select SENSORS_OCC help This option enables support for monitoring sensors provided by the @@ -17,6 +18,7 @@ config SENSORS_OCC_P8_I2C config SENSORS_OCC_P9_SBE tristate "POWER9 OCC through SBE" depends on FSI_OCC + depends on ARM || ARM64 || COMPILE_TEST select SENSORS_OCC help This option enables support for monitoring sensors provided by the -- Jean Delvare SUSE L3 Support
[PATCH v2 1/2] hwmon: (occ) Move common code to a separate module
Instead of duplicating the common code into the 2 (binary) drivers,
move the common code to a separate module. This is cleaner.
Signed-off-by: Jean Delvare
Cc: Eddie James
Cc: Guenter Roeck
---
Eddie, can you please give it a try and confirm it works?
Note: I kept the module names as they were before, hence the extra
"*-objs :=" statements. They could be removed if we rename the source
files, but that's better done in git directly. I don't mind either way
personally.
drivers/hwmon/occ/Kconfig |3 +--
drivers/hwmon/occ/Makefile |6 --
drivers/hwmon/occ/common.c |7 +++
drivers/hwmon/occ/sysfs.c |2 ++
4 files changed, 14 insertions(+), 4 deletions(-)
--- linux-5.0.orig/drivers/hwmon/occ/Kconfig2019-04-10 11:30:05.579537638
+0200
+++ linux-5.0/drivers/hwmon/occ/Kconfig 2019-04-10 11:31:20.843383376 +0200
@@ -27,5 +27,4 @@ config SENSORS_OCC_P9_SBE
called occ-p9-hwmon.
config SENSORS_OCC
- bool "POWER On-Chip Controller"
- depends on SENSORS_OCC_P8_I2C || SENSORS_OCC_P9_SBE
+ tristate
--- linux-5.0.orig/drivers/hwmon/occ/Makefile 2019-03-04 00:21:29.0
+0100
+++ linux-5.0/drivers/hwmon/occ/Makefile2019-04-10 11:33:23.631765535
+0200
@@ -1,5 +1,7 @@
-occ-p8-hwmon-objs := common.o sysfs.o p8_i2c.o
-occ-p9-hwmon-objs := common.o sysfs.o p9_sbe.o
+occ-hwmon-common-objs := common.o sysfs.o
+occ-p8-hwmon-objs := p8_i2c.o
+occ-p9-hwmon-objs := p9_sbe.o
+obj-$(CONFIG_SENSORS_OCC) += occ-hwmon-common.o
obj-$(CONFIG_SENSORS_OCC_P8_I2C) += occ-p8-hwmon.o
obj-$(CONFIG_SENSORS_OCC_P9_SBE) += occ-p9-hwmon.o
--- linux-5.0.orig/drivers/hwmon/occ/common.c 2019-03-04 00:21:29.0
+0100
+++ linux-5.0/drivers/hwmon/occ/common.c2019-04-10 11:44:53.035573580
+0200
@@ -1,11 +1,13 @@
// SPDX-License-Identifier: GPL-2.0
#include
+#include
#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -1096,3 +1098,8 @@ int occ_setup(struct occ *occ, const cha
return rc;
}
+EXPORT_SYMBOL_GPL(occ_setup);
+
+MODULE_AUTHOR("Eddie James ");
+MODULE_DESCRIPTION("Common OCC hwmon code");
+MODULE_LICENSE("GPL");
--- linux-5.0.orig/drivers/hwmon/occ/sysfs.c2019-03-04 00:21:29.0
+0100
+++ linux-5.0/drivers/hwmon/occ/sysfs.c 2019-04-10 11:39:38.627003382 +0200
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
#include
@@ -186,3 +187,4 @@ void occ_shutdown(struct occ *occ)
{
sysfs_remove_group(>bus_dev->kobj, _sysfs);
}
+EXPORT_SYMBOL_GPL(occ_shutdown);
--
Jean Delvare
SUSE L3 Support
[PATCH] hwmon: OCC drivers are PowerPC-only
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 Cc: Eddie James Cc: Guenter Roeck --- 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/Kconfig2019-03-04 00:21:29.0 +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.0 +0100 +++ linux-5.0/drivers/hwmon/Makefile2019-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: libsensors soname bump
Hi Ondřej, On Tue, 18 Dec 2018 18:06:00 +0100, Ondřej Lysoněk wrote: > On 16. 12. 18 12:43, Jean Delvare wrote: > > Would you consider quickly releasing lm-sensors 3.5.1 with the proper > > library version number, to save all that work to all application > > authors/maintainers and distribution package maintainers? > > I did consider it, but I'm afraid I can't do this, sorry. I'm afraid it > would cause more problems than it would solve. > > lm_sensors 3.5.0 has already been picked up by several distributions > [1], and at least Arch Linux, Slackware and Gentoo have already rebuilt > the dependent packages. It feels wrong to force the people, who already > picked up the new version, to do the work again. I've spoken to the > Gentoo maintainer and he's not thrilled about doing that. I'm really not > looking forward to another batch of angry breakage reports after doing > another soname change. > > Doing the 3.5.1 release would also mean that everyone using 3.5.0 would > have to upgrade, otherwise the values of the SENSORS_API_VERSION macro > would be different on different systems, forcing users of that macro to > deal with the mess. OK. Thank you for taking the time to investigate the option. I understand and respect your decision. > I suggest that you use the patch that reverts the soname change in > OpenSUSE, but keep the new SENSORS_API_VERSION so that it remains > consistent with other distros. You are right. I have reverted the change to SENSORS_API_VERSION from my SUSE-local patch. While there was some beauty in having a "structured" SENSORS_API_VERSION, there is a lot more value in using the same value as upstream. -- Jean Delvare SUSE L3 Support
Re: libsensors soname bump
On Mon, 17 Dec 2018 11:35:46 +0100, Ondřej Lysoněk wrote: > I mean, I would love to revert the soname change, however doing so now > seems like a bad thing to do - people may have already adopted > lm_sensors 3.5.0. So I'd like to avoid reverting the change unless there > is a good justification to do so. It's not in Debian yet: https://packages.debian.org/sid/lm-sensors Gentoo have picked it but it is still in testing: https://packages.gentoo.org/packages/sys-apps/lm_sensors You'll know better than I do about Fedora. For openSUSE, I have packaged lm-sensors 3.5.0 but with a patch reverting the undue soname change. I used "4.5.0" instead. I intend to carry this patch for as long as I have to. This goes against our policy of sticking to upstream as much as possible, but in this specific case, I consider it the least of 2 evils. Thanks, -- Jean Delvare SUSE L3 Support
Re: libsensors soname bump
On Mon, 17 Dec 2018 10:46:43 +0100, Ondřej Lysoněk wrote: > On 16. 12. 18 12:43, Jean Delvare wrote: > > You have recently released lm-sensors 3.5.0 with a new soname for > > libsensors: > > > > -LIBMAINVER := 4 > > -LIBMINORVER := 4.0 > > +LIBMAINVER := 5 > > +LIBMINORVER := 0.0 > > > > -#define SENSORS_API_VERSION0x440 > > +#define SENSORS_API_VERSION0x500 > > > > This is declaring the new library as incompatible with the previous > > version, meaning that distributions will have to build and ship both > > libsensors4 and libsensors5 for a long time until all applications have > > been updated and rebuilt to link with the new library. This is a > > significant effort for the whole community and should only be done when > > necessary. > > I thought there was an ABI change, which would warrant a soname bump. Or > am I wrong in thinking that? I was mistaken however and I'm sorry about > that, there was no ABI change. > > I don't see why distributions would have to ship two versions of the > library. The *name* of the library didn't change, it's still libsensors > (not libsensors4 or libsensors5). But the packages as they exist today, they link with libsensors.so.4, not libsensors.so: $ rpm -q --requires sensors | grep sensors libsensors.so.4()(64bit) Having only one library works only if you update the whole system at once. That can work in some specific cases I suppose, but in practice can be hard to achieve in a complex ecosystem without breaking things "temporarily". That's really not the kind of thing users enjoy. The whole point of using a distribution is that things just work, all the time. So the standard way to handle major library version changes is to provide both in parallel at first, then migrate all applications, and once all applications have been migrated, deprecate the old library. And after a few years, remove that old library. > [~/git/lm-sensors]$ make > ... > [~/git/lm-sensors]$ ls lib/ | grep libsensors.so > libsensors.so > libsensors.so.5 > libsensors.so.5.0.0 > > So all distros need to do is rebuild dependent packages. No changes to > other packages should be required. Well, that alone is already pretty painful. Having to rebuild, ship, download and install all these applications is a major waste of resources. That's no less that 98 packages for my distribution. The whole point of having shared libraries is to make it possible to update one package and leave the rest unchanged. Changing the soname breaks that possibility. For us (openSUSE) it would also require changing the BuildRequires of all these applications from "libsensors4-devel" to "libsensors5-devel". Maybe that package should be named "libsensors-devel" instead, I'm not sure why we versioned it, I see that most libraries are not doing that. Maybe we had to when transitioning from lm-sensors 2 to lm-sensors 3. I don't remember the details. > Am I missing something? There are also all the 3rd party applications. Either shipped as binaries by proprietary software editors, or built in the Open Build Service. Changing the library version with no reason breaks compatibility with them. Without the soname change, it is possible for a user to cherry-pick a package update they need, independent of what the distribution has. For example, a user may need the latest version of libsensors because it fixes a bug for them. They could get it from the Open Build Service easily... But now they won't be able to install it, because the soname is incompatible with their current distribution. They would either have to pull all the packages using libsensors from the Open Build System (which can be very painful if some of these packages also have complex dependencies), or changing distributions altogether. This is opposite to the flexibility we intend to provide to our users. The major version of a library should only ever be changed for incompatible changes, that is, if an application built for the old version of the library will fail to run with the new version of the library. If this is not the case then then major version must stay the same. Thanks, -- Jean Delvare SUSE L3 Support
libsensors soname bump
Hi Ondřej, You have recently released lm-sensors 3.5.0 with a new soname for libsensors: -LIBMAINVER := 4 -LIBMINORVER := 4.0 +LIBMAINVER := 5 +LIBMINORVER := 0.0 -#define SENSORS_API_VERSION0x440 +#define SENSORS_API_VERSION0x500 This is declaring the new library as incompatible with the previous version, meaning that distributions will have to build and ship both libsensors4 and libsensors5 for a long time until all applications have been updated and rebuilt to link with the new library. This is a significant effort for the whole community and should only be done when necessary. In this specific case, I can't see what warranted such a change of major library version change. From lm-sensors/doc/developers/release_checklist: Remember: update main number when interface changes, minor if new functionality is added, and patch if only bugs are fixed. In this case I can only see new functionality added, there is no interface change. Therefore the correct value for SENSORS_API_VERSION was 0x450, not 0x500. This would avoid the parallel maintenance and installation of 2 versions of the library for several years to come. Would you consider quickly releasing lm-sensors 3.5.1 with the proper library version number, to save all that work to all application authors/maintainers and distribution package maintainers? Thanks, -- Jean Delvare SUSE L3 Support
Re: How to detect which device is mapped to hwmonX ?
On Wed, 12 Dec 2018 10:54:19 +0200, Ranran wrote: > On Tue, Dec 11, 2018 at 11:13 PM Guenter Roeck wrote: > > > > On Tue, Dec 11, 2018 at 10:30:58PM +0200, Ranran wrote: > > > Hello, > > > > > > I have several nodes /sys/class/hwmon/hwmonX , > > > but checking the fields I could not find yet a way how to identify a > > > device (for example i2c address) to the given mapping of hwmonX. > > > > > > Is there a way to find out which device is mapped to which hwmonX ? > > > > > > > Maybe libsensors would work for you ? > > > Thanks , I will check that. > But I was looking for something to check in /sys or /proc to get the matching. > Probably if libsensor somehow knows the mapping it also looks there, No ? libsensors looks in /sys indeed but it's not trivial and libsensors does not offer an API for it. I remember writing a libsensors-based name look-up tool years ago. I'm a bit in a hurry but I have copied what I have at: http://jdelvare.nerim.net/devel/lm-sensors/patches/ Maybe we can revive the idea if there is a need. -- Jean Delvare SUSE L3 Support
Re: [PATCH] hwmon: (lm92) Fix whitespace issues
On Sun, 2018-09-16 at 07:47 -0700, Guenter Roeck wrote: > Sparse complains: > > drivers/hwmon/lm92.c:209 set_temp_hyst() warn: inconsistent indenting > > While at it, fix various other whitespace issues reported by checkpatch > (double empty lines, missing empty lines, whitespace at empty line). > > Signed-off-by: Guenter Roeck > --- > drivers/hwmon/lm92.c | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) > (...) Reviewed-by: Jean Delvare Thanks Guenter, -- Jean Delvare SUSE L3 Support
Re: [PATCH] hwmon: Mark expected switch fall-throughs
Hi Gustavo,
On Mon, 2 Jul 2018 16:30:17 -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
That's a great initiative, thanks for doing that.
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/hwmon/emc1403.c | 2 ++
> drivers/hwmon/nct6775.c | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c
> index 1ea7ca5..aaebeb7 100644
> --- a/drivers/hwmon/emc1403.c
> +++ b/drivers/hwmon/emc1403.c
> @@ -443,8 +443,10 @@ static int emc1403_probe(struct i2c_client *client,
> switch (id->driver_data) {
> case emc1404:
> data->groups[2] = _group;
> + /* fall through */
> case emc1403:
> data->groups[1] = _group;
> + /* fall through */
> case emc1402:
> data->groups[0] = _group;
> }
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 3714990..c6bd61e 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -2541,7 +2541,7 @@ static void pwm_update_registers(struct nct6775_data
> *data, int nr)
> case thermal_cruise:
> nct6775_write_value(data, data->REG_TARGET[nr],
> data->target_temp[nr]);
> - /* intentional */
> + /* fall through */
Do you know which -Wimplicit-fallthrough level we will be using? Level
1 would be happy with the initial comment, no change would be needed.
If level 2-4 then yes the change is needed.
> default:
> reg = nct6775_read_value(data, data->REG_FAN_MODE[nr]);
> reg = (reg & ~data->tolerance_mask) |
Reviewed-by: Jean Delvare
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: (lm92) Add max6635 to lm92_id[]
On Thu, 22 Mar 2018 15:37:44 +0100, Alvaro Gamez Machado wrote:
> Since autodetection of this chip was removed, it makes sense to add prefix
> max6635 so that the device can be instantiated by its actual name.
>
> Signed-off-by: Alvaro Gamez Machado <alvaro.ga...@hazent.com>
> ---
> Differences:
> * Update also Documentation/hwmon/lm92 with new prefix to use
>
> Documentation/hwmon/lm92 | 2 +-
> drivers/hwmon/lm92.c | 5 +++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/hwmon/lm92 b/Documentation/hwmon/lm92
> index f2a5adcf4ead..cfa99a353b8c 100644
> --- a/Documentation/hwmon/lm92
> +++ b/Documentation/hwmon/lm92
> @@ -11,7 +11,7 @@ Supported chips:
> Addresses scanned: none, force parameter needed
> Datasheet: http://www.national.com/pf/LM/LM76.html
>* Maxim MAX6633/MAX6634/MAX6635
> -Prefix: 'lm92'
> +Prefix: 'max6635'
> Addresses scanned: none, force parameter needed
> Datasheet: http://www.maxim-ic.com/quick_view2.cfm/qv_pk/3074
>
> diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c
> index 18509b5af11e..d40fe5122e94 100644
> --- a/drivers/hwmon/lm92.c
> +++ b/drivers/hwmon/lm92.c
> @@ -52,6 +52,7 @@
> */
> static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> I2C_CLIENT_END };
> +enum chips { lm92, max6635 };
>
> /* The LM92 registers */
> #define LM92_REG_CONFIG 0x01 /* 8-bit, RW */
> @@ -329,8 +330,8 @@ static int lm92_probe(struct i2c_client *new_client,
> */
>
> static const struct i2c_device_id lm92_id[] = {
> - { "lm92", 0 },
> - /* max6635 could be added here */
> + { "lm92", lm92 },
> + { "max6635", max6635 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, lm92_id);
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (lm92) Add max6635 to lm92_id[]
Hi Alvaro,
On Thu, 22 Mar 2018 14:43:11 +0100, Alvaro Gamez Machado wrote:
> Since autodetection of this chip was removed, it makes sense to add prefix
> max6635 so that the device can be instantiated by its actual name.
>
> Signed-off-by: Alvaro Gamez Machado <alvaro.ga...@hazent.com>
> ---
> drivers/hwmon/lm92.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c
> index 18509b5af11e..d40fe5122e94 100644
> --- a/drivers/hwmon/lm92.c
> +++ b/drivers/hwmon/lm92.c
> @@ -52,6 +52,7 @@
> */
> static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> I2C_CLIENT_END };
> +enum chips { lm92, max6635 };
>
> /* The LM92 registers */
> #define LM92_REG_CONFIG 0x01 /* 8-bit, RW */
> @@ -329,8 +330,8 @@ static int lm92_probe(struct i2c_client *new_client,
> */
>
> static const struct i2c_device_id lm92_id[] = {
> - { "lm92", 0 },
> - /* max6635 could be added here */
> + { "lm92", lm92 },
> + { "max6635", max6635 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, lm92_id);
As the driver doesn't treat the two devices differently, the enum isn't
really needed. I don't really mind though, just a notice.
Reviewed-by: Jean Delvare <jdelv...@suse.de>
Please also update Documentation/hwmon/lm92 which currently claims that
the max6635 must use "lm92" as prefix.
Thanks,
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: (lm92) Remove spurious test that prevented max6635 detection
Hi Alvaro, On Wed, 21 Mar 2018 10:27:09 +0100, Alvaro Gamez Machado wrote: > An assumption was made that reads of a max6635 LM92_REG_MAN_ID register > would return the last value read from any other of its registers. > > This assumption is not only something that is not documented anywhere, Correct. > but it is also untrue, so this prevented max6635 recognition. You can easily imagine that it must have been true at some point. Do you really believe some developer would put that code in the driver just for fun? > > Signed-off-by: Alvaro Gamez Machado <alvaro.ga...@hazent.com> > --- > > Changes in v2: > Sorry, I seem to be slightly dyslexic and keep confusing max6635 <> max6335 > > > drivers/hwmon/lm92.c | 8 > 1 file changed, 8 deletions(-) > > diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c > index 2a91974a10bb..a2c3f3080192 100644 > --- a/drivers/hwmon/lm92.c > +++ b/drivers/hwmon/lm92.c > @@ -272,16 +272,8 @@ static int max6635_check(struct i2c_client *client) > u8 conf; > int i; > > - /* > - * No manufacturer ID register, so a read from this address will > - * always return the last read value. > - */ > temp_low = i2c_smbus_read_word_data(client, LM92_REG_TEMP_LOW); > - if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_low) > - return 0; > temp_high = i2c_smbus_read_word_data(client, LM92_REG_TEMP_HIGH); > - if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_high) > - return 0; > > /* Limits are stored as integer values (signed, 9-bit). */ > if ((temp_low & 0x7f00) || (temp_high & 0x7f00)) The detection was already rather weak for these devices. With this step removed, it is below what I think is acceptable in terms of reliability, as it could easily misdetect another chip and accidentally bind to it. For a driver which uses SMBus word transactions, this could be pretty problematic. So if register LM92_REG_MAN_ID does not contain any useful value on the MAX6635, I would rather drop auto-detection of these devices altogether, and only support them when explicitly instantiated. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] hwmon: Drop reference to Jean's tree
This tree has not been used for over a year, Guenter is taking all the hwmon patches in practice. Signed-off-by: Jean Delvare <jdelv...@suse.de> Cc: Guenter Roeck <li...@roeck-us.net> --- MAINTAINERS |1 - 1 file changed, 1 deletion(-) --- linux-4.14.orig/MAINTAINERS 2017-11-12 19:46:13.0 +0100 +++ linux-4.14/MAINTAINERS 2017-11-21 17:07:29.991926400 +0100 @@ -6072,7 +6072,6 @@ M: Jean Delvare <jdelv...@suse.com> M: Guenter Roeck <li...@roeck-us.net> L: linux-hwmon@vger.kernel.org W: http://hwmon.wiki.kernel.org/ -T: quilt http://jdelvare.nerim.net/devel/linux/jdelvare-hwmon/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git S: Maintained F: Documentation/hwmon/ -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: Drop unnecessary 'default n' from Kconfig
Hi Guenter, On Sun, 19 Nov 2017 09:16:02 -0800, Guenter Roeck wrote: > 'default n' is default, so there is no need to specify it explicitly. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > drivers/hwmon/Kconfig | 16 > 1 file changed, 16 deletions(-) > (...) Reviewed-by: Jean Delvare <jdelv...@suse.de> (and heartily approved - thanks for doing that) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 12/15] hwmon: (w83793) Remove duplicate NULL check
On Tue, 31 Oct 2017 16:21:46 +0200, Andy Shevchenko wrote: > Since i2c_unregister_device() became NULL-aware we may remove duplicate > NULL check. > > Cc: Rudolf Marek <r.ma...@assembler.cz> > Cc: Jean Delvare <jdelv...@suse.com> > Cc: Guenter Roeck <li...@roeck-us.net> > Cc: linux-hwmon@vger.kernel.org > Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> > --- > drivers/hwmon/w83793.c | 15 +-- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c > index 5ba9d9f1daa1..0af0f6283b35 100644 > --- a/drivers/hwmon/w83793.c > +++ b/drivers/hwmon/w83793.c > @@ -1564,10 +1564,8 @@ static int w83793_remove(struct i2c_client *client) > for (i = 0; i < ARRAY_SIZE(w83793_temp); i++) > device_remove_file(dev, _temp[i].dev_attr); > > - if (data->lm75[0] != NULL) > - i2c_unregister_device(data->lm75[0]); > - if (data->lm75[1] != NULL) > - i2c_unregister_device(data->lm75[1]); > + i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[1]); > > /* Decrease data reference counter */ > mutex_lock(_data_mutex); > @@ -1625,8 +1623,7 @@ w83793_detect_subclients(struct i2c_client *client) > /* Undo inits in case of errors */ > > ERROR_SC_1: > - if (data->lm75[0] != NULL) > - i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[0]); > ERROR_SC_0: > return err; > } > @@ -1962,10 +1959,8 @@ static int w83793_probe(struct i2c_client *client, > for (i = 0; i < ARRAY_SIZE(w83793_temp); i++) > device_remove_file(dev, _temp[i].dev_attr); > > - if (data->lm75[0] != NULL) > - i2c_unregister_device(data->lm75[0]); > - if (data->lm75[1] != NULL) > - i2c_unregister_device(data->lm75[1]); > + i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[1]); > free_mem: > kfree(data); > exit: Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 10/15] hwmon: (w83791d) Remove duplicate NULL check
On Tue, 31 Oct 2017 16:21:44 +0200, Andy Shevchenko wrote: > Since i2c_unregister_device() became NULL-aware we may remove duplicate > NULL check. > > Cc: Marc Hulsman <m.huls...@tudelft.nl> > Cc: Jean Delvare <jdelv...@suse.com> > Cc: Guenter Roeck <li...@roeck-us.net> > Cc: linux-hwmon@vger.kernel.org > Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> > --- > drivers/hwmon/w83791d.c | 15 +-- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/hwmon/w83791d.c b/drivers/hwmon/w83791d.c > index 8af6081b4ab4..28fa3bd2c096 100644 > --- a/drivers/hwmon/w83791d.c > +++ b/drivers/hwmon/w83791d.c > @@ -1316,8 +1316,7 @@ static int w83791d_detect_subclients(struct i2c_client > *client) > /* Undo inits in case of errors */ > > error_sc_1: > - if (data->lm75[0] != NULL) > - i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[0]); > error_sc_0: > return err; > } > @@ -1434,10 +1433,8 @@ static int w83791d_probe(struct i2c_client *client, > error4: > sysfs_remove_group(>dev.kobj, _group); > error3: > - if (data->lm75[0] != NULL) > - i2c_unregister_device(data->lm75[0]); > - if (data->lm75[1] != NULL) > - i2c_unregister_device(data->lm75[1]); > + i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[1]); > return err; > } > > @@ -1448,10 +1445,8 @@ static int w83791d_remove(struct i2c_client *client) > hwmon_device_unregister(data->hwmon_dev); > sysfs_remove_group(>dev.kobj, _group); > > - if (data->lm75[0] != NULL) > - i2c_unregister_device(data->lm75[0]); > - if (data->lm75[1] != NULL) > - i2c_unregister_device(data->lm75[1]); > + i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[1]); > > return 0; > } Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 09/15] hwmon: (w83781d) Remove duplicate NULL check
On Tue, 31 Oct 2017 16:21:43 +0200, Andy Shevchenko wrote: > Since i2c_unregister_device() became NULL-aware we may remove duplicate > NULL check. > > Cc: Jean Delvare <jdelv...@suse.com> > Cc: Guenter Roeck <li...@roeck-us.net> > Cc: linux-hwmon@vger.kernel.org > Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> > --- > drivers/hwmon/w83781d.c | 12 > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwmon/w83781d.c b/drivers/hwmon/w83781d.c > index 246fb2365126..2b0f182daa87 100644 > --- a/drivers/hwmon/w83781d.c > +++ b/drivers/hwmon/w83781d.c > @@ -1246,10 +1246,8 @@ w83781d_probe(struct i2c_client *client, const struct > i2c_device_id *id) > > exit_remove_files: > w83781d_remove_files(dev); > - if (data->lm75[0]) > - i2c_unregister_device(data->lm75[0]); > - if (data->lm75[1]) > - i2c_unregister_device(data->lm75[1]); > + i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[1]); > return err; > } > > @@ -1262,10 +1260,8 @@ w83781d_remove(struct i2c_client *client) > hwmon_device_unregister(data->hwmon_dev); > w83781d_remove_files(dev); > > - if (data->lm75[0]) > - i2c_unregister_device(data->lm75[0]); > - if (data->lm75[1]) > - i2c_unregister_device(data->lm75[1]); > + i2c_unregister_device(data->lm75[0]); > + i2c_unregister_device(data->lm75[1]); > > return 0; > } Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tmp102: Fix first temperature reading
Hi Guenter,
On Mon, 23 Oct 2017 17:55:26 -0700, Guenter Roeck wrote:
> Commit 3d8f7a89a197 ("hwmon: (tmp102) Improve handling of initial read
> delay") reduced the initial temperature read delay and made it dependent
> on the chip's shutdown mode. If the chip was not in shutdown mode at probe,
> the read delay no longer applies.
>
> This ignores the fact that the chip initialization changes the temperature
> sensor resolution, and that the temperature register values change when
> the resolution is changed. As a result, the reported temperature is twice
> as high as the real temperature until the first temperature conversion
> after the configuration change is complete. This can result in unexpected
> behavior and, worst case, in a system shutdown. To fix the problem,
> let's just always wait for a conversion to complete before reporting
> a temperature.
>
> Fixes: 3d8f7a89a197 ("hwmon: (tmp102) Improve handling of initial read delay")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197167
> Reported-by: Ralf Goebel <ralf.goe...@imago-technologies.com>
> Cc: Ralf Goebel <ralf.goe...@imago-technologies.com>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/tmp102.c | 9 +
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
> index 5eafbaada795..1af1932a72db 100644
> --- a/drivers/hwmon/tmp102.c
> +++ b/drivers/hwmon/tmp102.c
> @@ -268,14 +268,7 @@ static int tmp102_probe(struct i2c_client *client,
> return err;
> }
>
> - tmp102->ready_time = jiffies;
> - if (tmp102->config_orig & TMP102_CONF_SD) {
> - /*
> - * Mark that we are not ready with data until the first
> - * conversion is complete
> - */
> - tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
> - }
> + tmp102->ready_time = jiffies + msecs_to_jiffies(CONVERSION_TIME_MS);
>
> hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> tmp102,
Thanks for the quick fix. The change itself looks good but why remove
the comment, which I think still retains its value?
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] hwmon: (stts751) Fix buffer size passed to snprintf
Function snprintf already cares for the terminating NUL at the end of
the string, the caller doesn't need to do it.
Signed-off-by: Jean Delvare <jdelv...@suse.de>
Cc: Andrea Merello <andrea.mere...@gmail.com>
Cc: Guenter Roeck <li...@roeck-us.net>
---
drivers/hwmon/stts751.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
--- linux-4.14-rc1.orig/drivers/hwmon/stts751.c 2017-09-17 00:47:51.0
+0200
+++ linux-4.14-rc1/drivers/hwmon/stts751.c 2017-09-19 14:51:39.773843565
+0200
@@ -396,7 +396,7 @@ static ssize_t show_max_alarm(struct dev
if (ret < 0)
return ret;
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->max_alert);
}
static ssize_t show_min_alarm(struct device *dev, struct device_attribute
*attr,
@@ -413,7 +413,7 @@ static ssize_t show_min_alarm(struct dev
if (ret < 0)
return ret;
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->min_alert);
}
static ssize_t show_input(struct device *dev, struct device_attribute *attr,
@@ -428,7 +428,7 @@ static ssize_t show_input(struct device
if (ret < 0)
return ret;
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->temp);
}
static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
@@ -436,7 +436,7 @@ static ssize_t show_therm(struct device
{
struct stts751_priv *priv = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->therm);
}
static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
@@ -478,7 +478,7 @@ static ssize_t show_hyst(struct device *
{
struct stts751_priv *priv = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->hyst);
}
static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
@@ -518,7 +518,7 @@ static ssize_t show_therm_trip(struct de
if (ret < 0)
return ret;
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm_trip);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->therm_trip);
}
static ssize_t show_max(struct device *dev, struct device_attribute *attr,
@@ -526,7 +526,7 @@ static ssize_t show_max(struct device *d
{
struct stts751_priv *priv = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->event_max);
}
static ssize_t set_max(struct device *dev, struct device_attribute *attr,
@@ -560,7 +560,7 @@ static ssize_t show_min(struct device *d
{
struct stts751_priv *priv = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
+ return snprintf(buf, PAGE_SIZE, "%d\n", priv->event_min);
}
static ssize_t set_min(struct device *dev, struct device_attribute *attr,
@@ -594,7 +594,7 @@ static ssize_t show_interval(struct devi
{
struct stts751_priv *priv = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+ return snprintf(buf, PAGE_SIZE, "%d\n",
stts751_intervals[priv->interval]);
}
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: adt7475: wusbhc: constify attribute_group structures.
Hi Arvind,
On lun., 2017-08-07 at 11:49 +0530, Arvind Yadav wrote:
> attribute_group are not supposed to change at runtime. All functions
> working with attribute_group provided by work with
> const attribute_group. So mark the non-const structs as const.
Confused by the subject. What is "wusbhc:"?
Also please avoid trailing dot in mail subjects.
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
> ---
> drivers/hwmon/adt7475.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index 1baa213..9ef8499 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -1319,14 +1319,14 @@ static SENSOR_DEVICE_ATTR_2(pwm3_stall_disable,
> S_IRUGO | S_IWUSR,
> NULL
> };
>
> -static struct attribute_group adt7475_attr_group = { .attrs = adt7475_attrs
> };
> -static struct attribute_group fan4_attr_group = { .attrs = fan4_attrs };
> -static struct attribute_group pwm2_attr_group = { .attrs = pwm2_attrs };
> -static struct attribute_group in0_attr_group = { .attrs = in0_attrs };
> -static struct attribute_group in3_attr_group = { .attrs = in3_attrs };
> -static struct attribute_group in4_attr_group = { .attrs = in4_attrs };
> -static struct attribute_group in5_attr_group = { .attrs = in5_attrs };
> -static struct attribute_group vid_attr_group = { .attrs = vid_attrs };
> +static const struct attribute_group adt7475_attr_group = { .attrs =
> adt7475_attrs };
> +static const struct attribute_group fan4_attr_group = { .attrs = fan4_attrs
> };
> +static const struct attribute_group pwm2_attr_group = { .attrs = pwm2_attrs
> };
> +static const struct attribute_group in0_attr_group = { .attrs = in0_attrs };
> +static const struct attribute_group in3_attr_group = { .attrs = in3_attrs };
> +static const struct attribute_group in4_attr_group = { .attrs = in4_attrs };
> +static const struct attribute_group in5_attr_group = { .attrs = in5_attrs };
> +static const struct attribute_group vid_attr_group = { .attrs = vid_attrs };
>
> static int adt7475_detect(struct i2c_client *client,
> struct i2c_board_info *info)
Looks good to me.
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: userspace regression with hwmon
On Tue, 27 Jun 2017 05:52:39 -0700, Guenter Roeck wrote: > On 06/27/2017 05:23 AM, Sudip Mukherjee wrote: > > Well, using the raw platform device path was a bad design decision. > > But I still see a difference. In v3.8 all the temp* nodes were in > > /sys/class/hwmon/hwmon0/device/ but in v4.4 I can see that they are in > > /sys/class/hwmon/hwmon0/. So if we do modify the code, then we still > > need to have two versions of userspace code based on the kernel. > > Your code should check for the 'name' attribute in both locations. ... as libsensors does. If you just do what libsensors does, you'll be on the safe side. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (it87) Remove useless test during device detection
On Thu, 4 May 2017 11:35:06 +0200, Jean Delvare wrote: > There is no reason to treat the IT8705F differently during device > detection. If a single IT8705F chip indeed answers to both Super-IO > addresses, we have code in place to detect the duplicate device > address and skip the second one. > (...) Bah, scratch this. I can't even convince myself that this is a good idea. Sure, the rest of the code is enough to deal with the situation, but why keep looking for something when we already know we will find and discard a duplicate... Sorry for the noise, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] hwmon-NCT: Fine-tuning for four function implementations
On Thu, 27 Apr 2017 11:27:21 +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Thu, 27 Apr 2017 11:18:22 +0200 > > A few update suggestions were taken into account > from static source code analysis. > > Markus Elfring (4): > Use devm_kcalloc() in nct6683_create_attr_group() > Adjust five checks for null pointers > Use devm_kcalloc() in nct6775_create_attr_group() > Adjust seven checks for null pointers > > drivers/hwmon/nct6683.c | 15 +++ > drivers/hwmon/nct6775.c | 19 +-- > 2 files changed, 16 insertions(+), 18 deletions(-) Nack. You have proven in the past on various lists that 1* you have no understanding about the changes you are proposing and 2* you have no skills to discuss any problem that could be found in your patches. So I'll make it straight: we will not take any patch from you. You are an annoyance we don't need. Do not waste our time. Go away. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: it87 causes VIA hardware to lockup
Hi Guenter, On Sun, 9 Apr 2017 08:24:07 -0700, Guenter Roeck wrote: > On Sun, Apr 09, 2017 at 03:38:06PM +0200, Jean Delvare wrote: > > On Tue, 21 Mar 2017 10:05:03 -0700, Guenter Roeck wrote: > > > I'll submit the patch as-is upstream; at least it doesn't break anything. > > > If it doesn't fix your problem, we'll have to look at it again at a later > > > point. > > > > Given that this patch fixes a regression in kernels v4.7 to v.4.10, > > shouldn't it go to stable@? > > The patch has a Fixes: tag, so that should happen automatically. > I'll have to check if it applies cleanly to earlier kernels and if > necessary send backport(s) to Greg. I took a look at stable branches v4.9 and v4.10 and I can't find this fix. Do you still plan to check if the fix applies and poke Greg about it? Or do you want me to do it? > > As a side note, I think the second half of the patch is redundant, it > > only makes registration slightly faster on IT8705F, and could have bad > > side effects at least in theory. The first half seems sufficient to > > me... > > It only affects systems with two Super-IO chips, and I wanted to play safe. > The > worst side effect I can imagine would be that a second chip in a system with > IT8705 as first chip would not be accepted, which is not worse than before > when only one chip was supported. But isn't as good as doing the right thing, which would require less code. So I don't really follow your logic. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: it87 causes VIA hardware to lockup
Hi Guenter, On Tue, 21 Mar 2017 10:05:03 -0700, Guenter Roeck wrote: > I'll submit the patch as-is upstream; at least it doesn't break anything. > If it doesn't fix your problem, we'll have to look at it again at a later > point. Given that this patch fixes a regression in kernels v4.7 to v.4.10, shouldn't it go to stable@? As a side note, I think the second half of the patch is redundant, it only makes registration slightly faster on IT8705F, and could have bad side effects at least in theory. The first half seems sufficient to me... -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (dell-smm) Add Dell XPS 15 9560 into DMI list
On Wed, 8 Mar 2017 20:25:33 +0100, Vasile Dumitrescu wrote: > sudo sensors > => Note that you don't need to be root to run "sensors". > (...) > dell_smm-virtual-0 > Adapter: Virtual device > Processor Fan: 2490 RPM > Video Fan: 2493 RPM > CPU:+48.0°C > Ambient:+48.0°C > Ambient:+44.0°C > Other: +40.0°C > > sudo pwmconfig > => fans definitely stop and restart as the script indicates they should > > Conclusion: works for me (TM) - without force or any special options > > Thanks Jean, that was easy. You're welcome, glad I could help :-) > Looking forward to see it in some future kernel. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Question about hwmon_attr_show_string
Hi Guenter,
On Mon, 6 Mar 2017 15:47:55 -0800, Guenter Roeck wrote:
> On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter Hüwe wrote:
> > Hi Guenter,
> >
> > I was wondering whether there was a particular reason why
> > hwmon_attr_show_string passes only an "empty" pointer(pointer) to the ops-
> > >read_string function rather than the buffer itself?
> >
> > Wouldn't this mean that in ops->read_string I'd have to reserve some space
> > for
> > the value on the heap (and taking care to free it somewhere, since
> > returning
> > an address on the stack is bad idea), instead of calling sprintf(buf,
> > "%s\n",
> > s) directly?
> >
> > With the current implementation I have to sprintf it into my local buffer
> > and
> > you sprintf it again into the final buffer.
>
> The idea was that the called code would return a pointer to a constant string,
> ie one that isn't changing from call to call.
In that case, what about the following change?
Subject: hwmon: Constify str parameter of hwmon_ops->read_string
The read_string callback is supposed to retrieve a pointer to a
constant string.
Signed-off-by: Jean Delvare <jdelv...@suse.de>
---
drivers/hwmon/hwmon.c |2 +-
include/linux/hwmon.h |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--- linux-4.10.orig/drivers/hwmon/hwmon.c 2017-02-19 23:34:00.0
+0100
+++ linux-4.10/drivers/hwmon/hwmon.c2017-03-07 08:22:27.784527968 +0100
@@ -186,7 +186,7 @@ static ssize_t hwmon_attr_show_string(st
char *buf)
{
struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
- char *s;
+ const char *s;
int ret;
ret = hattr->ops->read_string(dev, hattr->type, hattr->attr,
--- linux-4.10.orig/include/linux/hwmon.h 2017-02-19 23:34:00.0
+0100
+++ linux-4.10/include/linux/hwmon.h2017-03-07 08:21:28.247998585 +0100
@@ -336,7 +336,7 @@ struct hwmon_ops {
int (*read)(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val);
int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
- u32 attr, int channel, char **str);
+ u32 attr, int channel, const char **str);
int (*write)(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long val);
};
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (dell-smm) Add Dell XPS 15 9560 into DMI list
Hi Vasile, On Sat, 4 Mar 2017 22:58:26 +0100, Vasile Dumitrescu wrote: > ok, but how? I am on Debian, and I never in my life compiled a kernel... > > What would I need to do? If your Debian system has a kernel >= 3.13 and you are somewhat familiar with compiling from sources, you could give a try to this standalone driver: http://jdelvare.nerim.net/devel/lm-sensors/drivers/dell-smm-hwmon/ Instructions are at: http://jdelvare.nerim.net/devel/lm-sensors/drivers/INSTALL -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: Relax name attribute validation for new APIs
Hi Guenter, Dmitry, On Fri, 27 Jan 2017 19:49:49 -0800, Guenter Roeck wrote: > While invalid name attributes are really not desirable and do mess up > libsensors, enforcing valid names has the detrimental effect of driving > users away from using the new hardware monitoring API, especially those > registering name attributes violating the ABI restrictions. Another > undesirable side effect is that this violation and the resulting error > may only be discovered some time after a conversion to the new API, > which in turn may trigger a revert of that conversion. > > To solve the problem, relax validation and only issue a warning instead > of returning an error if a name attribute violating the ABI is provided. > This lets callers continue to provide invalid name attributes while > notifying them about it. > > Many thanks are due to Dmitry Torokhov for the idea. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > drivers/hwmon/hwmon.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > index a8195fff..53c54a81f7ad 100644 > --- a/drivers/hwmon/hwmon.c > +++ b/drivers/hwmon/hwmon.c > @@ -544,9 +544,10 @@ __hwmon_device_register(struct device *dev, const char > *name, void *drvdata, > struct device *hdev; > int i, j, err, id; > > - /* Do not accept invalid characters in hwmon name attribute */ > + /* Complain about invalid characters in hwmon name attribute */ > if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) > - return ERR_PTR(-EINVAL); > + dev_warn(dev, "hwmon: '%s' is not a valid name attribute\n", > + name); May I suggest adding ", please fix"? > > id = ida_simple_get(_ida, 0, 0, GFP_KERNEL); > if (id < 0) Reviewed-by: Jean Delvare <jdelv...@suse.de> Do I understand correctly that in the long run we will make it a fatal error again? -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: Update documentation to clarify rules for the 'name' attribute
On Wed, 25 Jan 2017 01:28:02 -0800, Guenter Roeck wrote: > Clarify that the name attribute must report a valid name, and the rules > for valid names. Also clarify that the name parameter must be provided > for all supported API functions. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > v2: '.' is an acceptable character in the 'name' attribute. > > Documentation/hwmon/hwmon-kernel-api.txt | 4 > Documentation/hwmon/sysfs-interface | 5 +++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/Documentation/hwmon/hwmon-kernel-api.txt > b/Documentation/hwmon/hwmon-kernel-api.txt > index 2505ae67e2b6..53a806696c64 100644 > --- a/Documentation/hwmon/hwmon-kernel-api.txt > +++ b/Documentation/hwmon/hwmon-kernel-api.txt > @@ -89,6 +89,10 @@ the call to devm_hwmon_device_register_with_groups or > hwmon_device_register_with_info and if the automatic (device managed) > removal would be too late. > > +All supported hwmon device registration functions only accept valid device > +names. Device names including invalid characters (whitespace, '*', or '-') > +will be rejected. The 'name' parameter is mandatory. > + > Using devm_hwmon_device_register_with_info() > > > diff --git a/Documentation/hwmon/sysfs-interface > b/Documentation/hwmon/sysfs-interface > index 2cc95ad46604..fc337c317c67 100644 > --- a/Documentation/hwmon/sysfs-interface > +++ b/Documentation/hwmon/sysfs-interface > @@ -86,8 +86,9 @@ given driver if the chip has the feature. > > name The chip name. > This should be a short, lowercase string, not containing > - spaces nor dashes, representing the chip name. This is > - the only mandatory attribute. > + whitespace, dashes, or the wildcard character '*'. > + This attribute represents the chip name. It is the only > + mandatory attribute. > I2C devices get this attribute created automatically. > RO > Reviewed-by: Jean Delvare <jdelv...@suse.de> Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] hwmon: Make name attribute mandatory for new APIs
On Wed, 25 Jan 2017 01:34:40 -0800, Guenter Roeck wrote:
> It does not make sense to use one of the the new APIs when not
> even providing a name attribute. Make it mandatory.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> v3: hwmon_device_register() needs to call __hwmon_device_register() directly.
> v2: One should use ERR_PTR where appropriate
>
> drivers/hwmon/hwmon.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 0c5660ccdbf4..a8195fff 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -651,6 +651,9 @@ hwmon_device_register_with_groups(struct device *dev,
> const char *name,
> void *drvdata,
> const struct attribute_group **groups)
> {
> + if (!name)
> + return ERR_PTR(-EINVAL);
> +
> return __hwmon_device_register(dev, name, drvdata, NULL, groups);
> }
> EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
> @@ -674,6 +677,9 @@ hwmon_device_register_with_info(struct device *dev, const
> char *name,
> const struct hwmon_chip_info *chip,
> const struct attribute_group **extra_groups)
> {
> + if (!name)
> + return ERR_PTR(-EINVAL);
> +
> if (chip && (!chip->ops || !chip->ops->is_visible || !chip->info))
> return ERR_PTR(-EINVAL);
>
> @@ -695,7 +701,7 @@ struct device *hwmon_device_register(struct device *dev)
> dev_warn(dev,
>"hwmon_device_register() is deprecated. Please convert the
> driver to use hwmon_device_register_with_info().\n");
>
> - return hwmon_device_register_with_groups(dev, NULL, NULL, NULL);
> + return __hwmon_device_register(dev, NULL, NULL, NULL, NULL);
> }
> EXPORT_SYMBOL_GPL(hwmon_device_register);
>
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: Update documentation to clarify rules for the 'name' attribute
Hi Guenter, On Tue, 24 Jan 2017 20:26:34 -0800, Guenter Roeck wrote: > Clarify that the name attribute must report a valid name, and the rules > for valid names. Also clarify that the name parameter must be provided > for all supported API functions. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > Documentation/hwmon/hwmon-kernel-api.txt | 4 > Documentation/hwmon/sysfs-interface | 5 +++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/Documentation/hwmon/hwmon-kernel-api.txt > b/Documentation/hwmon/hwmon-kernel-api.txt > index 2505ae67e2b6..41bc15b34737 100644 > --- a/Documentation/hwmon/hwmon-kernel-api.txt > +++ b/Documentation/hwmon/hwmon-kernel-api.txt > @@ -89,6 +89,10 @@ the call to devm_hwmon_device_register_with_groups or > hwmon_device_register_with_info and if the automatic (device managed) > removal would be too late. > > +All supported hwmon device registration functions only accept valid device > +names. Device names including invalid characters (whitespace, '.'. '*', Typo: should be , after '.' instead of . . I am confused as to why '.' is forbidden. The code in __hwmon_device_register() does not reject that character, and I can't see how this would cause any problem to libsensors. > +or '-') will be rejected. The 'name' parameter is mandatory. > + > Using devm_hwmon_device_register_with_info() > > > diff --git a/Documentation/hwmon/sysfs-interface > b/Documentation/hwmon/sysfs-interface > index 2cc95ad46604..89f0bcbcaa18 100644 > --- a/Documentation/hwmon/sysfs-interface > +++ b/Documentation/hwmon/sysfs-interface > @@ -86,8 +86,9 @@ given driver if the chip has the feature. > > name The chip name. > This should be a short, lowercase string, not containing > - spaces nor dashes, representing the chip name. This is > - the only mandatory attribute. > + whitespace, dashes, or the wildcard characters '.' and '*'. > + This attribute represents the chip name. It is the only > + mandatory attribute. > I2C devices get this attribute created automatically. > RO > -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: Make name attribute mandatory for new APIs
Hi Guenter,
On Tue, 24 Jan 2017 08:57:14 -0800, Guenter Roeck wrote:
> It does not make sense to use one of the the new APIs when not
> even providing a name attribute. Make it mandatory.
Fully agreed, but...
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> v2: One should use ERR_PTR where appropriate
>
> drivers/hwmon/hwmon.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 0c5660ccdbf4..a408b10d9a86 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -651,6 +651,9 @@ hwmon_device_register_with_groups(struct device *dev,
> const char *name,
> void *drvdata,
> const struct attribute_group **groups)
> {
> + if (!name)
> + return ERR_PTR(-EINVAL);
> +
> return __hwmon_device_register(dev, name, drvdata, NULL, groups);
> }
> EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
> @@ -674,6 +677,9 @@ hwmon_device_register_with_info(struct device *dev, const
> char *name,
> const struct hwmon_chip_info *chip,
> const struct attribute_group **extra_groups)
> {
> + if (!name)
> + return ERR_PTR(-EINVAL);
> +
> if (chip && (!chip->ops || !chip->ops->is_visible || !chip->info))
> return ERR_PTR(-EINVAL);
>
... this breaks hwmon_device_register(), which calls:
return hwmon_device_register_with_groups(dev, NULL, NULL, NULL);
name
So you would have to change hwmon_device_register() to call
__hwmon_device_register() directly first.
Thanks,
--
Jean Delvare
SUSE L3 Support
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] hwmon: (lm93) Fix overflows seen when writing into limit attributes
Hi Guetner, I kept the more tasty one for last ;-) On Sun, 4 Dec 2016 20:55:26 -0800, Guenter Roeck wrote: > Module test shows a large number of overflows, caused by missing clamps > as well as various conversions between variable types. > > Also fix temperature calculations for hysteresis and offset registers. > For those, temperature calculations were a mix of millisecond and second > based, causing reported and accepted hysteresis and offset temperatures > to be widely off target. > > This also changes the offset and base temperature attributes to be > officially reported and set in milli-degrees C. This was already the case > for the base temperature attribute, even though it was documented to be > reported and set in degrees C. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > Documentation/hwmon/lm93 | 26 - > drivers/hwmon/lm93.c | 49 > +--- > 2 files changed, 39 insertions(+), 36 deletions(-) > > diff --git a/Documentation/hwmon/lm93 b/Documentation/hwmon/lm93 > index f3b2ad2ceb01..7bda7f0291e4 100644 > --- a/Documentation/hwmon/lm93 > +++ b/Documentation/hwmon/lm93 > @@ -174,25 +174,25 @@ a "0" disables it. The h/w default is 0x0f (all > temperatures bound). > > The function y = f(x) takes a source temperature x to a PWM output y. This > function of the LM93 is derived from a base temperature and a table of 12 > -temperature offsets. The base temperature is expressed in degrees C in the > -sysfs files temp_auto_base. The offsets are expressed in cumulative > -degrees C, with the value of offset for temperature value being > +temperature offsets. The base temperature is expressed in milli-degrees C in > +the sysfs files temp_auto_base. The offsets are expressed in cumulative > +milli-degrees C, with the value of offset for temperature value being > contained in the file temp_auto_offset. E.g. if the base temperature > is 40C: > > offset #temp_auto_offset range pwm >1 0 -25.00% >2 0 -28.57% > - 3 1 40C - 41C32.14% > - 4 1 41C - 42C35.71% > - 5 2 42C - 44C39.29% > - 6 2 44C - 46C42.86% > - 7 2 48C - 50C46.43% > - 8 2 50C - 52C50.00% > - 9 2 52C - 54C53.57% > - 10 2 54C - 56C57.14% > - 11 2 56C - 58C71.43% > - 12 2 58C - 60C85.71% > + 3 500 40C - 41C32.14% > + 4 500 41C - 42C35.71% > + 5 100042C - 44C39.29% > + 6 100044C - 46C42.86% > + 7 100048C - 50C46.43% > + 8 100050C - 52C50.00% > + 9 100052C - 54C53.57% > + 10 100054C - 56C57.14% > + 11 100056C - 58C71.43% > + 12 100058C - 60C85.71% > > 60C 100.00% I'm a bit confused, I would have expected 1000 and 2000 for temp_auto_offset, not 500 and 1000? Otherwise I can't see how you get the announced ranges. > > Valid offsets are in the range 0C <= x <= 7.5C in 0.5C increments. > diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c > index 90bb04858117..7b3152368e3b 100644 > --- a/drivers/hwmon/lm93.c > +++ b/drivers/hwmon/lm93.c > (...) Code changes all look good to me, good job. Hairy code :-/ Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/17] hwmon: (gl518sm) Fix overflows seen when writing into limit attributes
Hi Guenter,
On Sun, 4 Dec 2016 20:55:39 -0800, Guenter Roeck wrote:
> Writes into limit attributes can overflow due to additions and
> multiplications with unchecked parameters.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/gl518sm.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/gl518sm.c b/drivers/hwmon/gl518sm.c
> index 0212c8317bca..0a11a550c2a2 100644
> --- a/drivers/hwmon/gl518sm.c
> +++ b/drivers/hwmon/gl518sm.c
> @@ -86,9 +86,8 @@ enum chips { gl518sm_r00, gl518sm_r80 };
> #define BOOL_FROM_REG(val) ((val) ? 0 : 1)
> #define BOOL_TO_REG(val) ((val) ? 0 : 1)
>
> -#define TEMP_TO_REG(val) clamp_val(val) < 0 ? \
> - (val) - 500 : \
> - (val) + 500) / 1000) + 119), 0, 255)
> +#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -119000, 136000),
> \
> + 1000) + 119)
> #define TEMP_FROM_REG(val) (((val) - 119) * 1000)
>
> static inline u8 FAN_TO_REG(long rpm, int div)
> @@ -101,10 +100,11 @@ static inline u8 FAN_TO_REG(long rpm, int div)
> }
> #define FAN_FROM_REG(val, div) ((val) == 0 ? 0 : (48 / ((val) *
> (div
>
> -#define IN_TO_REG(val) clamp_valval) + 9) / 19), 0, 255)
> +#define IN_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19),
> 19)
> #define IN_FROM_REG(val) ((val) * 19)
>
> -#define VDD_TO_REG(val) clamp_valval) * 4 + 47) / 95), 0,
> 255)
> +#define VDD_TO_REG(val) \
> + DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
> #define VDD_FROM_REG(val)(((val) * 95 + 2) / 4)
>
> #define DIV_FROM_REG(val)(1 << (val))
Code looks good. Alignment is a bit clumsy though. Also it feels
strange now that you are using DIV_ROUND_CLOSEST for VDD_TO_REG but not
VDD_FROM_REG.
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: (nct7802) Fix overflows seen when writing into limit attributes
On Mon, 12 Dec 2016 06:14:14 -0800, Guenter Roeck wrote: > Hi Jean, > > On 12/12/2016 02:03 AM, Jean Delvare wrote: > > On Fri, 9 Dec 2016 12:41:02 -0800, Guenter Roeck wrote: > >> Fix overflows seen when writing voltage and temperature limit attributes. > >> > >> The value passed to DIV_ROUND_CLOSEST() needs to be clamped, and the > >> value parameter passed to nct7802_write_fan_min() is an unsigned long. > >> > >> Also, writing values larger than 270 into a limit attribute results > >> in writing 0 into the chip's limit registers. > > > > You are only talking about _fan_ limits, right? > > > Yes. I'll clarify. > > >> The exact behavior when > >> writing this value is unspecified. For consistency, report a limit of > >> 135 if the chip register reads 0. This may be wrong, and the chip > >> behavior should be verified with the actual chip, but it is better than > >> reporting a value of 0 (which, if written, results in writing a value > >> of 0x1fff into the chip register). > > > > This fix is good by doesn't seem to be related with the overflows? > > > Writing a limit larger than 270 results in writing 0 into the register, > which is reported back as 0. 269 -> 1 -> 135, 270 -> 0 -> 0. > I consider that an overflow situation. OK, I understand, you are right. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/17] hwmon: (lm85) Fix overflows seen when writing voltage limit attributes
On Sun, 4 Dec 2016 20:55:34 -0800, Guenter Roeck wrote:
> Writes into voltage limit attributes can overflow due to an unbound
> multiplication.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/lm85.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c
> index 6ff773fcaefb..29c8136ce9c5 100644
> --- a/drivers/hwmon/lm85.c
> +++ b/drivers/hwmon/lm85.c
> @@ -136,7 +136,8 @@ static const int lm85_scaling[] = { /* .001 Volts */
> #define SCALE(val, from, to) (((val) * (to) + ((from) / 2)) / (from))
>
> #define INS_TO_REG(n, val) \
> - clamp_val(SCALE(val, lm85_scaling[n], 192), 0, 255)
> + SCALE(clamp_val(val, 0, 255 * lm85_scaling[n] / 192), \
> + lm85_scaling[n], 192)
>
> #define INSEXT_FROM_REG(n, val, ext) \
> SCALE(((val) << 4) + (ext), 192 << 4, lm85_scaling[n])
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/17] hwmon: (nct7802) Fix overflows seen when writing into limit attributes
On Fri, 9 Dec 2016 06:22:43 -0800, Guenter Roeck wrote: > On 12/09/2016 01:49 AM, Jean Delvare wrote: > > Looking at function nct7802_write_fan_min() I think an overflow can > > happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val(). > > Any reason why you did not fix that one? > > > Not really. The call is > DIV_ROUND_CLOSEST(135U, limit); > and thus won't overflow. Limit is originally parsed by kstrtoul into an unsigned long, however the nct7802_write_fan_min function parameter is an unsigned int, so it is implicitly cast to an unsigned int. On a 32-bit system, that may not fit? -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/17] hwmon: (lm87) Fix overflow seen when writing voltage limit attributes
On Sun, 4 Dec 2016 20:55:33 -0800, Guenter Roeck wrote:
> Writes into voltage limit attributes can overflow due to an unbound
> multiplication.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/lm87.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
> index a5e295826aea..24f971b75e46 100644
> --- a/drivers/hwmon/lm87.c
> +++ b/drivers/hwmon/lm87.c
> @@ -121,7 +121,7 @@ static u8 LM87_REG_TEMP_LOW[3] = { 0x3A, 0x38, 0x2C };
>
> #define IN_FROM_REG(reg, scale) (((reg) * (scale) + 96) / 192)
> #define IN_TO_REG(val, scale)((val) <= 0 ? 0 : \
> - (val) * 192 >= (scale) * 255 ? 255 : \
> + (val) >= (scale) * 255 / 192 ? 255 : \
>((val) * 192 + (scale) / 2) / (scale))
>
> #define TEMP_FROM_REG(reg) ((reg) * 1000)
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/17] hwmon: (nct7802) Fix overflows seen when writing into limit attributes
Hi Guenter,
On Sun, 4 Dec 2016 20:55:32 -0800, Guenter Roeck wrote:
> Fix overflows seen when writing voltage and temperature limit attributes.
>
> The value passed to DIV_ROUND_CLOSEST() needs to be clamped.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
I think you can also add:
Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y")
> ---
> drivers/hwmon/nct7802.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
> index 3ce33d244cc0..6fe71cfe0320 100644
> --- a/drivers/hwmon/nct7802.c
> +++ b/drivers/hwmon/nct7802.c
> @@ -326,8 +326,9 @@ static int nct7802_write_voltage(struct nct7802_data
> *data, int nr, int index,
> int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr];
> int err;
>
> - voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]);
> - voltage = clamp_val(voltage, 0, 0x3ff);
> + voltage = DIV_ROUND_CLOSEST(clamp_val(voltage, 0,
> + 0x3ff * nct7802_vmul[nr]),
> + nct7802_vmul[nr]);
As for previous patches, I think separate lines make the code more
readable.
>
> mutex_lock(>access_lock);
> err = regmap_write(data->regmap,
> @@ -402,7 +403,7 @@ static ssize_t store_temp(struct device *dev, struct
> device_attribute *attr,
> if (err < 0)
> return err;
>
> - val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
> + val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
You change the low limit from -128 to -127 without justification.
>
> err = regmap_write(data->regmap, nr, val & 0xff);
> return err ? : count;
Looking at function nct7802_write_fan_min() I think an overflow can
happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val().
Any reason why you did not fix that one?
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] hwmon: (adm1026) Fix overflows seen when writing into limit attributes
Hi Guenter,
On Thu, 8 Dec 2016 07:34:35 -0800, Guenter Roeck wrote:
> On 12/08/2016 06:33 AM, Jean Delvare wrote:
> > On Sun, 4 Dec 2016 20:55:29 -0800, Guenter Roeck wrote:
> >> @@ -215,11 +216,11 @@ static int adm1026_scaling[] = { /* .001 Volts */
> >> #define DIV_TO_REG(val) ((val) >= 8 ? 3 : (val) >= 4 ? 2 : (val) >= 2 ? 1
> >> : 0)
> >>
> >> /* Temperature is reported in 1 degC increments */
> >> -#define TEMP_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \
> >> - / 1000, -127, 127))
> >> +#define TEMP_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000,
> >> 127000), \
> >> + 1000)
> >> #define TEMP_FROM_REG(val) ((val) * 1000)
> >> -#define OFFSET_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \
> >> -/ 1000, -127, 127))
> >> +#define OFFSET_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000,
> >> 127000), \
> >> + 1000)
> >
> > Sorry for nitpicking but the original code had -127 °C as the negative
> > limit. You are changing it to -128 °C without a justification. If it
> > matters, it should be at least documented in the commit message. If
> > not, it should be left as it was.
>
> I had seen -128 as value reported by the chip with one of my register dumps,
> which messes up my module tests because it tries to rewrite the value it read
> into the attribute, and that fails. Also, the datasheet lists -128 degrees C
> as a valid register value.
OK, I understand.
> This is one of those philosophical questions, for which I don't have a really
> good answer. Should we clamp to the register limits or to the chip
> specification ?
> I tend to clamp to register limits, because I think that it should always be
> possible
> to write back what was read, but we are highly inconsistent in the various
> drivers.
> Any opinion ?
I have noticed that inconsistency too. Given that we do not have
attributes to expose the limits to user-space, I consider it a nice
feature to clamp the requested limits to what the chip is actually
specified for. But I don't have a strong opinion on it either, and am
not going to spend time "fixing" all drivers not doing it. And you have
a point with being able to write back what was read, especially if the
power-on value isn't withing the specified limits.
> >> (...)
> >> @@ -593,7 +595,10 @@ static ssize_t set_in16_min(struct device *dev,
> >> struct device_attribute *attr,
> >>return err;
> >>
> >>mutex_lock(>update_lock);
> >> - data->in_min[16] = INS_TO_REG(16, val + NEG12_OFFSET);
> >> + data->in_min[16] = INS_TO_REG(16,
> >> +clamp_val(val, INT_MIN,
> >> + INT_MAX - NEG12_OFFSET) +
> >> +NEG12_OFFSET);
> >>adm1026_write_value(client, ADM1026_REG_IN_MIN[16], data->in_min[16]);
> >>mutex_unlock(>update_lock);
> >>return count;
> >> @@ -618,7 +623,10 @@ static ssize_t set_in16_max(struct device *dev,
> >> struct device_attribute *attr,
> >>return err;
> >>
> >>mutex_lock(>update_lock);
> >> - data->in_max[16] = INS_TO_REG(16, val+NEG12_OFFSET);
> >> + data->in_max[16] = INS_TO_REG(16,
> >> +clamp_val(val, INT_MIN,
> >> + INT_MAX - NEG12_OFFSET) +
> >> +NEG12_OFFSET);
> >>adm1026_write_value(client, ADM1026_REG_IN_MAX[16], data->in_max[16]);
> >>mutex_unlock(>update_lock);
> >>return count;
> >
> > On these code paths, you end up calling clamp_val() twice. This could
> > certainly be avoided, but I'm too lazy to do the math ;-)
> >
> I know. Problem here is that there are two overflows, one in the calling code
> (since it does arithmetic on the input value), and another one in
> INS_TO_REG().
> When I wrote this, I didn't have a good idea how to avoid the first overflow
> without a second clamp.
>
> One possibility would be to define INS_TO_REG_NOCLAMP(). Would that be worth
> it ?
No, don't bother. God only knows if there's any user left for this
driver ;-)
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: (adt7462) Fix overflows seen when writing into limit attributes
On Thu, 8 Dec 2016 11:26:49 -0800, Guenter Roeck wrote: > Fix overflows seen when writing large values into temperature limit, > voltage limit, and pwm hysteresis attributes. > > The input parameter to DIV_ROUND_CLOSEST() needs to be clamped to avoid > such overflows. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > v2: Simplified clamping > Separate clamp_val() and DIV_ROUND_CLOSEST() into two lines. > > drivers/hwmon/adt7462.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > (...) Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: (adm1026) Fix overflows seen when writing into limit attributes
On Thu, 8 Dec 2016 11:29:29 -0800, Guenter Roeck wrote: > Fix overflows seen when writing large values into voltage limit, > temperature limit, temperature offset, and DAC attributes. > > Overflows are seen due to unbound multiplications and additions. > > While at it, change the low temperature limit to -128 degrees C, > since this is the minimum temperature accepted by the chip. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > v2: Provide reason for changing lower temperature range. > > drivers/hwmon/adm1026.c | 26 +- > 1 file changed, 17 insertions(+), 9 deletions(-) > (...) Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/17] hwmon: (adt7462) Fix overflows seen when writing into limit attributes
On Sun, 4 Dec 2016 20:55:30 -0800, Guenter Roeck wrote: > Fix overflows seen when writing large values into temperature limit, > voltage limit, and pwm hysteresis attributes. > > The input parameter to DIV_ROUND_CLOSEST() needs to be clamped to avoid > such overflows. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > drivers/hwmon/adt7462.c | 14 ++ > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/hwmon/adt7462.c b/drivers/hwmon/adt7462.c > index 5929e126da63..0e9c8d3e8f5c 100644 > --- a/drivers/hwmon/adt7462.c > +++ b/drivers/hwmon/adt7462.c > @@ -810,8 +810,7 @@ static ssize_t set_temp_min(struct device *dev, > if (kstrtol(buf, 10, ) || !temp_enabled(data, attr->index)) > return -EINVAL; > > - temp = DIV_ROUND_CLOSEST(temp, 1000) + 64; > - temp = clamp_val(temp, 0, 255); > + temp = DIV_ROUND_CLOSEST(clamp_val(temp, -64000, 191000), 1000) + 64; > > mutex_lock(>lock); > data->temp_min[attr->index] = temp; > @@ -848,8 +847,7 @@ static ssize_t set_temp_max(struct device *dev, > if (kstrtol(buf, 10, ) || !temp_enabled(data, attr->index)) > return -EINVAL; > > - temp = DIV_ROUND_CLOSEST(temp, 1000) + 64; > - temp = clamp_val(temp, 0, 255); > + temp = DIV_ROUND_CLOSEST(clamp_val(temp, -64000, 191000), 1000) + 64; > > mutex_lock(>lock); > data->temp_max[attr->index] = temp; The original code had the division and the clamping on separate lines, and I believe it makes the code more readable. It would also make the patch smaller and more readable as well. > @@ -912,6 +910,7 @@ static ssize_t set_volt_max(struct device *dev, > if (kstrtol(buf, 10, ) || !x) > return -EINVAL; > > + temp = clamp_val(temp, 0, INT_MAX / 1000 - x); > temp *= 1000; /* convert mV to uV */ > temp = DIV_ROUND_CLOSEST(temp, x); > temp = clamp_val(temp, 0, 255); > @@ -954,6 +953,7 @@ static ssize_t set_volt_min(struct device *dev, > if (kstrtol(buf, 10, ) || !x) > return -EINVAL; > > + temp = clamp_val(temp, 0, INT_MAX / 1000 - x); > temp *= 1000; /* convert mV to uV */ > temp = DIV_ROUND_CLOSEST(temp, x); > temp = clamp_val(temp, 0, 255); Any chance to get the new clamp_val()s such that the second ones are no longer needed? > @@ -1220,8 +1220,7 @@ static ssize_t set_pwm_hyst(struct device *dev, > if (kstrtol(buf, 10, )) > return -EINVAL; > > - temp = DIV_ROUND_CLOSEST(temp, 1000); > - temp = clamp_val(temp, 0, 15); > + temp = DIV_ROUND_CLOSEST(clamp_val(temp, 0, 15000), 1000); > > /* package things up */ > temp &= ADT7462_PWM_HYST_MASK; > @@ -1306,8 +1305,7 @@ static ssize_t set_pwm_tmin(struct device *dev, > if (kstrtol(buf, 10, )) > return -EINVAL; > > - temp = DIV_ROUND_CLOSEST(temp, 1000) + 64; > - temp = clamp_val(temp, 0, 255); > + temp = DIV_ROUND_CLOSEST(clamp_val(temp, -64000, 191000), 1000) + 64; > > mutex_lock(>lock); > data->pwm_tmin[attr->index] = temp; Same comment as first 2. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/17] hwmon: (adm1025) Fix overflows seen when writing voltage limits
On Sun, 4 Dec 2016 20:55:28 -0800, Guenter Roeck wrote:
> Writes into voltage limit attributes can overflow due to an unbound
> multiplication.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/adm1025.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/adm1025.c b/drivers/hwmon/adm1025.c
> index d6c767ace916..1abb4609b412 100644
> --- a/drivers/hwmon/adm1025.c
> +++ b/drivers/hwmon/adm1025.c
> @@ -93,7 +93,7 @@ static const int in_scale[6] = { 2500, 2250, 3300, 5000,
> 12000, 3300 };
>
> #define IN_FROM_REG(reg, scale) (((reg) * (scale) + 96) / 192)
> #define IN_TO_REG(val, scale)((val) <= 0 ? 0 : \
> - (val) * 192 >= (scale) * 255 ? 255 : \
> + (val) >= (scale) * 255 / 192 ? 255 : \
>((val) * 192 + (scale) / 2) / (scale))
>
> #define TEMP_FROM_REG(reg) ((reg) * 1000)
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] hwmon: (smsc47m192) Fix overflows seen when writing into limit attributes
On Sun, 4 Dec 2016 20:55:27 -0800, Guenter Roeck wrote:
> Module test reports overflows when writing into temperature and voltage
> limit attributes
>
> temp1_min: Suspected overflow: [127000 vs. 0]
> temp1_max: Suspected overflow: [127000 vs. 0]
> temp1_offset: Suspected overflow: [127000 vs. 0]
> temp2_min: Suspected overflow: [127000 vs. 0]
> temp2_max: Suspected overflow: [127000 vs. 0]
> temp2_offset: Suspected overflow: [127000 vs. 0]
> temp3_min: Suspected overflow: [127000 vs. 0]
> temp3_max: Suspected overflow: [127000 vs. 0]
> temp3_offset: Suspected overflow: [127000 vs. 0]
> in0_min: Suspected overflow: [3320 vs. 0]
> in0_max: Suspected overflow: [3320 vs. 0]
> in4_min: Suspected overflow: [15938 vs. 0]
> in4_max: Suspected overflow: [15938 vs. 0]
> in6_min: Suspected overflow: [1992 vs. 0]
> in6_max: Suspected overflow: [1992 vs. 0]
> in7_min: Suspected overflow: [2391 vs. 0]
> in7_max: Suspected overflow: [2391 vs. 0]
>
> The problem is caused by conversions from unsigned long to long and
> from long to int.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/smsc47m192.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/smsc47m192.c b/drivers/hwmon/smsc47m192.c
> index 6ac7cda72d4c..202e167d6a59 100644
> --- a/drivers/hwmon/smsc47m192.c
> +++ b/drivers/hwmon/smsc47m192.c
> @@ -77,6 +77,7 @@ static inline unsigned int IN_FROM_REG(u8 reg, int n)
>
> static inline u8 IN_TO_REG(unsigned long val, int n)
> {
> + val = clamp_val(val, 0, 65535);
> return clamp_val(SCALE(val, 192, nom_mv[n]), 0, 255);
> }
Same as for adm9240, I would suggest to remove the second clamp_val():
static inline u8 IN_TO_REG(unsigned long val, int n)
{
val = clamp_val(val, 0, nom_mv[n] * 255 / 192);
return SCALE(val, 192, nom_mv[n]);
}
>
> @@ -84,7 +85,7 @@ static inline u8 IN_TO_REG(unsigned long val, int n)
> * TEMP: 0.001 degC units (-128C to +127C)
> * REG: 1C/bit, two's complement
> */
> -static inline s8 TEMP_TO_REG(int val)
> +static inline s8 TEMP_TO_REG(long val)
> {
> return SCALE(clamp_val(val, -128000, 127000), 1, 1000);
> }
Other than this:
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/17] hwmon: (ds620) Fix overflows seen when writing temperature limits
On Sun, 4 Dec 2016 20:55:25 -0800, Guenter Roeck wrote:
> Module test reports:
>
> temp1_max: Suspected overflow: [16 vs. 0]
> temp1_min: Suspected overflow: [16 vs. 0]
>
> This is seen because the values passed when writing temperature limits
> are unbound.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/ds620.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/ds620.c b/drivers/hwmon/ds620.c
> index edf550fc4eef..0043a4c02b85 100644
> --- a/drivers/hwmon/ds620.c
> +++ b/drivers/hwmon/ds620.c
> @@ -166,7 +166,7 @@ static ssize_t set_temp(struct device *dev, struct
> device_attribute *da,
> if (res)
> return res;
>
> - val = (val * 10 / 625) * 8;
> + val = (clamp_val(val, -128000, 128000) * 10 / 625) * 8;
>
> mutex_lock(>update_lock);
> data->temp[attr->index] = val;
Reviewed-by: Jean Delvare <jdelv...@suse.de>
Fixes: 6099469805c2 ("hwmon: Support for Dallas Semiconductor DS620")
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/17] hwmon: (adm9240) Fix overflows seen when writing into limit attributes
Hi Guenter,
On Sun, 4 Dec 2016 20:55:24 -0800, Guenter Roeck wrote:
> Module test reports:
>
> in0_min: Suspected overflow: [3320 vs. 0]
> in0_max: Suspected overflow: [3320 vs. 0]
> in4_min: Suspected overflow: [15938 vs. 0]
> in4_max: Suspected overflow: [15938 vs. 0]
> temp1_max: Suspected overflow: [127000 vs. 0]
> temp1_max_hyst: Suspected overflow: [127000 vs. 0]
> aout_output: Suspected overflow: [1250 vs. 0]
>
> Code analysis reveals that the overflows are caused by conversions
> from unsigned long to long to int, combined with multiplications on
> passed values.
>
> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/hwmon/adm9240.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index 2fe1828bd10b..347afacedcf5 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -98,12 +98,14 @@ static inline unsigned int IN_FROM_REG(u8 reg, int n)
>
> static inline u8 IN_TO_REG(unsigned long val, int n)
> {
> + val = clamp_val(val, 0, INT_MAX / 192 - 12000);
> return clamp_val(SCALE(val, 192, nom_mv[n]), 0, 255);
> }
I understand the idea of clamping before the conversion to avoid the
overflow. However I would have hoped that clamping the input makes
clamping the output unneeded. Clamping is full of tests, which aren't
cheap as they break the CPU instruction prediction, so we should not
abuse it.
Would the following work?
static inline u8 IN_TO_REG(unsigned long val, int n)
{
val = clamp_val(val, 0, nom_mv[n] * 255 / 192);
return SCALE(val, 192, nom_mv[n]);
}
This should be more compact and faster.
>
> /* temperature range: -40..125, 127 disables temperature alarm */
> static inline s8 TEMP_TO_REG(long val)
> {
> + val = clamp_val(val, INT_MIN + 1000, INT_MAX - 1000);
> return clamp_val(SCALE(val, 1, 1000), -40, 127);
> }
>
> @@ -122,6 +124,7 @@ static inline unsigned int FAN_FROM_REG(u8 reg, u8 div)
> /* analog out 0..1250mV */
> static inline u8 AOUT_TO_REG(unsigned long val)
> {
> + val = clamp_val(val, 0, INT_MAX / 255 - 1250);
> return clamp_val(SCALE(val, 255, 1250), 0, 255);
> }
>
Same comment and same suggested solution for these two functions:
/* temperature range: -40..125, 127 disables temperature alarm */
static inline s8 TEMP_TO_REG(long val)
{
val = clamp_val(val, -4, 127000);
return SCALE(val, 1, 1000);
}
/* analog out 0..1250mV */
static inline u8 AOUT_TO_REG(unsigned long val)
{
val = clamp_val(val, 0, 1250);
return SCALE(val, 255, 1250);
}
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API
Hi Guenter, On Mon, 10 Oct 2016 16:48:22 -0700, Guenter Roeck wrote: > On Fri, Oct 07, 2016 at 02:32:13PM +0200, Jean Delvare wrote: > > On Tue, 4 Oct 2016 12:37:13 -0700, Guenter Roeck wrote: > > > On Tue, Oct 04, 2016 at 10:45:59AM +0200, Jean Delvare wrote: > > > > May I ask if any other subsystem is already doing anything like that? > > > > > > > Depends what you mean with "anything like that". The API is inspired by > > > the iio API and a little by the thermal API. The details are probably > > > unique, though. > > > > I meant the idea of describing the device capabilities and letting the > > core code generate the device attributes (or anything else) based on > > that data. The benefits are obvious, but it has a complexity and > > run-time cost so I am wondering how popular this approach is. > > The goal of this patch series was to provide abstraction between driver > and userspace ABI. I think this is quite common in the kernel. Yes, you are right. I am so used to the (wrong) way hwmon did that I did not realize all other subsystems already provided some form of abstraction for their device drivers. Even i2c does, so I should know. > I am not sure I undersatand the run-time cost concern. The main run-time > cost occurs during device instantiation and only happens once. Other > abstraction APIs in the kernel have a much higher runtime cost, and > instantiation tends to be quite costly as well. I'm not too worried about this one, except for the memory allocations as mentioned previously. I am more concerned about the per-sysfs read overhead. Compared to the old model, you have another indirection call for each access. And for each driver, you end up with a single function to read from all attributes. You will inevitably end up with a huge switch/case statement to figure out what value to return. This smells like linear search? Not sure if the compiler can optimize it. I was also wondering how cache-friendly such a large function can be. But anyway, I didn't measure anything, it's pure speculation on my side. And it's probably irrelevant as your change has many benefits anyway. And I wouldn't even ask myself the question if things had been implemented right in the first place. So just ignore me ^^ > The new API has the addd benefit of reducing driver size, in some cases > significantly. Maybe I am off, but I considered this important, which is > why I mentioned it. Maybe I should not have mentioned it at all, and > instead have focused on abstraction alone. You were very right mentioning it, I mention it every time a patch of mine reduces binary size. That's one of the actual benefits of the change, no reason to silent it. > > You mean at the binary level? > > > > Have you considered the case where several devices exist for the same > > driver? Static attributes are shared between devices, but with the > > generated ones this is no longer the case, right? > > As mentioned above, I considered static sizes to be more important. > Sure, there are situations where multiple instances of a driver are > loaded, but those are not typically memory size limited. But, again, > I guess maybe I should not have mentioned driver size impact at all. I did not make any measurement, and I won't take the time to make them. So I have no idea how the various costs compare to each other. Which in turn means I don't have any point here, and we can go with what you have ;-) Really I was only commenting out loud while reviewing. Doesn't mean much... > > > > (...) > > > > This is adding a 4th and 5th way to register a hwmon device. Starts > > > > being a bit messy... Do you have any plans to get rid of some of the > > > > other functions to make things more consistent and efficient? > > > > > > Would be nice, but then someone would have to spend the time cleaning > > > up the old drivers to replace the old API, and for the most part we would > > > not be able to test the result. Are you sure that is worth the risk ? > > > > What I had in mind was rather along the lines of marking the function > > as __deprecated, adding a run-time notice message when it is called, > > and let whoever uses these driver contribute the fix. Call me an > > optimistic :-) There are 54 drivers still using > > hwmon_device_register(), out of 157. > > For most of those testing was not possible, otherwise I would have converted > them by now. > > I am not sure about deprecated; doesn't that mean a failure to compile with > -Werror ? That would not help much. It causes gcc to generate a warning, so I guess it would indeed break with -Werror. But this option isn't enabled by default, and I doubt anyone can actua
Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API
Hi Guenter,
On Tue, 4 Oct 2016 12:37:13 -0700, Guenter Roeck wrote:
> On Tue, Oct 04, 2016 at 10:45:59AM +0200, Jean Delvare wrote:
> > I see this patch is upstream now, but I had started reviewing it and
> > maybe some of my comments are still relevant.
> >
> As always, I appreciate the feedback. I'll be happy to submit follow-up
> patches to address any concerns.
>
> > It's not meant to be a complete review, which is why I had not sent it
> > yet :-(
> >
> > Also I did not follow the first iterations of this patchset so my
> > apologies if I raise points which have already been discussed.
> >
> > May I ask if any other subsystem is already doing anything like that?
> >
> Depends what you mean with "anything like that". The API is inspired by
> the iio API and a little by the thermal API. The details are probably
> unique, though.
I meant the idea of describing the device capabilities and letting the
core code generate the device attributes (or anything else) based on
that data. The benefits are obvious, but it has a complexity and
run-time cost so I am wondering how popular this approach is.
> > FYI I gave a presentation about the hwmon device registration API
> > evolution last week at Kernel Recipes [1] in Paris and mentioned this
> > proposal.
> >
> > [1] https://kernel-recipes.org/en/2016/
> >
> > On dim., 2016-07-24 at 20:32 -0700, Guenter Roeck wrote:
> > > Up to now, each hwmon driver has to implement its own sysfs attributes.
> > > This requires a lot of template code, and distracts from the driver's core
> > > function to read and write chip registers.
> > >
> > > To be able to reduce driver complexity, move sensor attribute handling
> > > and thermal zone registration into hwmon core. By using the new API,
> > > driver code and data size is typically reduced by 20-70%, depending
> > > on driver complexity and the number of sysfs attributes supported.
> >
> > I looked at the diffstats for the drivers you have already converted and
> > couldn't see any such huge improvement... Some drivers appear to be even
> > larger after conversion?
> >
> The above refers to code and data sizes.
You mean at the binary level?
Have you considered the case where several devices exist for the same
driver? Static attributes are shared between devices, but with the
generated ones this is no longer the case, right?
> > > (...)
> > > +static struct attribute *hwmon_genattr(struct device *dev,
> > > +const void *drvdata,
> > > +enum hwmon_sensor_types type,
> > > +u32 attr,
> > > +int index,
> > > +const char *template,
> > > +const struct hwmon_ops *ops)
> > > +{
> > > + struct hwmon_device_attribute *hattr;
> > > + struct device_attribute *dattr;
> > > + struct attribute *a;
> > > + umode_t mode;
> > > + char *name;
> > > +
> > > + /* The attribute is invisible if there is no template string */
> > > + if (!template)
> > > + return ERR_PTR(-ENOENT);
> > > +
> > > + mode = ops->is_visible(drvdata, type, attr, index);
> >
> > Any reason why you don't simply attach the provided ->is_visible to the
> > attribute group and let the driver core do the work?
> >
> Parameter are all different
> umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
> u32 attr, int channel);
> vs.
> umode_t (*is_visible)(struct kobject *, struct attribute *, int);
But this is the consequence of how you implemented it, not the reason?
Now I seem to understand that delegating the check to the driver core
would make it happen later, which means you would have to instantiate
all attributes, even if some are never going to be used? So this is an
optimization?
But this prevents sharing the attributes between devices, as pointed
out above. Well, I guess you can't have it all. I don't know what is
the more important.
> > > + return ERR_PTR(-ENOENT);
> > > +
> > > + if ((mode & S_IRUGO) && !ops->read)
> > > + return ERR_PTR(-EINVAL);
> > > + if ((mode & S_IWUGO) && !ops->write)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (type == hwmon_chip) {
> >
> > This needs a comment. It's not obvious what "hwmon_chip" is supposed to
> > represent. From what I
Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API
ree(ptr);
> + return hwdev;
> +}
> +EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_info);
> +
> static int devm_hwmon_match(struct device *dev, void *res, void *data)
> {
> struct device **hwdev = res;
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 09354f6c1d63..52e56d71d742 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -14,9 +14,147 @@
> #ifndef _HWMON_H_
> #define _HWMON_H_
>
> +#include
> +
> struct device;
> struct attribute_group;
>
> +enum hwmon_sensor_types {
> + hwmon_chip,
> + hwmon_temp,
> + hwmon_in,
> + hwmon_curr,
> + hwmon_power,
> + hwmon_energy,
> +};
> +
> +enum hwmon_chip_attributes {
> + hwmon_chip_temp_reset_history,
> + hwmon_chip_register_tz,
> + hwmon_chip_update_interval,
> + hwmon_chip_alarms,
> +};
> +
> +#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
> +#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
Ideally this one should go in next patch ("Add voltage attribute support
to new API".)
> +#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
> +#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
> +#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
> +
> +enum hwmon_temp_attributes {
> + hwmon_temp_input = 0,
> + hwmon_temp_type,
> + hwmon_temp_lcrit,
> + hwmon_temp_lcrit_hyst,
> + hwmon_temp_min,
> + hwmon_temp_min_hyst,
> + hwmon_temp_max,
> + hwmon_temp_max_hyst,
> + hwmon_temp_crit,
> + hwmon_temp_crit_hyst,
> + hwmon_temp_emergency,
> + hwmon_temp_emergency_hyst,
> + hwmon_temp_alarm,
> + hwmon_temp_lcrit_alarm,
> + hwmon_temp_min_alarm,
> + hwmon_temp_max_alarm,
> + hwmon_temp_crit_alarm,
> + hwmon_temp_emergency_alarm,
> + hwmon_temp_fault,
> + hwmon_temp_offset,
> + hwmon_temp_label,
> + hwmon_temp_lowest,
> + hwmon_temp_highest,
> + hwmon_temp_reset_history,
> +};
> +
> +#define HWMON_T_INPUTBIT(hwmon_temp_input)
> +#define HWMON_T_TYPE BIT(hwmon_temp_type)
> +#define HWMON_T_LCRITBIT(hwmon_temp_lcrit)
> +#define HWMON_T_LCRIT_HYST BIT(hwmon_temp_lcrit_hyst)
> +#define HWMON_T_MIN BIT(hwmon_temp_min)
> +#define HWMON_T_MIN_HYST BIT(hwmon_temp_min_hyst)
> +#define HWMON_T_MAX BIT(hwmon_temp_max)
> +#define HWMON_T_MAX_HYST BIT(hwmon_temp_max_hyst)
> +#define HWMON_T_CRIT BIT(hwmon_temp_crit)
> +#define HWMON_T_CRIT_HYSTBIT(hwmon_temp_crit_hyst)
> +#define HWMON_T_EMERGENCYBIT(hwmon_temp_emergency)
> +#define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
> +#define HWMON_T_MIN_ALARMBIT(hwmon_temp_min_alarm)
> +#define HWMON_T_MAX_ALARMBIT(hwmon_temp_max_alarm)
> +#define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
> +#define HWMON_T_EMERGENCY_ALARM BIT(hwmon_temp_emergency_alarm)
> +#define HWMON_T_FAULTBIT(hwmon_temp_fault)
> +#define HWMON_T_OFFSET BIT(hwmon_temp_offset)
> +#define HWMON_T_LABELBIT(hwmon_temp_label)
> +#define HWMON_T_LOWEST BIT(hwmon_temp_lowest)
> +#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
> +#define HWMON_T_RESET_HISTORYBIT(hwmon_temp_reset_history)
> +
> +/**
> + * struct hwmon_ops - hwmon device operations
> + * @is_visible: Callback to return attribute visibility. Mandatory.
> + * Parameters are:
> + * @const void *drvdata:
> + * Pointer to driver-private data structure passed
> + * as argument to hwmon_device_register_with_info().
> + * @type: Sensor type
> + * @attr: Sensor attribute
> + * @channel:
> + * Channel number
> + * The function returns the file permissions.
> + * If the return value is 0, no attribute will be created.
> + * @read: Read callback. Optional. If not provided, attributes
> + * will not be readable.
> + * Parameters are:
> + * @dev: Pointer to hardware monitoring device
> + * @type: Sensor type
> + * @attr: Sensor attribute
> + * @channel:
> + * Channel number
> + * @val: Pointer to returned value
> + * The function returns 0 on success or a negative error number.
> + * @write: Write callback. Optional. If not provided, attributes
> + * will not be writable.
> + * Parameters are:
> + * @dev: Pointer to hardware monitoring device
> + * @type: Sensor type
> + * @attr: Sensor attribute
> + * @channel:
> + * Channel number
> + * @val: Value to write
> + * The function returns 0 on success or a negative error number.
> + */
> +struct hwmon_ops {
> + umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
> + u32 attr, int channel);
> + int (*read)(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val);
> + int (*write)(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val);
> +};
> +
> +/**
> + * Channel information
> + * @type:Channel type.
> + * @config: Pointer to NULL-terminated list of channel parameters.
> + * Use for per-channel attributes.
> + */
> +struct hwmon_channel_info {
> + enum hwmon_sensor_types type;
> + const u32 *config;
> +};
> +
> +/**
> + * Chip configuration
> + * @ops: Pointer to hwmon operations.
> + * @info:Null-terminated list of channel information.
> + */
> +struct hwmon_chip_info {
> + const struct hwmon_ops *ops;
> + const struct hwmon_channel_info **info;
> +};
> +
> struct device *hwmon_device_register(struct device *dev);
> struct device *
> hwmon_device_register_with_groups(struct device *dev, const char *name,
> @@ -26,6 +164,16 @@ struct device *
> devm_hwmon_device_register_with_groups(struct device *dev, const char *name,
> void *drvdata,
> const struct attribute_group **groups);
> +struct device *
> +hwmon_device_register_with_info(struct device *dev,
> + const char *name, void *drvdata,
> + const struct hwmon_chip_info *info,
> + const struct attribute_group **groups);
> +struct device *
> +devm_hwmon_device_register_with_info(struct device *dev,
> + const char *name, void *drvdata,
> + const struct hwmon_chip_info *info,
> + const struct attribute_group **groups);
>
> void hwmon_device_unregister(struct device *dev);
> void devm_hwmon_device_unregister(struct device *dev);
This is adding a 4th and 5th way to register a hwmon device. Starts
being a bit messy... Do you have any plans to get rid of some of the
other functions to make things more consistent and efficient?
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/9] hwmon: (core) Order include files alphabetically
On dim., 2016-07-24 at 20:32 -0700, Guenter Roeck wrote: > Ordering include files alphabetically makes it easier to add new ones. > Stop including linux/spinlock.h and linux/kdev_t.h since both are not > needed. > > Reviewed-by: Jonathan Cameron <ji...@kernel.org> > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > v3: No change > v2: Added patch > > drivers/hwmon/hwmon.c | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > (...) Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sensors-detect: Report proper processor information on ppc
The format of /proc/cpuinfo on ppc differs from the x86 format. Add
the missing pieces to the parsing code so that ppc processor
information is reported properly.
---
prog/detect/sensors-detect |8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
--- a/prog/detect/sensors-detect
+++ b/prog/detect/sensors-detect
@@ -2864,7 +2864,7 @@ sub initialize_cpu_list
};
next;
}
- if (m/^(vendor_id|cpu family|model|model name|stepping|cpuid
level)\s*:\s*(.+)$/) {
+ if (m/^(vendor_id|cpu family|model|model name|stepping|cpuid
level|cpu|revision)\s*:\s*(.+)$/) {
my $k = $1;
my $v = $2;
$v =~ s/\s+/ /g;# Merge multiple spaces
@@ -2880,7 +2880,11 @@ sub initialize_cpu_list
sub print_cpu_info
{
my $cpu = $cpu[0];
- print "# Processor: $cpu->{'model name'} ($cpu->{'cpu
family'}/$cpu->{model}/$cpu->{stepping})\n";
+ if (defined $cpu->{'model name'}) {
+ print "# Processor: $cpu->{'model name'} ($cpu->{'cpu
family'}/$cpu->{model}/$cpu->{stepping})\n";
+ } elsif (defined $cpu->{'cpu'}) { # ppc
+ print "# Processor: $cpu->{'cpu'}, revision
$cpu->{'revision'}\n";
+ }
}
# @i2c_adapters is a list of references to hashes, one hash per I2C/SMBus
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: adt7470: Allow faster removal
Hi Chris, On Thu, 1 Sep 2016 21:08:54 +, Chris Packham wrote: > On 09/02/2016 12:12 AM, Guenter Roeck wrote: > > This puts a heavy burden on the system, forcing it to run every ms, just for > > the unlikely case of driver removal. Why is quick removal so important ? > > If it is, we'll have to find a better solution. > > The particular use case we have is a chassis based system with an > adt7470 on a removable fan-tray. The problem is that when the fan tray > is removed it takes auto_update_interval/1000 seconds for the update > thread to stop and the i2c device to be removed. In the intervening time > a new fan-tray could have been installed. Thanks for the clarification. An actual use case makes it easier to think about solutions. > On 09/01/2016 08:18 PM, Jean Delvare wrote: > > > > This change looks terribly costly to me. In order to shorten the > > duration of a rare event (you don't "rmmod adt7470" on a regular basis, > > do you?) > > It's worse than that. We're not doing rmmod, hardware is physically > being removed. I wouldn't call it "worse", it's pretty much the same to me. > > you increase the cost of a kernel thread which runs all the > > time. I'm afraid this msleep_interruptible(1) is going to prevent the > > CPU from entering low power states at all, leading to an increased > > system temperature and power consumption. Have you compared the output > > of "powertop" (specifically the "Idle stats" section) before and after > > your patch? > > > > Is there no way to voluntarily interrupt this long msleep_interruptible? > > If there is I'd like to know. That would be the ideal solution for us. I've never done that before myself so I don't know more than you. stackoverflow has a few promising pointers though: http://stackoverflow.com/questions/26050745/is-there-a-way-inside-the-kernel-of-killing-a-kernel-kthread-just-like-kill-9 http://stackoverflow.com/questions/36344295/wakeup-a-kernel-thread-that-is-in-sleep-using-msleep My own research suggests that maybe calling wake_up_process(data->auto_update) right after kthread_stop() may work. Have you tried that? I only find it suspect that nobody else in the kernel is doing that. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: adt7470: Allow faster removal
Hi Joshua,
On Thu, 1 Sep 2016 17:17:21 +1200, Joshua Scott wrote:
> adt7470_remove will wait for the update thread to complete before
> returning. This has a worst-case time of up to the user-configurable
> auto_update_interval.
>
> Break this delay into smaller slices to allow the thread to exit quickly
> when adt7470_remove is called.
>
> Signed-off-by: Joshua Scott <joshua.sc...@alliedtelesis.co.nz>
> ---
> drivers/hwmon/adt7470.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 7d185a9..ba97392 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -268,14 +268,21 @@ static int adt7470_update_thread(void *p)
> {
> struct i2c_client *client = p;
> struct adt7470_data *data = i2c_get_clientdata(client);
> + unsigned long next_read = jiffies - 1;
>
> while (!kthread_should_stop()) {
> - mutex_lock(>lock);
> - adt7470_read_temperatures(client, data);
> - mutex_unlock(>lock);
> +
> + if (time_after_eq(jiffies, next_read)) {
> + next_read = jiffies + data->auto_update_interval * HZ /
> 1000;
> + mutex_lock(>lock);
> + adt7470_read_temperatures(client, data);
> + mutex_unlock(>lock);
> + }
> +
> + msleep_interruptible(1);
> +
> if (kthread_should_stop())
> break;
> - msleep_interruptible(data->auto_update_interval);
> }
>
> complete_all(>auto_update_stop);
This change looks terribly costly to me. In order to shorten the
duration of a rare event (you don't "rmmod adt7470" on a regular basis,
do you?) you increase the cost of a kernel thread which runs all the
time. I'm afraid this msleep_interruptible(1) is going to prevent the
CPU from entering low power states at all, leading to an increased
system temperature and power consumption. Have you compared the output
of "powertop" (specifically the "Idle stats" section) before and after
your patch?
Is there no way to voluntarily interrupt this long msleep_interruptible?
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v4.7.2: Oops] When loading IT87 HARDWARE MONITORING DRIVER
On Mon, 29 Aug 2016 07:46:36 -0700, Guenter Roeck wrote: > On 08/29/2016 03:11 AM, Jean Delvare wrote: > > On Mon, 29 Aug 2016 12:06:34 +0200, Jean Delvare wrote: > >> I'm trying to find when the bug was introduced. I have a hard time > >> believing it went unnoticed for long. If it fixes your problem I'll > >> send a clean patch. > > > > Broken by: > > > > commit 52929715634ad36782bd7018ab0bf59a6619c393 > > Author: Guenter Roeck <li...@roeck-us.net> > > Date: Sat Mar 30 14:00:08 2013 -0700 > > > > hwmon: (it87) Use is_visible for voltage sensors > > > Yes. Oops, I should know better. Thanks for picking this up so quickly. You're welcome. This is my punishment for not reviewing the patches as you post them... -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v4.7.2: Oops] When loading IT87 HARDWARE MONITORING DRIVER
Hello Alexander,
On Sat, 27 Aug 2016 18:52:42 +0300, Alexander Kapshuk wrote:
> I get an Oops when loading it87.ko on a 4.7.2 kernel. See below for details:
it87_attributes_in lacks its NULL terminator, looks suspicious to me.
Can you try the following quick fix?
---
drivers/hwmon/it87.c |1 +
1 file changed, 1 insertion(+)
--- linux-4.7.orig/drivers/hwmon/it87.c 2016-07-04 08:01:00.0 +0200
+++ linux-4.7/drivers/hwmon/it87.c 2016-08-29 12:04:52.926911625 +0200
@@ -2015,6 +2015,7 @@ static struct attribute *it87_attributes
_dev_attr_in10_input.dev_attr.attr, /* 41 */
_dev_attr_in11_input.dev_attr.attr, /* 41 */
_dev_attr_in12_input.dev_attr.attr, /* 41 */
+ NULL
};
static const struct attribute_group it87_group_in = {
I'm trying to find when the bug was introduced. I have a hard time
believing it went unnoticed for long. If it fixes your problem I'll
send a clean patch.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (lm90) Improve error handling
Hi Guenter, On Tue, 26 Jul 2016 08:19:47 -0700, Guenter Roeck wrote: > Replace devm_add_action() with devm_add_action_or_reset(), > and check its return value. > > Signed-off-by: Guenter Roeck <li...@roeck-us.net> > --- > drivers/hwmon/lm90.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c > index 7b3eedf76c62..fa15f7238a28 100644 > --- a/drivers/hwmon/lm90.c > +++ b/drivers/hwmon/lm90.c > @@ -1551,9 +1551,7 @@ static int lm90_init_client(struct i2c_client *client, > struct lm90_data *data) > if (config != data->config_orig) /* Only write if changed */ > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config); > > - devm_add_action(>dev, lm90_restore_conf, data); > - > - return 0; > + return devm_add_action_or_reset(>dev, lm90_restore_conf, data); > } > > static bool lm90_is_tripped(struct i2c_client *client, u16 *status) > @@ -1640,7 +1638,9 @@ static int lm90_probe(struct i2c_client *client, > return err; > } > > - devm_add_action(dev, lm90_regulator_disable, regulator); > + err = devm_add_action_or_reset(dev, lm90_regulator_disable, regulator); > + if (err) > + return err; > > data = devm_kzalloc(dev, sizeof(struct lm90_data), GFP_KERNEL); > if (!data) > @@ -1696,7 +1696,9 @@ static int lm90_probe(struct i2c_client *client, > err = device_create_file(dev, _attr_pec); > if (err) > return err; > - devm_add_action(dev, lm90_remove_pec, dev); > + err = devm_add_action_or_reset(dev, lm90_remove_pec, dev); > + if (err) > + return err; > } > > hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, I'm not yet familiar with that API but it looks like the right thing to do. Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] hwmon: (pmbus) Add explicit support for DPS-460, DPS-800, and SGD009
On Mon, 25 Jul 2016 10:55:53 +, Vadim Pasternak wrote: > Provide support for PSU DPS-460, DPS-800 from Delta Electronics, INC and for > SGD009 from Acbel Polytech, INC. > These devices do not support the STATUS_CML register, and reports a > communication error in response to this command. > For this reason, the status register check is disabled for these controllers. > > Signed-off-by: Vadim Pasternak <vad...@mellanox.com> > Reviewed-by: Jiri Pirko <j...@mellanox.com> > --- > drivers/hwmon/pmbus/pmbus.c | 20 ++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > (...) Reviewed-by: Jean Delvare <jdelv...@suse.de> I leave the final ack to Guenter. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: contact / hwmon
On Wed, 15 Jun 2016 11:40:56 -0700, Guenter Roeck wrote: > On Wed, Jun 15, 2016 at 02:03:08PM +0200, Andrea Scopece wrote: > > > > Hello, > > > > my system is afflicted from a small bug, from a long time, and I have no > > idea > > where to report it. > > > > Just found your email address on github, linked to hwmon related commits, > > so > > I decided to write to you directly, hope this is not much of problem. > > If I did it wrong, my apologies, and if you can please direct me to the > > right > > place. > > > > > > The bug: > > > > I'm using a "conky" panel to monitor some parameters, among them the CPU > > temps. > > > > What is happening is that upon reboot, sometimes the temps display > > correctly, > > sometimes not. (by reboot, I mean also complete power off / power on > > sequence). > > > > I did a bit of research and found that the hwmon instance sometime is > > enumerated as 0 and sometimes as 1 ... both works but obviously when I make > > a > > constant reference in conky, say "hwmon 0" it work only half of the times. > > > > These are the involved parameters: > > sometimes as: > > /sys/devices/virtual/hwmon/hwmon0/temp1_input > > /sys/devices/virtual/hwmon/hwmon0/temp1_label > > /sys/devices/virtual/hwmon/hwmon0/temp2_input > > /sys/devices/virtual/hwmon/hwmon0/temp2_label > > > > sometimes as: > > /sys/devices/virtual/hwmon/hwmon1/temp1_input > > /sys/devices/virtual/hwmon/hwmon1/temp1_label > > /sys/devices/virtual/hwmon/hwmon1/temp2_input > > /sys/devices/virtual/hwmon/hwmon1/temp2_label > > > > > > coretemp enumeration show the same behavior: > > /sys/devices/platform/coretemp.0 > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp3_input > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp3_label > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp2_crit_alarm > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp2_crit > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp3_crit > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp3_crit_alarm > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp2_max > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp3_max > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp2_input > > /sys/devices/platform/coretemp.0/hwmon/hwmon0/temp2_label > > > > sometimes as: > > /sys/devices/platform/coretemp.0 > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp3_input > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp3_label > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp2_crit_alarm > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp2_crit > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp3_crit > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp3_crit_alarm > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp2_max > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp3_max > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp2_input > > /sys/devices/platform/coretemp.0/hwmon/hwmon1/temp2_label > > Unfortunately, that is a "feature". Numbering is determined by the module > load order. hwmon0 is the first registered hwmon device, hwmon1 is the next, > and so on. Since the module load order is not fixed, device name assignments > are not fixed either. > > This is quite similar to network device naming problems; unfortunately > the solution applied there (using udev to select fixed names) does not work > with non-network devices. > > I know this is very annoying, but right now I don't have a good idea > how to solve it. libsensors provides stable names for monitoring devices. conky should simply use it. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] hwmon: (sht3x) set initial jiffies to last_update
On Sun, 24 Jul 2016 17:05:32 -0700, Matt Ranostay wrote: > Handling the wraparound requires the data->last_update to be set to an > initial jiffies value. Otherwise on 32-bit systems you will not be able > to request a reading till the 5 minute jiffies rollover happens. > > Cc: Guenter Roeck <li...@roeck-us.net> > Cc: David Frey <david.f...@sensirion.com> > Signed-off-by: Matt Ranostay <mranos...@gmail.com> > --- > > Changes from v1: > * document more in commit message the reason for the patch > * mark last sample before driver load so initial reading can happen >shortly after boot > > drivers/hwmon/sht3x.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c > index b73a48832732..6ea99cd6ae79 100644 > --- a/drivers/hwmon/sht3x.c > +++ b/drivers/hwmon/sht3x.c > @@ -720,7 +720,7 @@ static int sht3x_probe(struct i2c_client *client, > data->setup.blocking_io = false; > data->setup.high_precision = true; > data->mode = 0; > - data->last_update = 0; > + data->last_update = jiffies - msecs_to_jiffies(3000); > data->client = client; > crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL); > Reviewed-by: Jean Delvare <jdelv...@suse.de> -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: sht3x: set initial jiffies to last_update
Hi Matt, On Fri, 8 Jul 2016 17:22:10 -0700, Matt Ranostay wrote: > On Fri, Jul 8, 2016 at 12:56 AM, Jean Delvare <jdelv...@suse.de> wrote: > > On jeu., 2016-07-07 at 19:46 -0700, Matt Ranostay wrote: > >> Handling the wraparound requires the data->last_update to be set to an > >> initial jiffies value. Otherwise you can start in a state where the > >> sensor will never request a reading. > > > > I can't see how. As I read the code, in the worst case, readings can be > > blocked for interval_ms (2 seconds maximum.) > > On 64-bit systems this is never an issue because the jiffies counter > will never wrap around. > > But my system is a 32-bit ARM core, so the the kernel sets the initial > value to 0xfffb6c20 so it will wrap around in 5 minutes to find buggy > code. > > So looking at time_after(0xfffb6c20, 0) will return false always till > it finally rolls over. I've always been confused by how jiffies wrapping is handled. So I tested it, and you are correct. > >> Cc: Guenter Roeck <li...@roeck-us.net> > >> Cc: David Frey <david.f...@sensirion.com> > >> Signed-off-by: Matt Ranostay <mranos...@gmail.com> > >> --- > >> drivers/hwmon/sht3x.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c > >> index 450645b6053d..05a925257938 100644 > >> --- a/drivers/hwmon/sht3x.c > >> +++ b/drivers/hwmon/sht3x.c > >> @@ -722,7 +722,7 @@ static int sht3x_probe(struct i2c_client *client, > >> data->setup.blocking_io = false; > >> data->setup.high_precision = true; > >> data->mode = 0; > >> - data->last_update = 0; > >> + data->last_update = jiffies; > >> data->client = client; > >> crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL); > >> > > > > Both look equally wrong to me. With your proposal, accessing the sysfs > > attributes right after loading the driver will not trigger a reading. > > > > In order to guarantee that the first access will trigger a reading, > > data->last_update should be initialized to jiffies - > > msecs_to_jiffies(2000) (the maximum interval value.) > > Ok that is fine. Rather do jiffies + (2 * HZ) 2 * HZ is fine. But it's minus, not plus. You want to initialize the last update time to 2 seconds BEFORE the driver is being loaded, so that the current time is at least 2 seconds AFTER the (fake) last update at first access. And you should subtract another jiffy to be on the safe side. Can you please send an updated patch? -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/25] hwmon: (it87) Various enhancements and fixes
Hi Guenter, On Sat, 9 Apr 2016 08:07:27 -0700, Guenter Roeck wrote: > Hi Martin, > > On 04/09/2016 02:33 AM, Martin Blumenstingl wrote: > > Guenter Roeck patchwork.roeck-us.net> writes: > >> From: Guenter Roeck roeck-us.net> > >> > >> This patch series builds on top of v4.5-rc1. The series was tested on > >> a system with IT8728F. Out-of-kernel versions were tested by various > >> people using a github repository. > > I've been running the whole series on top of v4.5 for the past two > > weeks on a system with IT8628E. I did not find any problems so far. > > thanks a lot for the feedback. It would be great if you can send me > an official Tested-by: that I can add into the series (or a permission > to do so). > > With the experience gained with the off-list driver, I think this series > is good to go for upstream, even without code review feedback. Otherwise > I fear we'll never get there. Jean, any objections ? I am sorry that I do not have the time to review this patch series. But I trust your judgment and I have no objection, if you think the series is good to go upstream then please push it there. Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
