Subject: Re: envsys(4) questions and a proposed extension.
To: Luke Mewburn <lukem@NetBSD.org>
From: Bill Squier <groo@old-ones.com>
List: tech-kern
Date: 11/02/2005 17:45:43
Executive summary: I agree with all of Luke's proposed changes.
On Oct 8, 2005, at 10:10 AM, Luke Mewburn wrote:
> Hi all:
>
> I've been working with envsys(4) recently - both as a userland
> consumer and a kernel provider, and I have some questions about
> it as well as a proposed extension.
>
> Questions:
>
> (a) The envsys ioctls are documented in envsys(4) as:
> ENVSYS_GTREDATA (envsys_tre_data)
> ENVSYS_STREINFO (envsys_basic_info_t)
> ENVSYS_GTREINFO (envsys_basic_info_t)
>
> Yet the implementation in <sys/envsys.h> has:
> ENVSYS_GTREDATA (envsys_temp_data_t)
> ENVSYS_STREINFO (envsys_temp_info_t)
> ENVSYS_GTREINFO (envsys_temp_info_t)
When and why did this change? It seems odd considering from its very
inception, envsys(4) supported at least 3 types of sensors.
envsys_temp_info_t is at the very least misleading.
> (b) The kernel currently assumes that that the sme_gtredata
> and sme_streinfo members of 'struct sysmon_envsys' are
> not NULL pointers. So, if a kernel provider doesn't
> provide these we get a panic deref-ing NULL.
> (Look for these members in sys/dev/sysmon/sysmon_envsys.c)
>
> I think that this is suboptimal, and see the following solutions:
>
> 1. Check that the function pointer isn't NULL before
> using. If NULL, return EINVAL (?) from the ioctl.
> This is my preferred solution, from a "don't panic"
> PoV, especially since we can have envsys providers
> added by LKMs.
I prefer this, and absolutely back something being done. That would
be a nasty trap to leave behind.
> (c) I have a need to change a sensor (e.g, change the alarm
> state, turn on/off a sensor, etc), and I've seen other
> comments where people need to change the thresholds
> of a fan. The ENVSYS_STREINFO ioctl isn't really
> sufficient; I'm currently (ab)using the `rpms' member
> of envsys_basic_info_t but that's an ugly hack.
>
> I propose adding a new ioctl
> ENVSYS_STREDATA (envsys_tre_data_t)
> and add support in various envsys providers for this if
> appropriate (via a new sme_stredata function pointer).
>
> This ioctl could allow the cur, min, max, and avg members
> to be modified if the appropriate ENVSYS_xxxVALID flag is
> set in the struct passed into the ioctl.
> For each member changed the appropriate bit would be set
> on the return of ioctl.
>
> Thoughts on the idea?
>
> I think that this should be optional and so the kernel wouldn't
> panic if a driver decided not to implement the new sme_stredata
> function pointer, a la (b)(1) above.
>
> Would we need to bump either the kernel's version or the
> envsys API version if we added ENVSYS_STREDATA ?
If we have an API version (I honestly can't remember if we did in the
beginning), then wouldn't just an API bump be sufficient?
-wps