Message-ID: <1ad78d64-ab6a-41fd-b149-dd52ea08e4e3@linux.dev>
Date: Mon, 21 Jul 2025 10:24:13 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
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 7/21/25 03:42, Andy Shevchenko wrote:
> 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?

Yes! My base assumption would be that includes should be alphabetized,
but that no other ordering or spacing is necessary. Your "common sense"
is not so common.

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

Sorry, I missed this the first time. Anyway the reason it doesn't work
for alarms/ids is that those are members of listener and not separate
variables. And I think it's more concise this way. Compare the existing

	listener->alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
	if (!listener->alarms)
		goto err_listener;

	/* ... */
	return listener;

err_alarms:
	kfree(listener->alarms);
	/* ... */

vs

	unsigned long __free alarms = NULL;

	alarms = bitmap_zalloc(listener->num_alarms, GFP_KERNEL);
	if (!alarms)
		return -ENOMEM;
	listener->alarms = alarms;
	
	/* ... */
	no_free_ptr(alarms);
	return listener;

I don't really think there's an advantage.

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

It's not the standard for refcounts protected by a mutex. There are
literally hundreds of existing examples of this.

--Sean 

Powered by blists - more mailing lists