Re: CVS commit: src/sys/arch/x86/x86

[Jason Thorpe <thorpej%me.com@localhost>]

> On Jun 7, 2018, at 7:59 AM, maya%netbsd.org@localhost wrote:
> 
> You've changed a default and selectively fixed the one driver that
> people noticed breaks from it. How do you know the rest aren't broken?

As you know, there is very little certainty in this world.  For example, how can I be certain that I won’t be hit by a bus when crossing the street? (Pun intended.)  At best, I can take precautions to minimize my risk.

In this case, here are the precautions I have taken:

- I have used a method of probing for i2c devices that is widely considered to be the best approach for doing so in a general way.

- The method I have selected is equivalent to the SMBus “quick” command.

- The overwhelming majority of “intelligent” i2c controllers that provide an “exec” interface to our i2c subsystem are, in fact, specifically SMBus controllers, and thus will, by their nature, support this operation.

- The other general-purpose “intelligent” i2c controllers that provide an “exec” interface should also, at the hardware layer, should support this operation because it’s a completely legal i2c bus sequence that should be free of side-effects.

- Per a lengthy discussion on tech-kern, we know of a specific, odd case of “intelligent” i2c controller that is fairly neutered, which we have special-cased.  It is truly the odd-ball.

- For the non-intelligent i2c controllers that use the i2c subsystem’s bit-bang logic, if this method doesn’t work, it’s simply a software bug that should be fixed.

Of course, you’re also ignoring the fact that i2c configuration was already somewhat broken in -current on a bunch of platforms, due to the effects of fixing another bug.  So, at the very least, this is moving the needle forward after a consensus among various stakeholders that this was a good approach.

In all my years of being involved with NetBSD, I have always made a good faith effort to improve the system to benefit the community, and attempt to correct my mistakes promptly when I make them.  If there’s one thing I’m certain of, it’s that.




> 
> On Thu, Jun 07, 2018 at 01:35:31PM +0000, Jason R Thorpe wrote:
>> Module Name:	src
>> Committed By:	thorpej
>> Date:		Thu Jun  7 13:35:31 UTC 2018
>> 
>> Modified Files:
>> 	src/sys/arch/x86/x86: x86_autoconf.c
>> 
>> Log Message:
>> In device_register(), if the device is an "iic" child of "imcsmb",
>> attach a I2C_PROP_INDIRECT_DEVICE_WHITELIST property that limits
>> the allowed devices to "spdmem" and "sdtemp".  Also set the
>> I2C_PROP_INDIRECT_PROBE_STRATEGY property to I2C_PROBE_STRATEGY_NONE,
>> since that controller can't issue any of the "quick" commands.
>> 
>> XXX It would be nice to be able to do this in the imcsmb driver
>> itself, but the way autoconfiguration works makes that infeasible.
>> 
>> 
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.76 -r1.77 src/sys/arch/x86/x86/x86_autoconf.c
>> 
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>> 
> 
>> Modified files:
>> 
>> Index: src/sys/arch/x86/x86/x86_autoconf.c
>> diff -u src/sys/arch/x86/x86/x86_autoconf.c:1.76 src/sys/arch/x86/x86/x86_autoconf.c:1.77
>> --- src/sys/arch/x86/x86/x86_autoconf.c:1.76	Thu Nov  9 01:02:56 2017
>> +++ src/sys/arch/x86/x86/x86_autoconf.c	Thu Jun  7 13:35:31 2018
>> @@ -1,4 +1,4 @@
>> -/*	$NetBSD: x86_autoconf.c,v 1.76 2017/11/09 01:02:56 christos Exp $	*/
>> +/*	$NetBSD: x86_autoconf.c,v 1.77 2018/06/07 13:35:31 thorpej Exp $	*/
>> 
>> /*-
>>  * Copyright (c) 1990 The Regents of the University of California.
>> @@ -35,7 +35,7 @@
>>  */
>> 
>> #include <sys/cdefs.h>
>> -__KERNEL_RCSID(0, "$NetBSD: x86_autoconf.c,v 1.76 2017/11/09 01:02:56 christos Exp $");
>> +__KERNEL_RCSID(0, "$NetBSD: x86_autoconf.c,v 1.77 2018/06/07 13:35:31 thorpej Exp $");
>> 
>> #include <sys/param.h>
>> #include <sys/systm.h>
>> @@ -54,6 +54,8 @@ __KERNEL_RCSID(0, "$NetBSD: x86_autoconf
>> #include <machine/bootinfo.h>
>> #include <machine/pio.h>
>> 
>> +#include <dev/i2c/i2cvar.h>
>> +
>> #include "acpica.h"
>> #include "wsdisplay.h"
>> 
>> @@ -547,6 +549,36 @@ device_register(device_t dev, void *aux)
>> {
>> 	device_t isaboot, pciboot;
>> 
>> +	/*
>> +	 * The Intel Integrated Memory Controller has a built-in i2c
>> +	 * controller that's rather limited in capability; it is intended
>> +	 * only for reading memory module EERPOMs and sensors.
>> +	 */
>> +	if (device_is_a(dev, "iic") &&
>> +	    device_is_a(dev->dv_parent, "imcsmb")) {
>> +		static const char *imcsmb_device_whitelist[] = {
>> +			"spdmem",
>> +			"sdtemp",
>> +			NULL,
>> +		};
>> +		prop_array_t whitelist = prop_array_create();
>> +		prop_dictionary_t props = device_properties(dev);
>> +		int i;
>> +
>> +		for (i = 0; imcsmb_device_whitelist[i] != NULL; i++) {
>> +			prop_string_t pstr = prop_string_create_cstring_nocopy(
>> +			    imcsmb_device_whitelist[i]);
>> +			(void) prop_array_add(whitelist, pstr);
>> +			prop_object_release(pstr);
>> +		}
>> +		(void) prop_dictionary_set(props,
>> +					   I2C_PROP_INDIRECT_DEVICE_WHITELIST,
>> +					   whitelist);
>> +		(void) prop_dictionary_set_cstring_nocopy(props,
>> +					   I2C_PROP_INDIRECT_PROBE_STRATEGY,
>> +					   I2C_PROBE_STRATEGY_NONE);
>> +	}
>> +
>> 	device_acpi_register(dev, aux);
>> 
>> 	isaboot = device_isa_register(dev, aux);
>> 
> 

-- thorpej