| Message ID | 20240624111519.15652-7-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d | expand |
On Monday 24 June 2024 13:15:18 Hans de Goede wrote: > Unfortunately the SMOxxxx ACPI device does not contain the i2c-address > of the accelerometer. So a DMI product-name to address mapping table > is used. > > At support to have the kernel probe for the i2c-address for modesl > which are not on the list. > > The new probing code sits behind a new probe_i2c_addr module parameter, > which is disabled by default because probing might be dangerous. > > Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > Signed-off-by: Hans de Goede <hdegoede@redhat.com> My comments from the previous version still apply there. There is no dangerous warning neither in parameter name and its description, there is no warning once the parameter was specified. And there is missing information from Dell how it is going to be handled for new machines. So first ask Dell about the current state and future. > --- > drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++- > 1 file changed, 131 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c > index a7409db0505b..173615fd2646 100644 > --- a/drivers/platform/x86/dell/dell-lis3lv02d.c > +++ b/drivers/platform/x86/dell/dell-lis3lv02d.c > @@ -15,6 +15,8 @@ > #include <linux/workqueue.h> > #include "dell-smo8800-ids.h" > > +#define LIS3_WHO_AM_I 0x0f > + > #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \ > { \ > .matches = { \ > @@ -57,6 +59,121 @@ static const struct dmi_system_id *lis3lv02d_dmi_id; > static struct i2c_client *i2c_dev; > static bool notifier_registered; > > +static bool probe_i2c_addr; > +module_param(probe_i2c_addr, bool, 0444); > +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown"); > + > +/* > + * This is the kernel version of the single register device sanity checks from > + * the i2c_safety_check function from lm_sensors sensor-detect script: > + * This is meant to prevent access to 1-register-only devices, > + * which are designed to be accessed with SMBus receive byte and SMBus send > + * byte transactions (i.e. short reads and short writes) and treat SMBus > + * read byte as a real write followed by a read. The device detection > + * routines would write random values to the chip with possibly very nasty > + * results for the hardware. Note that this function won't catch all such > + * chips, as it assumes that reads and writes relate to the same register, > + * but that's the best we can do. > + */ > +static int i2c_safety_check(struct i2c_adapter *adap, u8 addr) > +{ > + union i2c_smbus_data smbus_data; > + int err; > + u8 data; > + > + /* > + * First receive a byte from the chip, and remember it. This > + * also checks if there is a device at the address at all. > + */ > + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, > + I2C_SMBUS_BYTE, &smbus_data); > + if (err < 0) > + return err; > + > + data = smbus_data.byte; > + > + /* > + * Receive a byte again; very likely to be the same for > + * 1-register-only devices. > + */ > + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, > + I2C_SMBUS_BYTE, &smbus_data); > + if (err < 0) > + return err; > + > + if (smbus_data.byte != data) > + return 0; /* Not a 1-register-only device. */ > + > + /* > + * Then try a standard byte read, with a register offset equal to > + * the read byte; for 1-register-only device this should read > + * the same byte value in return. > + */ > + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data, > + I2C_SMBUS_BYTE_DATA, &smbus_data); > + if (err < 0) > + return err; > + > + if (smbus_data.byte != data) > + return 0; /* Not a 1-register-only device. */ > + > + /* > + * Then try a standard byte read, with a slightly different register > + * offset; this should again read the register offset in return. > + */ > + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01, > + I2C_SMBUS_BYTE_DATA, &smbus_data); > + if (err < 0) > + return err; > + > + if (smbus_data.byte != (data ^ 0x01)) > + return 0; /* Not a 1-register-only device. */ > + > + /* > + * Apparently this is a 1-register-only device, restore the original > + * register value and leave it alone. > + */ > + i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data, > + I2C_SMBUS_BYTE, NULL); > + pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr); > + return -ENODEV; > +} > + > +static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr, > + struct i2c_board_info *info) > +{ > + union i2c_smbus_data smbus_data; > + int err; > + > + pr_info("Probing for lis3lv02d on address 0x%02x\n", addr); > + err = i2c_safety_check(adap, addr); > + if (err < 0) > + return err; > + > + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I, > + I2C_SMBUS_BYTE_DATA, &smbus_data); > + if (err < 0) { > + pr_warn("Failed to read who-am-i register: %d\n", err); > + return err; > + } > + > + /* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */ > + switch (smbus_data.byte) { > + case 0x32: > + case 0x33: > + case 0x3a: > + case 0x3b: > + break; > + default: > + pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte); > + return -ENODEV; > + } > + > + pr_debug("Detected lis3lv02d on address 0x%02x\n", addr); > + info->addr = addr; > + return 0; > +} > + > static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) > { > /* > @@ -93,7 +210,17 @@ static void instantiate_i2c_client(struct work_struct *work) > if (!adap) > return; > > - info.addr = (long)lis3lv02d_dmi_id->driver_data; > + if (lis3lv02d_dmi_id) { > + info.addr = (long)lis3lv02d_dmi_id->driver_data; > + } else { > + /* First try address 0x29 (most used) and then try 0x1d */ > + if (detect_lis3lv02d(adap, 0x29, &info) != 0 && > + detect_lis3lv02d(adap, 0x1d, &info) != 0) { > + pr_warn("failed to probe for lis3lv02d I2C address\n"); > + goto out_put_adapter; > + } > + } > + > strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); > > i2c_dev = i2c_new_client_device(adap, &info); > @@ -104,6 +231,7 @@ static void instantiate_i2c_client(struct work_struct *work) > pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr); > } > > +out_put_adapter: > i2c_put_adapter(adap); > } > static DECLARE_WORK(i2c_work, instantiate_i2c_client); > @@ -166,8 +294,9 @@ static int __init dell_lis3lv02d_init(void) > put_device(dev); > > lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices); > - if (!lis3lv02d_dmi_id) { > + if (!lis3lv02d_dmi_id && !probe_i2c_addr) { > pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n"); > + pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n"); > return 0; > } > > -- > 2.45.1 >
Hi Hans, kernel test robot noticed the following build errors: [auto build test ERROR on andi-shyti/i2c/i2c-host] [also build test ERROR on wsa/i2c/for-next linus/master v6.10-rc5 next-20240627] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/i2c-core-Setup-i2c_adapter-runtime-pm-before-calling-device_add/20240626-053449 base: git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240624111519.15652-7-hdegoede%40redhat.com patch subject: [PATCH v4 6/6] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address config: i386-randconfig-002-20240628 (https://download.01.org/0day-ci/archive/20240628/202406280954.PwlEGWfP-lkp@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240628/202406280954.PwlEGWfP-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406280954.PwlEGWfP-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'i2c_safety_check': >> drivers/platform/x86/dell/dell-lis3lv02d.c:88:15: error: implicit declaration of function 'i2c_smbus_xfer' [-Werror=implicit-function-declaration] 88 | err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, | ^~~~~~~~~~~~~~ drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'find_i801': drivers/platform/x86/dell/dell-lis3lv02d.c:197:21: error: implicit declaration of function 'i2c_get_adapter'; did you mean 'i2c_get_adapdata'? [-Werror=implicit-function-declaration] 197 | *adap_ret = i2c_get_adapter(adap->nr); | ^~~~~~~~~~~~~~~ | i2c_get_adapdata drivers/platform/x86/dell/dell-lis3lv02d.c:197:19: warning: assignment to 'struct i2c_adapter *' from 'int' makes pointer from integer without a cast [-Wint-conversion] 197 | *adap_ret = i2c_get_adapter(adap->nr); | ^ drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'instantiate_i2c_client': drivers/platform/x86/dell/dell-lis3lv02d.c:226:19: error: implicit declaration of function 'i2c_new_client_device' [-Werror=implicit-function-declaration] 226 | i2c_dev = i2c_new_client_device(adap, &info); | ^~~~~~~~~~~~~~~~~~~~~ drivers/platform/x86/dell/dell-lis3lv02d.c:226:17: warning: assignment to 'struct i2c_client *' from 'int' makes pointer from integer without a cast [-Wint-conversion] 226 | i2c_dev = i2c_new_client_device(adap, &info); | ^ drivers/platform/x86/dell/dell-lis3lv02d.c:235:9: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration] 235 | i2c_put_adapter(adap); | ^~~~~~~~~~~~~~~ drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'dell_lis3lv02d_module_exit': drivers/platform/x86/dell/dell-lis3lv02d.c:325:9: error: implicit declaration of function 'i2c_unregister_device' [-Werror=implicit-function-declaration] 325 | i2c_unregister_device(i2c_dev); | ^~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/i2c_smbus_xfer +88 drivers/platform/x86/dell/dell-lis3lv02d.c 65 66 /* 67 * This is the kernel version of the single register device sanity checks from 68 * the i2c_safety_check function from lm_sensors sensor-detect script: 69 * This is meant to prevent access to 1-register-only devices, 70 * which are designed to be accessed with SMBus receive byte and SMBus send 71 * byte transactions (i.e. short reads and short writes) and treat SMBus 72 * read byte as a real write followed by a read. The device detection 73 * routines would write random values to the chip with possibly very nasty 74 * results for the hardware. Note that this function won't catch all such 75 * chips, as it assumes that reads and writes relate to the same register, 76 * but that's the best we can do. 77 */ 78 static int i2c_safety_check(struct i2c_adapter *adap, u8 addr) 79 { 80 union i2c_smbus_data smbus_data; 81 int err; 82 u8 data; 83 84 /* 85 * First receive a byte from the chip, and remember it. This 86 * also checks if there is a device at the address at all. 87 */ > 88 err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, 89 I2C_SMBUS_BYTE, &smbus_data); 90 if (err < 0) 91 return err; 92 93 data = smbus_data.byte; 94 95 /* 96 * Receive a byte again; very likely to be the same for 97 * 1-register-only devices. 98 */ 99 err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, 100 I2C_SMBUS_BYTE, &smbus_data); 101 if (err < 0) 102 return err; 103 104 if (smbus_data.byte != data) 105 return 0; /* Not a 1-register-only device. */ 106 107 /* 108 * Then try a standard byte read, with a register offset equal to 109 * the read byte; for 1-register-only device this should read 110 * the same byte value in return. 111 */ 112 err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data, 113 I2C_SMBUS_BYTE_DATA, &smbus_data); 114 if (err < 0) 115 return err; 116 117 if (smbus_data.byte != data) 118 return 0; /* Not a 1-register-only device. */ 119 120 /* 121 * Then try a standard byte read, with a slightly different register 122 * offset; this should again read the register offset in return. 123 */ 124 err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01, 125 I2C_SMBUS_BYTE_DATA, &smbus_data); 126 if (err < 0) 127 return err; 128 129 if (smbus_data.byte != (data ^ 0x01)) 130 return 0; /* Not a 1-register-only device. */ 131 132 /* 133 * Apparently this is a 1-register-only device, restore the original 134 * register value and leave it alone. 135 */ 136 i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data, 137 I2C_SMBUS_BYTE, NULL); 138 pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr); 139 return -ENODEV; 140 } 141
diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c index a7409db0505b..173615fd2646 100644 --- a/drivers/platform/x86/dell/dell-lis3lv02d.c +++ b/drivers/platform/x86/dell/dell-lis3lv02d.c @@ -15,6 +15,8 @@ #include <linux/workqueue.h> #include "dell-smo8800-ids.h" +#define LIS3_WHO_AM_I 0x0f + #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \ { \ .matches = { \ @@ -57,6 +59,121 @@ static const struct dmi_system_id *lis3lv02d_dmi_id; static struct i2c_client *i2c_dev; static bool notifier_registered; +static bool probe_i2c_addr; +module_param(probe_i2c_addr, bool, 0444); +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown"); + +/* + * This is the kernel version of the single register device sanity checks from + * the i2c_safety_check function from lm_sensors sensor-detect script: + * This is meant to prevent access to 1-register-only devices, + * which are designed to be accessed with SMBus receive byte and SMBus send + * byte transactions (i.e. short reads and short writes) and treat SMBus + * read byte as a real write followed by a read. The device detection + * routines would write random values to the chip with possibly very nasty + * results for the hardware. Note that this function won't catch all such + * chips, as it assumes that reads and writes relate to the same register, + * but that's the best we can do. + */ +static int i2c_safety_check(struct i2c_adapter *adap, u8 addr) +{ + union i2c_smbus_data smbus_data; + int err; + u8 data; + + /* + * First receive a byte from the chip, and remember it. This + * also checks if there is a device at the address at all. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE, &smbus_data); + if (err < 0) + return err; + + data = smbus_data.byte; + + /* + * Receive a byte again; very likely to be the same for + * 1-register-only devices. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE, &smbus_data); + if (err < 0) + return err; + + if (smbus_data.byte != data) + return 0; /* Not a 1-register-only device. */ + + /* + * Then try a standard byte read, with a register offset equal to + * the read byte; for 1-register-only device this should read + * the same byte value in return. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data, + I2C_SMBUS_BYTE_DATA, &smbus_data); + if (err < 0) + return err; + + if (smbus_data.byte != data) + return 0; /* Not a 1-register-only device. */ + + /* + * Then try a standard byte read, with a slightly different register + * offset; this should again read the register offset in return. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01, + I2C_SMBUS_BYTE_DATA, &smbus_data); + if (err < 0) + return err; + + if (smbus_data.byte != (data ^ 0x01)) + return 0; /* Not a 1-register-only device. */ + + /* + * Apparently this is a 1-register-only device, restore the original + * register value and leave it alone. + */ + i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data, + I2C_SMBUS_BYTE, NULL); + pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr); + return -ENODEV; +} + +static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr, + struct i2c_board_info *info) +{ + union i2c_smbus_data smbus_data; + int err; + + pr_info("Probing for lis3lv02d on address 0x%02x\n", addr); + err = i2c_safety_check(adap, addr); + if (err < 0) + return err; + + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I, + I2C_SMBUS_BYTE_DATA, &smbus_data); + if (err < 0) { + pr_warn("Failed to read who-am-i register: %d\n", err); + return err; + } + + /* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */ + switch (smbus_data.byte) { + case 0x32: + case 0x33: + case 0x3a: + case 0x3b: + break; + default: + pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte); + return -ENODEV; + } + + pr_debug("Detected lis3lv02d on address 0x%02x\n", addr); + info->addr = addr; + return 0; +} + static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) { /* @@ -93,7 +210,17 @@ static void instantiate_i2c_client(struct work_struct *work) if (!adap) return; - info.addr = (long)lis3lv02d_dmi_id->driver_data; + if (lis3lv02d_dmi_id) { + info.addr = (long)lis3lv02d_dmi_id->driver_data; + } else { + /* First try address 0x29 (most used) and then try 0x1d */ + if (detect_lis3lv02d(adap, 0x29, &info) != 0 && + detect_lis3lv02d(adap, 0x1d, &info) != 0) { + pr_warn("failed to probe for lis3lv02d I2C address\n"); + goto out_put_adapter; + } + } + strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); i2c_dev = i2c_new_client_device(adap, &info); @@ -104,6 +231,7 @@ static void instantiate_i2c_client(struct work_struct *work) pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr); } +out_put_adapter: i2c_put_adapter(adap); } static DECLARE_WORK(i2c_work, instantiate_i2c_client); @@ -166,8 +294,9 @@ static int __init dell_lis3lv02d_init(void) put_device(dev); lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices); - if (!lis3lv02d_dmi_id) { + if (!lis3lv02d_dmi_id && !probe_i2c_addr) { pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n"); + pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n"); return 0; }
Unfortunately the SMOxxxx ACPI device does not contain the i2c-address of the accelerometer. So a DMI product-name to address mapping table is used. At support to have the kernel probe for the i2c-address for modesl which are not on the list. The new probing code sits behind a new probe_i2c_addr module parameter, which is disabled by default because probing might be dangerous. Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++- 1 file changed, 131 insertions(+), 2 deletions(-)