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