| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aHYWQOjJEWdLjy7H@smile.fi.intel.com>
Date: Tue, 15 Jul 2025 11:50:08 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: Jonathan Cameron <jic23@...nel.org>, Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>, linux-iio@...r.kernel.org,
linux-hwmon@...r.kernel.org, Andy Shevchenko <andy@...nel.org>,
Nuno Sá <nuno.sa@...log.com>,
linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH 7/7] hwmon: iio: Add alarm support
On Mon, Jul 14, 2025 at 09:20:23PM -0400, Sean Anderson wrote:
> Add alarm support based on IIO threshold events. The alarm is cleared on
> read, but will be set again if the condition is still present. This is
> detected by disabling and re-enabling the event. The same trick is done
> when creating the attribute to detect already-triggered events.
>
> The alarms are updated by an event listener. To keep the notifier call
> chain short, we create one listener per iio device, shared across all
> hwmon devices.
>
> To avoid dynamic creation of alarms, alarms for all possible events are
> allocated at creation. Lookup is done by a linear scan, as I expect
> events to occur rarely. If performance becomes an issue, a binary search
> could be done instead (or some kind of hash lookup).
...
> #include <linux/hwmon-sysfs.h>
+ blank line here..
> #include <linux/iio/consumer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> #include <linux/iio/types.h>
...and here?
> +#include <uapi/linux/iio/events.h>
...
> +static ssize_t iio_hwmon_lookup_alarm(struct iio_hwmon_listener *listener,
> + u64 id)
> +{
> + ssize_t i;
> +
> + for (i = 0; i < listener->num_alarms; i++)
> + if (listener->ids[i] == id)
> + return i;
> + return -1;
-ENOENT ?
This will allow to propagate an error code to the upper layer(s).
> +}
...
> +static int iio_hwmon_listener_callback(struct notifier_block *block,
> + unsigned long action, void *data)
> +{
> + struct iio_hwmon_listener *listener =
> + container_of(block, struct iio_hwmon_listener, block);
> + struct iio_event_data *ev = data;
> + ssize_t i;
> +
> + if (action != IIO_NOTIFY_EVENT)
> + return NOTIFY_DONE;
> +
> + i = iio_hwmon_lookup_alarm(listener, ev->id);
> + if (i >= 0)
> + set_bit(i, listener->alarms);
Do you need an atomic set?
> + else
> + dev_warn_once(&listener->indio_dev->dev,
> + "unknown event %016llx\n", ev->id);
> +
> + return NOTIFY_DONE;
> +}
...
> +static struct iio_hwmon_listener *iio_hwmon_listener_get(struct iio_dev *indio_dev)
> +{
> + struct iio_hwmon_listener *listener;
> + int err = -ENOMEM;
> + size_t i, j;
> +
> + guard(mutex)(&iio_hwmon_listener_lock);
> + list_for_each_entry(listener, &iio_hwmon_listeners, list) {
> + if (listener->indio_dev == indio_dev) {
> + if (likely(listener->refcnt != UINT_MAX))
> + listener->refcnt++;
> + return listener;
> + }
> + }
> +
> + listener = kzalloc(sizeof(*listener), GFP_KERNEL);
> + if (!listener)
> + goto err_unlock;
> +
> + listener->refcnt = 1;
> + listener->indio_dev = indio_dev;
> + listener->block.notifier_call = iio_hwmon_listener_callback;
> + for (i = 0; i < indio_dev->num_channels; i++)
> + listener->num_alarms += indio_dev->channels[i].num_event_specs;
> +
> + listener->ids = kcalloc(listener->num_alarms, sizeof(*listener->ids),
> + GFP_KERNEL);
> + listener->alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
> + if (!listener->ids || !listener->alarms)
> + goto err_listener;
> +
> + i = 0;
> + for (j = 0; j < indio_dev->num_channels; j++) {
> + struct iio_chan_spec const *chan = &indio_dev->channels[j];
> + size_t k;
> +
> + for (k = 0; k < chan->num_event_specs; k++)
> + listener->ids[i++] =
> + iio_event_id(chan, chan->event_spec[k].type,
> + chan->event_spec[k].dir);
> + }
> +
> + err = iio_event_register(indio_dev, &listener->block);
> + if (err)
> + goto err_alarms;
> +
> + list_add(&listener->list, &iio_hwmon_listeners);
> + mutex_unlock(&iio_hwmon_listener_lock);
With guard() ???
> + return listener;
> +
> +err_alarms:
> + kfree(listener->alarms);
> + kfree(listener->ids);
> +err_listener:
> + kfree(listener);
> +err_unlock:
> + mutex_unlock(&iio_hwmon_listener_lock);
> + return ERR_PTR(err);
What about using __free()?
> +}
...
> +static void iio_hwmon_listener_put(void *data)
> +{
> + struct iio_hwmon_listener *listener = data;
> +
> + scoped_guard(mutex, &iio_hwmon_listener_lock) {
> + if (unlikely(listener->refcnt == UINT_MAX))
> + return;
> +
> + if (--listener->refcnt)
> + return;
Can the refcount_t be used with the respective APIs? Or even kref?
> + list_del(&listener->list);
> + iio_event_unregister(listener->indio_dev, &listener->block);
> + }
> +
> + kfree(listener->alarms);
> + kfree(listener->ids);
> + kfree(listener);
> +}
...
> +static ssize_t iio_hwmon_read_alarm(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_hwmon_alarm_attribute *sattr = to_alarm_attr(attr);
> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
> + struct iio_channel *chan = &state->channels[sattr->index];
> +
> + if (test_and_clear_bit(sattr->alarm, sattr->listener->alarms)) {
> + u64 id = sattr->listener->ids[sattr->alarm];
> + enum iio_event_direction dir = IIO_EVENT_CODE_EXTRACT_DIR(id);
> +
> + WARN_ON(iio_hwmon_alarm_toggle(chan, dir));
> + strcpy(buf, "1\n");
> + return 2;
> + }
> +
> + strcpy(buf, "0\n");
> + return 2;
Better to assign the value and
return sysfs_emit(...);
which will make even easier to recognize that this is supplied to user via
sysfs.
> +}
...
> +static int add_alarm_attr(struct device *dev, struct iio_hwmon_state *st,
> + int i, enum iio_event_direction dir,
> + const char *fmt, ...)
Same comments as per previous patches.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists