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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jul 17, 2025 at 12:23:58PM -0400, Sean Anderson wrote:
> 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.

Does any of the common sense approach need to be written in the documentation?

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

This makes sense. And IIRC it's even documented.

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

No, my point is that readers should not care about error code. If it's
propagated to the upper layer, the upper layer will decide on how to proceed.
And -ENOENT is de facto standard for "entity not found".

> >> >> +}

...

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

No answer? Have you checked how cleanup.h suggests to avoid cleaning the memory
when it's supposed to be used later on?

...

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

It will warn the user in such cases. Your code won't do it, even if it's not a
big deal or never happens situation, it's still better to use in-kernel
standard ways of handling these things.

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

Yes, and that's why I asked about existing interface / API / type to use.

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

-- 
With Best Regards,
Andy Shevchenko