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

[Andy Shevchenko]

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