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

[Sean Anderson <sean.anderson@linux.dev>]

On 7/16/25 06:08, Andy Shevchenko wrote:
> 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.

Then you should send a patch for coding-style.rst.

>> >>  #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.

OK, I will return a bool next time to avoid any misconceptions that the return
code means anything other than "found" or "not found"

>> 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!

your point is that you want readers to have to read the definition of
iio_hwmon_lookup_alarm in order to determine that ENOENT is a possible
error from add_alarm_attr? I don't follow.

>> >> +}
> 
> ...
> 
>> >> +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.

refcount_t can't prevent underflow because it's atomic. All it can do is
warn after the fact. And of course overflow is handled properly here.
But it can't occur in practice unless you specifically load multiple
devicetrees at runtime. So we don't need it anyway.

> Also it gives better
> understanding from the data type to see which value and how does that.

That's why I named the variable "refcnt".

>> >> +		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.