--BR08w6M/iFoPdHs7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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)

	The _temp_ types are equivalent to _tre_ and _basic_
	(using typedefs).

	For consistency with the documentation and with "sanity",
	I think it would be better if the header file was updated
	to use envsys_tre_data_t and envsys_basic_info_t (and the
	minor typo in the man page fixed)

	Thoughts?


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

	    2.	KASSERT(pointer != NULL)
		(Maybe diagnostic)

	    3.	Do nothing.

	Thoughts?


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


Luke.

--BR08w6M/iFoPdHs7
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (NetBSD)

iD8DBQFDR9NSpBhtmn8zJHIRAmkxAJwPoIxpcKjFRSGgYiaEgaffYX5BQACaA1SJ
gZf6ngpb1amhfQxe6sJFYkw=
=9yHV
-----END PGP SIGNATURE-----

--BR08w6M/iFoPdHs7--