linux-kernel - Re: [PATCH 7/7] hwmon: iio: Add alarm support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aHd6NEHcCa6aqJB5@smile.fi.intel.com>
Date: Wed, 16 Jul 2025 13:08:52 +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 Tue, Jul 15, 2025 at 12:20:24PM -0400, Sean Anderson wrote:
> On 7/15/25 04:50, Andy Shevchenko wrote:
> > On Mon, Jul 14, 2025 at 09:20:23PM -0400, Sean Anderson wrote:

...

> >>  #include <linux/hwmon-sysfs.h>
> > 
> > + blank line here..
> 
> why?

To group the subsystem related headers (which are more custom and less generic).
This allows to follow what the subsystems are in use and what APIs / types are
taken.

> >>  #include <linux/iio/consumer.h>
> >> +#include <linux/iio/events.h>
> >> +#include <linux/iio/iio.h>
> >>  #include <linux/iio/types.h>
> > 
> > ...and here?
> 
> OK
> 
> >> +#include <uapi/linux/iio/events.h>

As similar here, to visually split uAPI and the rest. This increases
readability and maintenance.

...

> >> +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).
> 
> I suppose. But I think
> 
> alarm = iio_hwmon_lookup_alarm(...);
> if (alarm < 0)
> 	return -ENOENT;
> 
> is clearer than

I disagree. This makes it worth as it shadows other possible code(s), if any,
and makes harder to follow as reader has to check the callee implementation.

The shadow error codes need a justification.

> alarm = iio_hwmon_lookup_alarm(...);
> if (alarm < 0)
> 	return alarm;
> 
> because you don't have to read the definition of iio_hwmon_lookup_alarm
> to determine what the return value is.

Exactly my point!

> >> +}

...

> >> +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()?
> 
> That works for listener, but not for alarms or ids.

Why not?

...

> >> +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?
> 
> Why? We do all the manipulation under a mutex, so there is no point in
> atomic access. Instead of the games refcnt_t has to play to try and
> prevent overflow we can just check for it directly.

refcount_t provides a facility of overflow/underflow. Also it gives better
understanding from the data type to see which value and how does that.

> >> +		list_del(&listener->list);
> >> +		iio_event_unregister(listener->indio_dev, &listener->block);
> >> +	}
> >> +
> >> +	kfree(listener->alarms);
> >> +	kfree(listener->ids);
> >> +	kfree(listener);
> >> +}

...

> >> +	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.
> 
> :l
> 
> the things we do to avoid memcpy...

...for the cost of readability. Also this is a slow path.

-- 
With Best Regards,
Andy Shevchenko