Re: [PATCH] hwmon: (nzxt-kraken2) mark and order concurrent accesses
On Tue, Mar 30, 2021 at 03:51:21AM -0700, Guenter Roeck wrote: > [ ... ] > > Then please explain why _this_ use of time_after() is wrong but all > others in the kernel are not. Also, please note that we are not > concerned with code generation by the compiler as long as the > generated code is correct (and I don't see any indication that > it isn't). The accesses to priv->temp_input[], ->fan_input[] and ->updated (where this relates to your question about time_after()) are not wrong, but undefined. But if we are not concerned with code that is currently generated correctly, which indeed is the case in the five arch x compiler combinations I tested, then there simply is no point to this patch. Thanks for going through this with me, Jonas P.S. Admittedly from a sample way too small, but in the kernel time_after(jiffies, x) calls do not generally appear to involve an expression x with a data race. And there are a few calls where READ_ONCE() is indeed used in x.
Re: [PATCH] hwmon: (nzxt-kraken2) mark and order concurrent accesses
On Mon, Mar 29, 2021 at 10:43:55PM -0700, Guenter Roeck wrote: > On 3/29/21 8:16 PM, Jonas Malaco wrote: > > On Mon, Mar 29, 2021 at 06:01:00PM -0700, Guenter Roeck wrote: > >> On 3/29/21 5:21 PM, Jonas Malaco wrote: > >>> On Mon, Mar 29, 2021 at 02:53:39PM -0700, Guenter Roeck wrote: > >>>> On Mon, Mar 29, 2021 at 05:22:01AM -0300, Jonas Malaco wrote: > >>>>> To avoid a spinlock, the driver explores concurrent memory accesses > >>>>> between _raw_event and _read, having the former updating fields on a > >>>>> data structure while the latter could be reading from them. Because > >>>>> these are "plain" accesses, those are data races according to the Linux > >>>>> kernel memory model (LKMM). > >>>>> > >>>>> Data races are undefined behavior in both C11 and LKMM. In practice, > >>>>> the compiler is free to make optimizations assuming there is no data > >>>>> race, including load tearing, load fusing and many others,[1] most of > >>>>> which could result in corruption of the values reported to user-space. > >>>>> > >>>>> Prevent undesirable optimizations to those concurrent accesses by > >>>>> marking them with READ_ONCE() and WRITE_ONCE(). This also removes the > >>>>> data races, according to the LKMM, because both loads and stores to each > >>>>> location are now "marked" accesses. > >>>>> > >>>>> As a special case, use smp_load_acquire() and smp_load_release() when > >>>>> loading and storing ->updated, as it is used to track the validity of > >>>>> the other values, and thus has to be stored after and loaded before > >>>>> them. These imply READ_ONCE()/WRITE_ONCE() but also ensure the desired > >>>>> order of memory accesses. > >>>>> > >>>>> [1] https://lwn.net/Articles/793253/ > >>>>> > >>>> > >>>> I think you lost me a bit there. What out-of-order accesses that would be > >>>> triggered by a compiler optimization are you concerned about here ? > >>>> The only "problem" I can think of is that priv->updated may have been > >>>> written before the actual values. The impact would be ... zero. An > >>>> attribute read would return "stale" data for a few microseconds. > >>>> Why is that a concern, and what difference does it make ? > >>> > >>> The impact of out-of-order accesses to priv->updated is indeed minimal. > >>> > >>> That said, smp_load_acquire() and smp_store_release() were meant to > >>> prevent reordering at runtime, and only affect architectures other than > >>> x86. READ_ONCE() and WRITE_ONCE() would already prevent reordering from > >>> compiler optimizations, and x86 provides the load-acquire/store-release > >>> semantics by default. > >>> > >>> But the reordering issue is not a concern to me, I got carried away when > >>> adding READ_ONCE()/WRITE_ONCE(). While smp_load_acquire() and > >>> smp_store_release() make the code work more like I intend it to, they > >>> are (small) costs we can spare. > >>> > >>> I still think that READ_ONCE()/WRITE_ONCE() are necessary, including for > >>> priv->updated. Do you agree? > >>> > >> > >> No. What is the point ? The order of writes doesn't matter, the writes > >> won't > >> be randomly dropped, and it doesn't matter if the reader reports old values > >> for a couple of microseconds either. This would be different if the values > >> were used as synchronization primitives or similar, but that isn't the case > >> here. As for priv->updated, if you are concerned about lost reports and > >> the 4th report is received a few microseconds before the read, I'd suggest > >> to loosen the interval a bit instead. > >> > >> Supposedly we are getting reports every 500ms. We have two situations: > >> - More than three reports are lost, making priv->updated somewhat relevant. > >> In this case, it doesn't matter if outdated values are reported for > >> a few uS since most/many/some reports are outdated more than a second > >> anyway. > >> - A report is received but old values are reported for a few uS. That > >> doesn't matter either because reports are always
Re: [PATCH] hwmon: (nzxt-kraken2) mark and order concurrent accesses
On Mon, Mar 29, 2021 at 06:01:00PM -0700, Guenter Roeck wrote: > On 3/29/21 5:21 PM, Jonas Malaco wrote: > > On Mon, Mar 29, 2021 at 02:53:39PM -0700, Guenter Roeck wrote: > >> On Mon, Mar 29, 2021 at 05:22:01AM -0300, Jonas Malaco wrote: > >>> To avoid a spinlock, the driver explores concurrent memory accesses > >>> between _raw_event and _read, having the former updating fields on a > >>> data structure while the latter could be reading from them. Because > >>> these are "plain" accesses, those are data races according to the Linux > >>> kernel memory model (LKMM). > >>> > >>> Data races are undefined behavior in both C11 and LKMM. In practice, > >>> the compiler is free to make optimizations assuming there is no data > >>> race, including load tearing, load fusing and many others,[1] most of > >>> which could result in corruption of the values reported to user-space. > >>> > >>> Prevent undesirable optimizations to those concurrent accesses by > >>> marking them with READ_ONCE() and WRITE_ONCE(). This also removes the > >>> data races, according to the LKMM, because both loads and stores to each > >>> location are now "marked" accesses. > >>> > >>> As a special case, use smp_load_acquire() and smp_load_release() when > >>> loading and storing ->updated, as it is used to track the validity of > >>> the other values, and thus has to be stored after and loaded before > >>> them. These imply READ_ONCE()/WRITE_ONCE() but also ensure the desired > >>> order of memory accesses. > >>> > >>> [1] https://lwn.net/Articles/793253/ > >>> > >> > >> I think you lost me a bit there. What out-of-order accesses that would be > >> triggered by a compiler optimization are you concerned about here ? > >> The only "problem" I can think of is that priv->updated may have been > >> written before the actual values. The impact would be ... zero. An > >> attribute read would return "stale" data for a few microseconds. > >> Why is that a concern, and what difference does it make ? > > > > The impact of out-of-order accesses to priv->updated is indeed minimal. > > > > That said, smp_load_acquire() and smp_store_release() were meant to > > prevent reordering at runtime, and only affect architectures other than > > x86. READ_ONCE() and WRITE_ONCE() would already prevent reordering from > > compiler optimizations, and x86 provides the load-acquire/store-release > > semantics by default. > > > > But the reordering issue is not a concern to me, I got carried away when > > adding READ_ONCE()/WRITE_ONCE(). While smp_load_acquire() and > > smp_store_release() make the code work more like I intend it to, they > > are (small) costs we can spare. > > > > I still think that READ_ONCE()/WRITE_ONCE() are necessary, including for > > priv->updated. Do you agree? > > > > No. What is the point ? The order of writes doesn't matter, the writes won't > be randomly dropped, and it doesn't matter if the reader reports old values > for a couple of microseconds either. This would be different if the values > were used as synchronization primitives or similar, but that isn't the case > here. As for priv->updated, if you are concerned about lost reports and > the 4th report is received a few microseconds before the read, I'd suggest > to loosen the interval a bit instead. > > Supposedly we are getting reports every 500ms. We have two situations: > - More than three reports are lost, making priv->updated somewhat relevant. > In this case, it doesn't matter if outdated values are reported for > a few uS since most/many/some reports are outdated more than a second > anyway. > - A report is received but old values are reported for a few uS. That > doesn't matter either because reports are always outdated anyway by > much more than a few uS anyway, and the code already tolerates up to > 2 seconds of lost reports. > > Sorry, I completely fail to see the problem you are trying to solve here. Please disregard the out-of-order accesses, I agree that preventing them "are a (small) cost we can spare". The main problem I still would like to address are the data races. While the stores and loads cannot be dropped, and we can tolerate their reordering, they could still be teared, fused, perhaps invented... According to [1] these types of optimizations are not unheard. Load tearing alone could easily produce values that are not stale, bu
Re: [PATCH] hwmon: (nzxt-kraken2) mark and order concurrent accesses
On Mon, Mar 29, 2021 at 02:53:39PM -0700, Guenter Roeck wrote: > On Mon, Mar 29, 2021 at 05:22:01AM -0300, Jonas Malaco wrote: > > To avoid a spinlock, the driver explores concurrent memory accesses > > between _raw_event and _read, having the former updating fields on a > > data structure while the latter could be reading from them. Because > > these are "plain" accesses, those are data races according to the Linux > > kernel memory model (LKMM). > > > > Data races are undefined behavior in both C11 and LKMM. In practice, > > the compiler is free to make optimizations assuming there is no data > > race, including load tearing, load fusing and many others,[1] most of > > which could result in corruption of the values reported to user-space. > > > > Prevent undesirable optimizations to those concurrent accesses by > > marking them with READ_ONCE() and WRITE_ONCE(). This also removes the > > data races, according to the LKMM, because both loads and stores to each > > location are now "marked" accesses. > > > > As a special case, use smp_load_acquire() and smp_load_release() when > > loading and storing ->updated, as it is used to track the validity of > > the other values, and thus has to be stored after and loaded before > > them. These imply READ_ONCE()/WRITE_ONCE() but also ensure the desired > > order of memory accesses. > > > > [1] https://lwn.net/Articles/793253/ > > > > I think you lost me a bit there. What out-of-order accesses that would be > triggered by a compiler optimization are you concerned about here ? > The only "problem" I can think of is that priv->updated may have been > written before the actual values. The impact would be ... zero. An > attribute read would return "stale" data for a few microseconds. > Why is that a concern, and what difference does it make ? The impact of out-of-order accesses to priv->updated is indeed minimal. That said, smp_load_acquire() and smp_store_release() were meant to prevent reordering at runtime, and only affect architectures other than x86. READ_ONCE() and WRITE_ONCE() would already prevent reordering from compiler optimizations, and x86 provides the load-acquire/store-release semantics by default. But the reordering issue is not a concern to me, I got carried away when adding READ_ONCE()/WRITE_ONCE(). While smp_load_acquire() and smp_store_release() make the code work more like I intend it to, they are (small) costs we can spare. I still think that READ_ONCE()/WRITE_ONCE() are necessary, including for priv->updated. Do you agree? Thanks, Jonas P.S. Architectures other than x86 are admittedly a niche case for this driver, but I would not rule them out. Not only can the cooler be adapted to mount on silicon other than mainstream Intel/AMD CPUs (and there even exists a first-party adapter for graphics cards), but it can trivially also be controlled by a secondary, possibly non-x86, system.
[PATCH] hwmon: (nzxt-kraken2) mark and order concurrent accesses
To avoid a spinlock, the driver explores concurrent memory accesses
between _raw_event and _read, having the former updating fields on a
data structure while the latter could be reading from them. Because
these are "plain" accesses, those are data races according to the Linux
kernel memory model (LKMM).
Data races are undefined behavior in both C11 and LKMM. In practice,
the compiler is free to make optimizations assuming there is no data
race, including load tearing, load fusing and many others,[1] most of
which could result in corruption of the values reported to user-space.
Prevent undesirable optimizations to those concurrent accesses by
marking them with READ_ONCE() and WRITE_ONCE(). This also removes the
data races, according to the LKMM, because both loads and stores to each
location are now "marked" accesses.
As a special case, use smp_load_acquire() and smp_load_release() when
loading and storing ->updated, as it is used to track the validity of
the other values, and thus has to be stored after and loaded before
them. These imply READ_ONCE()/WRITE_ONCE() but also ensure the desired
order of memory accesses.
[1] https://lwn.net/Articles/793253/
Signed-off-by: Jonas Malaco
---
drivers/hwmon/nzxt-kraken2.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
index 89f7ea4f42d4..f4fbc8771930 100644
--- a/drivers/hwmon/nzxt-kraken2.c
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -46,16 +46,22 @@ static int kraken2_read(struct device *dev, enum
hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
struct kraken2_priv_data *priv = dev_get_drvdata(dev);
+ unsigned long expires;
- if (time_after(jiffies, priv->updated + STATUS_VALIDITY * HZ))
+ /*
+* Order load from ->updated before the data it refers to.
+*/
+ expires = smp_load_acquire(>updated) + STATUS_VALIDITY * HZ;
+
+ if (time_after(jiffies, expires))
return -ENODATA;
switch (type) {
case hwmon_temp:
- *val = priv->temp_input[channel];
+ *val = READ_ONCE(priv->temp_input[channel]);
break;
case hwmon_fan:
- *val = priv->fan_input[channel];
+ *val = READ_ONCE(priv->fan_input[channel]);
break;
default:
return -EOPNOTSUPP; /* unreachable */
@@ -119,12 +125,15 @@ static int kraken2_raw_event(struct hid_device *hdev,
* and that the missing steps are artifacts of how the firmware
* processes the raw sensor data.
*/
- priv->temp_input[0] = data[1] * 1000 + data[2] * 100;
+ WRITE_ONCE(priv->temp_input[0], data[1] * 1000 + data[2] * 100);
- priv->fan_input[0] = get_unaligned_be16(data + 3);
- priv->fan_input[1] = get_unaligned_be16(data + 5);
+ WRITE_ONCE(priv->fan_input[0], get_unaligned_be16(data + 3));
+ WRITE_ONCE(priv->fan_input[1], get_unaligned_be16(data + 5));
- priv->updated = jiffies;
+ /*
+* Order store to ->updated after the data it refers to.
+*/
+ smp_store_release(>updated, jiffies);
return 0;
}
base-commit: 644b9af5c605762feffac96bd7ea2499e0197656
--
2.31.1
Re: [PATCH v2] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
On Fri, Mar 19, 2021 at 02:26:40PM -0700, Guenter Roeck wrote: > On Fri, Mar 19, 2021 at 01:55:44AM -0300, Jonas Malaco wrote: > > These are "all-in-one" CPU liquid coolers that can be monitored and > > controlled through a proprietary USB HID protocol. > > > > While the models have differently sized radiators and come with varying > > numbers of fans, they are all indistinguishable at the software level. > > > > The driver exposes fan/pump speeds and coolant temperature through the > > standard hwmon sysfs interface. > > > > Fan and pump control, while supported by the devices, are not currently > > exposed. The firmware accepts up to 61 trip points per channel > > (fan/pump), but the same set of trip temperatures has to be maintained > > for both; with pwmX_auto_point_Y_temp attributes, users would need to > > maintain this invariant themselves. > > > > Instead, fan and pump control, as well as LED control (which the device > > also supports for 9 addressable RGB LEDs on the CPU water block) are > > left for existing and already mature user-space tools, which can still > > be used alongside the driver, thanks to hidraw. A link to one, which I > > also maintain, is provided in the documentation. > > > > The implementation is based on USB traffic analysis. It has been > > runtime tested on x86_64, both as a built-in driver and as a module. > > > > Signed-off-by: Jonas Malaco > > Applied (after removing the now unnecessary spinlock.h include). Thanks for catching/fixing that. Jonas > > Thanks, > Guenter
[PATCH v2] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
These are "all-in-one" CPU liquid coolers that can be monitored and controlled through a proprietary USB HID protocol. While the models have differently sized radiators and come with varying numbers of fans, they are all indistinguishable at the software level. The driver exposes fan/pump speeds and coolant temperature through the standard hwmon sysfs interface. Fan and pump control, while supported by the devices, are not currently exposed. The firmware accepts up to 61 trip points per channel (fan/pump), but the same set of trip temperatures has to be maintained for both; with pwmX_auto_point_Y_temp attributes, users would need to maintain this invariant themselves. Instead, fan and pump control, as well as LED control (which the device also supports for 9 addressable RGB LEDs on the CPU water block) are left for existing and already mature user-space tools, which can still be used alongside the driver, thanks to hidraw. A link to one, which I also maintain, is provided in the documentation. The implementation is based on USB traffic analysis. It has been runtime tested on x86_64, both as a built-in driver and as a module. Signed-off-by: Jonas Malaco --- Changes in v2: - remove unnecessary type, attr and channel checks from _is_visible - remove unnecessary attr and channel checks from _read/_read_string - remove the spinlock by computing values in the event handler - add comment describing how the device reports data - report missing or stale data to user-space - adjust copyright notice Documentation/hwmon/index.rst| 1 + Documentation/hwmon/nzxt-kraken2.rst | 42 + MAINTAINERS | 7 + drivers/hwmon/Kconfig| 10 ++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nzxt-kraken2.c | 235 +++ 6 files changed, 296 insertions(+) create mode 100644 Documentation/hwmon/nzxt-kraken2.rst create mode 100644 drivers/hwmon/nzxt-kraken2.c diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index d4b422edbe3a..48bfa7887dd4 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -143,6 +143,7 @@ Hardware Monitoring Kernel Drivers npcm750-pwm-fan nsa320 ntc_thermistor + nzxt-kraken2 occ pc87360 pc87427 diff --git a/Documentation/hwmon/nzxt-kraken2.rst b/Documentation/hwmon/nzxt-kraken2.rst new file mode 100644 index ..94025de65a81 --- /dev/null +++ b/Documentation/hwmon/nzxt-kraken2.rst @@ -0,0 +1,42 @@ +.. SPDX-License-Identifier: GPL-2.0-or-later + +Kernel driver nzxt-kraken2 +== + +Supported devices: + +* NZXT Kraken X42 +* NZXT Kraken X52 +* NZXT Kraken X62 +* NZXT Kraken X72 + +Author: Jonas Malaco + +Description +--- + +This driver enables hardware monitoring support for NZXT Kraken X42/X52/X62/X72 +all-in-one CPU liquid coolers. Three sensors are available: fan speed, pump +speed and coolant temperature. + +Fan and pump control, while supported by the firmware, are not currently +exposed. The addressable RGB LEDs, present in the integrated CPU water block +and pump head, are not supported either. But both features can be found in +existing user-space tools (e.g. `liquidctl`_). + +.. _liquidctl: https://github.com/liquidctl/liquidctl + +Usage Notes +--- + +As these are USB HIDs, the driver can be loaded automatically by the kernel and +supports hot swapping. + +Sysfs entries +- + +=== +fan1_input Fan speed (in rpm) +fan2_input Pump speed (in rpm) +temp1_inputCoolant temperature (in millidegrees Celsius) +=== diff --git a/MAINTAINERS b/MAINTAINERS index 0635b30e467c..b8f9fc5eaf08 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12911,6 +12911,13 @@ L: linux-...@lists.01.org (moderated for non-subscribers) S: Supported F: drivers/nfc/nxp-nci +NZXT-KRAKEN2 HARDWARE MONITORING DRIVER +M: Jonas Malaco +L: linux-hw...@vger.kernel.org +S: Maintained +F: Documentation/hwmon/nzxt-kraken2.rst +F: drivers/hwmon/nzxt-kraken2.c + OBJAGG M: Jiri Pirko L: net...@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 54f04e61fb83..0ddc974b102e 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1492,6 +1492,16 @@ config SENSORS_NSA320 This driver can also be built as a module. If so, the module will be called nsa320-hwmon. +config SENSORS_NZXT_KRAKEN2 + tristate "NZXT Kraken X42/X51/X62/X72 liquid coolers" + depends on USB_HID + help + If you say yes here you get support for hardware monitoring for the + NZXT Kraken X42/X52/X62/X72 all-in-one CPU liquid coolers. + + This driver can also be buil
Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
On Thu, Mar 18, 2021 at 04:36:08PM -0700, Guenter Roeck wrote: > On Thu, Mar 18, 2021 at 08:15:06PM -0300, Jonas Malaco wrote: > > [ ... ] > > > > Either case, the spinlocks are overkill. It would be much easier to > > > convert raw readings here into temperature and fan speed and store > > > the resulting values in struct kraken2_priv_data, and then to > > > just report it in the read functions. That would be much less costly > > > because the spinlock would not be needed at all, and calculations > > > would be done only once per event. > > > > Oddly enough, this is very close to how the code read last week. > > > > But I was storing the values in kraken2_priv_data as longs, and I'm not > > sure that storing and loading longs is atomic on all architectures. > > > > Was that safe, or should I use something else instead of longs? > > > > Hard to say, but do you see any code in the kernel that assumes > that loading or storing a long is not atomic, for any architecture ? No, I don't think so. > > Also, I don't see how u16 * 1000 could ever require a long > for storage. int would be good enough. Oh, that's true. I'll submit a v2 shortly. Thanks again, Jonas > > > > > > > > + memcpy(priv->status, data, STATUS_USEFUL_SIZE); > > > > + spin_unlock_irqrestore(>lock, flags); > > > > + > > > > + return 0; > > > > +} > > > > > > For my education: What triggers those events ? Are they reported > > > by the hardware autonomously whenever something changes ? > > > A comment at the top of the driver explaining how this works > > > might be useful. > > > > The device autonomously sends these status reports twice a second. > > > > I'll add the comment for v2. > > > That would be great, thanks. > > > > > > > Also, is there a way to initialize values during probe ? Otherwise > > > the driver would report values of 0 until the hardware reports > > > something. > > > > The device doesn't respond to HID Get_Report, so we can't get valid > > initial values. > > > > The best we can do is identify the situation and report it to > > user-space. Am I right that ENODATA should be used for this? > > > Yes, I think that would be a good idea, and ENODATA sounds good. > > Thanks, > Guenter
Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
On Thu, Mar 18, 2021 at 11:55:39AM -0700, Guenter Roeck wrote:
> On 3/18/21 9:48 AM, Jonas Malaco wrote:
> > These are "all-in-one" CPU liquid coolers that can be monitored and
> > controlled through a proprietary USB HID protocol.
> >
> > While the models have differently sized radiators and come with varying
> > numbers of fans, they are all indistinguishable at the software level.
> > > The driver exposes fan/pump speeds and coolant temperature through the
> > standard hwmon sysfs interface.
> >
> > Fan and pump control, while supported by the devices, are not currently
> > exposed. The firmware accepts up to 61 trip points per channel
> > (fan/pump), but the same set of trip temperatures has to be maintained
> > for both; with pwmX_auto_point_Y_temp attributes, users would need to
> > maintain this invariant themselves.
> >
> > Instead, fan and pump control, as well as LED control (which the device
> > also supports for 9 addressable RGB LEDs on the CPU water block) are
> > left for existing and already mature user-space tools, which can still
> > be used alongside the driver, thanks to hidraw. A link to one, which I
> > also maintain, is provided in the documentation.
> >
> > The implementation is based on USB traffic analysis. It has been
> > runtime tested on x86_64, both as a built-in driver and as a module.
> >
> > Signed-off-by: Jonas Malaco
> > ---
> >
> > I was not sure whether to exhaustively check type, attr and channel in
> > _is_visible/_read/_read_string. Would it be preferred if those
> > functions assumed that they would never be called for unsupported
> > combinations, since that would be a programming error?
> >
> > In practice, should kraken2_is_visible be simplified into
> >
> > static umode_t kraken2_is_visible(...)
> > {
> > return 0444;
> > }
> >
> Yes, if nothing is optional, and all permissions are 0444.
>
> > and should _read/_read_string go through similar (but not as effective)
> > simplifications?
> >
> Unless I am missing something, all the channel checks are unnecessary
> and should be removed.
I'll remove them and the checks in kraken2_read for v2.
>
> > On another note, the copyright dates back to 2019 because this driver
> > was left to mature (and then was mostly forgotten about) in an
> > out-of-tree repository.[1]
> >
> > [1] https://github.com/liquidctl/liquidtux
> >
> > Documentation/hwmon/index.rst| 1 +
> > Documentation/hwmon/nzxt-kraken2.rst | 42
> > MAINTAINERS | 7 +
> > drivers/hwmon/Kconfig| 10 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/nzxt-kraken2.c | 279 +++
> > 6 files changed, 340 insertions(+)
> > create mode 100644 Documentation/hwmon/nzxt-kraken2.rst
> > create mode 100644 drivers/hwmon/nzxt-kraken2.c
> >
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index d4b422edbe3a..48bfa7887dd4 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -143,6 +143,7 @@ Hardware Monitoring Kernel Drivers
> > npcm750-pwm-fan
> > nsa320
> > ntc_thermistor
> > + nzxt-kraken2
> > occ
> > pc87360
> > pc87427
> > diff --git a/Documentation/hwmon/nzxt-kraken2.rst
> > b/Documentation/hwmon/nzxt-kraken2.rst
> > new file mode 100644
> > index ..94025de65a81
> > --- /dev/null
> > +++ b/Documentation/hwmon/nzxt-kraken2.rst
> > @@ -0,0 +1,42 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver nzxt-kraken2
> > +==
> > +
> > +Supported devices:
> > +
> > +* NZXT Kraken X42
> > +* NZXT Kraken X52
> > +* NZXT Kraken X62
> > +* NZXT Kraken X72
> > +
> > +Author: Jonas Malaco
> > +
> > +Description
> > +---
> > +
> > +This driver enables hardware monitoring support for NZXT Kraken
> > X42/X52/X62/X72
> > +all-in-one CPU liquid coolers. Three sensors are available: fan speed,
> > pump
> > +speed and coolant temperature.
> > +
> > +Fan and pump control, while supported by the firmware, are not currently
> > +exposed. The addressable RGB LEDs, present in the integrated CPU water
> > block
> > +and pump head,
[PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72
These are "all-in-one" CPU liquid coolers that can be monitored and
controlled through a proprietary USB HID protocol.
While the models have differently sized radiators and come with varying
numbers of fans, they are all indistinguishable at the software level.
The driver exposes fan/pump speeds and coolant temperature through the
standard hwmon sysfs interface.
Fan and pump control, while supported by the devices, are not currently
exposed. The firmware accepts up to 61 trip points per channel
(fan/pump), but the same set of trip temperatures has to be maintained
for both; with pwmX_auto_point_Y_temp attributes, users would need to
maintain this invariant themselves.
Instead, fan and pump control, as well as LED control (which the device
also supports for 9 addressable RGB LEDs on the CPU water block) are
left for existing and already mature user-space tools, which can still
be used alongside the driver, thanks to hidraw. A link to one, which I
also maintain, is provided in the documentation.
The implementation is based on USB traffic analysis. It has been
runtime tested on x86_64, both as a built-in driver and as a module.
Signed-off-by: Jonas Malaco
---
I was not sure whether to exhaustively check type, attr and channel in
_is_visible/_read/_read_string. Would it be preferred if those
functions assumed that they would never be called for unsupported
combinations, since that would be a programming error?
In practice, should kraken2_is_visible be simplified into
static umode_t kraken2_is_visible(...)
{
return 0444;
}
and should _read/_read_string go through similar (but not as effective)
simplifications?
On another note, the copyright dates back to 2019 because this driver
was left to mature (and then was mostly forgotten about) in an
out-of-tree repository.[1]
[1] https://github.com/liquidctl/liquidtux
Documentation/hwmon/index.rst| 1 +
Documentation/hwmon/nzxt-kraken2.rst | 42
MAINTAINERS | 7 +
drivers/hwmon/Kconfig| 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/nzxt-kraken2.c | 279 +++
6 files changed, 340 insertions(+)
create mode 100644 Documentation/hwmon/nzxt-kraken2.rst
create mode 100644 drivers/hwmon/nzxt-kraken2.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index d4b422edbe3a..48bfa7887dd4 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -143,6 +143,7 @@ Hardware Monitoring Kernel Drivers
npcm750-pwm-fan
nsa320
ntc_thermistor
+ nzxt-kraken2
occ
pc87360
pc87427
diff --git a/Documentation/hwmon/nzxt-kraken2.rst
b/Documentation/hwmon/nzxt-kraken2.rst
new file mode 100644
index ..94025de65a81
--- /dev/null
+++ b/Documentation/hwmon/nzxt-kraken2.rst
@@ -0,0 +1,42 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver nzxt-kraken2
+==
+
+Supported devices:
+
+* NZXT Kraken X42
+* NZXT Kraken X52
+* NZXT Kraken X62
+* NZXT Kraken X72
+
+Author: Jonas Malaco
+
+Description
+---
+
+This driver enables hardware monitoring support for NZXT Kraken X42/X52/X62/X72
+all-in-one CPU liquid coolers. Three sensors are available: fan speed, pump
+speed and coolant temperature.
+
+Fan and pump control, while supported by the firmware, are not currently
+exposed. The addressable RGB LEDs, present in the integrated CPU water block
+and pump head, are not supported either. But both features can be found in
+existing user-space tools (e.g. `liquidctl`_).
+
+.. _liquidctl: https://github.com/liquidctl/liquidctl
+
+Usage Notes
+---
+
+As these are USB HIDs, the driver can be loaded automatically by the kernel and
+supports hot swapping.
+
+Sysfs entries
+-
+
+===
+fan1_input Fan speed (in rpm)
+fan2_input Pump speed (in rpm)
+temp1_inputCoolant temperature (in millidegrees Celsius)
+===
diff --git a/MAINTAINERS b/MAINTAINERS
index 0635b30e467c..b8f9fc5eaf08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12911,6 +12911,13 @@ L: linux-...@lists.01.org (moderated for
non-subscribers)
S: Supported
F: drivers/nfc/nxp-nci
+NZXT-KRAKEN2 HARDWARE MONITORING DRIVER
+M: Jonas Malaco
+L: linux-hw...@vger.kernel.org
+S: Maintained
+F: Documentation/hwmon/nzxt-kraken2.rst
+F: drivers/hwmon/nzxt-kraken2.c
+
OBJAGG
M: Jiri Pirko
L: net...@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 54f04e61fb83..0ddc974b102e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1492,6 +1492,16 @@ config SENSORS_NSA320
This driver can also be built as a module. If so,
Re: [PATCH] hwmon: corsair-psu: update supported devices
On Mon, Nov 30, 2020 at 2:22 AM Wilken Gottwalt wrote: > > On Sun, 29 Nov 2020 13:59:33 -0800 > Guenter Roeck wrote: > > > On Sun, Nov 29, 2020 at 04:54:43PM +0100, Wilken Gottwalt wrote: > > > On Sun, 29 Nov 2020 05:00:49 -0800 > > > Guenter Roeck wrote: > > > > > > > On Sun, Nov 29, 2020 at 07:36:18AM +0100, Wilken Gottwalt wrote: > > > > > On Sat, 28 Nov 2020 17:21:40 -0300 > > > > > Jonas Malaco wrote: > > > > > > > > > > > On Sat, Nov 28, 2020 at 7:35 AM Wilken Gottwalt > > > > > > wrote: > > > > > > > > > > > > > > On Sat, 28 Nov 2020 02:37:38 -0300 > > > > > > > Jonas Malaco wrote: > > > > > > > > > > > > > > > On Thu, Nov 26, 2020 at 8:43 AM Wilken Gottwalt > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Adds support for another Corsair PSUs series: AX760i, AX860i, > > > > > > > > > AX1200i, > > > > > > > > > AX1500i and AX1600i. The first 3 power supplies are supported > > > > > > > > > through > > > > > > > > > the Corsair Link USB Dongle which is some kind of > > > > > > > > > USB/Serial/TTL > > > > > > > > > converter especially made for the COM ports of these power > > > > > > > > > supplies. > > > > > > > > > There are 3 known revisions of these adapters. The AX1500i > > > > > > > > > power supply > > > > > > > > > has revision 3 built into the case and AX1600i is the only > > > > > > > > > one in that > > > > > > > > > series, which has an unique usb hid id like the RM/RX series. > > > > > > > > > > > > > > > > Can I ask what AXi power supplies were tested? > > > > > > > > > > > > > > > > I ask because, based on the user-space implementations I am > > > > > > > > aware of, > > > > > > > > the AXi dongle protocol appears to be different from the > > > > > > > > RMi/HXi series. > > > > > > > > > > > > > > I was not able to test this against the AX power supplies, they > > > > > > > are really > > > > > > > hard to find (and are far to expensive). But I went through all > > > > > > > these tools > > > > > > > and stuck to the most common commands, which all 3 series > > > > > > > support. Not every > > > > > > > series supports all commands (there also seem to be different > > > > > > > firmwares in > > > > > > > the micro-conrollers). But this is fine, some sensors will show > > > > > > > up as N/A. > > > > > > > Even my HX850i does not support all commands covered in this > > > > > > > driver. > > > > > > > > > > > > I think the similarities come from all using wrappers over the PMBus > > > > > > interface to the voltage controller. But I am not sure the wrapping > > > > > > protocols are identical. > > > > > > > > > > > > For example, cpsumon shows significantly more things going on > > > > > > during a > > > > > > read than what is needed for the RMi/HXi series.[1] > > > > > > > > > > > > [1] > > > > > > https://github.com/ka87/cpsumon/blob/fd639684d7f9/libcpsumon/src/cpsumon.c#L213-L231 > > > > > > > > > > > > > > > > > > > > > > > > > > > AXi dongle: > > > > > > > > - https://github.com/ka87/cpsumon > > > > > > > > > > > > > > This tool made me to consider including the AX series, because it > > > > > > > uses some > > > > > > > of the same commands on the AX760i, AX860i, AX1200i and AX1500i. > > > > > > > But it is > > > > > > > a usb-serial tool only. But it was nice to know, that the > > > > > > > commands are mostly > > > > > > > the same. I left out all the commands for configuring, PCIe pow
Re: [PATCH] hwmon: corsair-psu: update supported devices
On Thu, Nov 26, 2020 at 8:43 AM Wilken Gottwalt
wrote:
>
> Adds support for another Corsair PSUs series: AX760i, AX860i, AX1200i,
> AX1500i and AX1600i. The first 3 power supplies are supported through
> the Corsair Link USB Dongle which is some kind of USB/Serial/TTL
> converter especially made for the COM ports of these power supplies.
> There are 3 known revisions of these adapters. The AX1500i power supply
> has revision 3 built into the case and AX1600i is the only one in that
> series, which has an unique usb hid id like the RM/RX series.
Can I ask what AXi power supplies were tested?
I ask because, based on the user-space implementations I am aware of,
the AXi dongle protocol appears to be different from the RMi/HXi series.
AXi dongle:
- https://github.com/ka87/cpsumon
RMi/HXi:
- https://github.com/jonasmalacofilho/liquidctl
- https://github.com/audiohacked/OpenCorsairLink
- https://github.com/notaz/corsairmi
One additional concern is that the non-HID AXi dongles may only have bulk
USB endpoints, and this is a HID driver.[1]
Thanks,
Jonas
[1] https://github.com/ka87/cpsumon/issues/4
>
> The patch also changes the usb hid ids to use upper case letters to be
> consistent with the rest of the hex numbers in the driver and updates
> the hwmon documentation.
>
> This patch adds:
> - hwmon/corsair-psu documentation update
> - corsair-psu driver update
>
> Signed-off-by: Wilken Gottwalt
> ---
> Documentation/hwmon/corsair-psu.rst | 10 +
> drivers/hwmon/Kconfig | 7 +++---
> drivers/hwmon/corsair-psu.c | 33 +++--
> 3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/hwmon/corsair-psu.rst
> b/Documentation/hwmon/corsair-psu.rst
> index 396b95c9a76a..6227e9046d73 100644
> --- a/Documentation/hwmon/corsair-psu.rst
> +++ b/Documentation/hwmon/corsair-psu.rst
> @@ -7,6 +7,16 @@ Supported devices:
>
> * Corsair Power Supplies
>
> + Corsair AX760i (by Corsair Link USB Dongle)
> +
> + Corsair AX860i (by Corsair Link USB Dongle)
> +
> + Corsair AX1200i (by Corsair Link USB Dongle)
> +
> + Corsair AX1500i (by builtin Corsair Link USB Dongle)
> +
> + Corsair AX1600i
> +
>Corsair HX550i
>
>Corsair HX650i
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 716df51edc87..3c059fc23cd6 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -453,11 +453,12 @@ config SENSORS_CORSAIR_PSU
> tristate "Corsair PSU HID controller"
> depends on HID
> help
> - If you say yes here you get support for Corsair PSUs with a HID
> + If you say yes here you get support for Corsair PSUs with an USB HID
> interface.
> Currently this driver supports the (RM/HX)550i, (RM/HX)650i,
> - (RM/HX)750i, (RM/HX)850i, (RM/HX)1000i and HX1200i power supplies
> - by Corsair.
> + (RM/HX)750i, (RM/HX)850i, (RM/HX)1000i, HX1200i and AX1600i power
> + supplies by Corsair. The AX760i, AX860i, AX1200i and AX1500i
> + power supplies are supported through the Corsair Link USB Dongle.
>
> This driver can also be built as a module. If so, the module
> will be called corsair-psu.
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index 99494056f4bd..0146dda3e2c3 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -571,17 +571,28 @@ static int corsairpsu_raw_event(struct hid_device
> *hdev, struct hid_report *repo
> }
>
> static const struct hid_device_id corsairpsu_idtable[] = {
> - { HID_USB_DEVICE(0x1b1c, 0x1c03) }, /* Corsair HX550i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c04) }, /* Corsair HX650i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c05) }, /* Corsair HX750i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c06) }, /* Corsair HX850i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c07) }, /* Corsair HX1000i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c08) }, /* Corsair HX1200i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c09) }, /* Corsair RM550i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c0a) }, /* Corsair RM650i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c0b) }, /* Corsair RM750i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c0c) }, /* Corsair RM850i */
> - { HID_USB_DEVICE(0x1b1c, 0x1c0d) }, /* Corsair RM1000i */
> + /*
> +* The Corsair USB/COM Dongles appear in at least 3 different
> revisions, where rev 1 and 2
> +* are commonly used with the AX760i, AX860i and AX1200i, while rev3
> is rarely seen with
> +* these PSUs. Rev3 is also build into the AX1500i, while the AX1600i
> is the first PSU of
> +* this series which has an unique usb hid id. Though, the actual
> device name is part of
> +* the HID message protocol, so it doesn't matter which dongle is
> connected.
> +*/
> + { HID_USB_DEVICE(0x1B1C, 0x1C00) }, /* Corsair Link USB/COM Dongle
> rev1 */
> + {
Re: [PATCH] hwmon: corsair-psu: update supported devices
On Sat, Nov 28, 2020 at 7:35 AM Wilken Gottwalt wrote: > > On Sat, 28 Nov 2020 02:37:38 -0300 > Jonas Malaco wrote: > > > On Thu, Nov 26, 2020 at 8:43 AM Wilken Gottwalt > > wrote: > > > > > > Adds support for another Corsair PSUs series: AX760i, AX860i, AX1200i, > > > AX1500i and AX1600i. The first 3 power supplies are supported through > > > the Corsair Link USB Dongle which is some kind of USB/Serial/TTL > > > converter especially made for the COM ports of these power supplies. > > > There are 3 known revisions of these adapters. The AX1500i power supply > > > has revision 3 built into the case and AX1600i is the only one in that > > > series, which has an unique usb hid id like the RM/RX series. > > > > Can I ask what AXi power supplies were tested? > > > > I ask because, based on the user-space implementations I am aware of, > > the AXi dongle protocol appears to be different from the RMi/HXi series. > > I was not able to test this against the AX power supplies, they are really > hard to find (and are far to expensive). But I went through all these tools > and stuck to the most common commands, which all 3 series support. Not every > series supports all commands (there also seem to be different firmwares in > the micro-conrollers). But this is fine, some sensors will show up as N/A. > Even my HX850i does not support all commands covered in this driver. I think the similarities come from all using wrappers over the PMBus interface to the voltage controller. But I am not sure the wrapping protocols are identical. For example, cpsumon shows significantly more things going on during a read than what is needed for the RMi/HXi series.[1] [1] https://github.com/ka87/cpsumon/blob/fd639684d7f9/libcpsumon/src/cpsumon.c#L213-L231 > > > AXi dongle: > > - https://github.com/ka87/cpsumon > > This tool made me to consider including the AX series, because it uses some > of the same commands on the AX760i, AX860i, AX1200i and AX1500i. But it is > a usb-serial tool only. But it was nice to know, that the commands are mostly > the same. I left out all the commands for configuring, PCIe power rails, > efficiency and others which do not really belong into hwmon. > > > RMi/HXi: > > - https://github.com/jonasmalacofilho/liquidctl > > - https://github.com/audiohacked/OpenCorsairLink > > This tool made me include the AX series, because it uses the rmi protocol > component for the rmi driver (RM/HX series) and the corsair dongles. The corsairlink_driver_dongle has no implementations for reading sensor data (compare that with the corsairlink_driver_rmi).[2][3] There is also no code that actually tries to read (write) from (to) the device using that dongle driver.[4] I also looked at a few of the issues, and all of the ones I read mentioned AXi support being under development, and the hypothesis of the AXi series being compatible with the RMi/HXi code still remaining to be confirmed. [2] https://github.com/audiohacked/OpenCorsairLink/blob/61d336a61b85/drivers/dongle.c#L33-L39 [3] https://github.com/audiohacked/OpenCorsairLink/blob/61d336a61b85/drivers/rmi.c#L33-L57 [4] https://github.com/audiohacked/OpenCorsairLink/blob/61d336a61b85/main.c#L106 > > > - https://github.com/notaz/corsairmi > > This one covers only some HX/RM PSUs, but is uses the rawhid access which > made me looking up the actual usb chips/bridges Corsair uses. > > > > > One additional concern is that the non-HID AXi dongles may only have bulk > > USB endpoints, and this is a HID driver.[1] > > You are right, in the case of the dongles it could be different. But I did > some research on Corsair usb driven devices and they really like to stick to > the cp210x, which is an usb hid bridge. The commit > b9326057a3d8447f5d2e74a7b521ccf21add2ec0 actually covers two Corsair USB > dongles as a cp210x device. So it is very likely that all Corsair PSUs with > such an interface/dongle use usb hid. But I'm completely open to get proven > wrong. Actually I really would like to see this tested by people who have > access to the more rare devices. I could be wrong (and I am sorry for the noise if that is the case), but as far as I can see the cp210x does not create a HID device. Thanks again, Jonas > > > Thanks, > > Jonas > > > > [1] https://github.com/ka87/cpsumon/issues/4 > > Yes ... that one. The last revision of the dongle could indeed be a problem. > But I'm not really sure what is described here. The last commenter is actually > the one who provided the cp210x patch mentioned up there. The problem here is, > the AX1500i has both connectors, USB and that other one. I call it the other > one because it is the only PSU where it
