| Message ID | 20240106160935.45487-3-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
+ Cc people from tags of 72921427d46b ("string.h: Add str_has_prefix() helper
function"). See below why.
On Sat, Jan 06, 2024 at 05:09:29PM +0100, Hans de Goede wrote:
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource
> inside the generic i801 I2C adapter driver.
>
> The kernel already instantiates platform_device-s for these ACPI devices
> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> platform drivers.
>
> Move the i2c_client instantiation from the generic i2c-i801 driver to
> the Dell specific dell-smo8800 driver.
...
> +/*
> + * NOTE: If new models with FEATURE_IDF are added please also update
> + * i801_idf_ids[] in drivers/platform/x86/dell-smo8800.c
> + */
> static const struct pci_device_id i801_ids[] = {
I believe this comment can be enforced with some defines / static_assert().
But I haven't given any thought about it, just an idea.
> };
...
> + * Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
2024?
...
> + if (!strstarts(adap->name, "SMBus I801 adapter"))
> + return 0;
Bah, we have str_has_prefix() and this, much older one...
Steven? Others? I mean we can do something about this duplication, right?
On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > +/* > + * Accelerometer's I2C address is not specified in DMI nor ACPI, > + * so it is needed to define mapping table based on DMI product names. > + */ > +static const struct { > + const char *dmi_product_name; > + unsigned short i2c_addr; > +} dell_lis3lv02d_devices[] = { > + /* > + * Dell platform team told us that these Latitude devices have > + * ST microelectronics accelerometer at I2C address 0x29. > + */ > + { "Latitude E5250", 0x29 }, > + { "Latitude E5450", 0x29 }, > + { "Latitude E5550", 0x29 }, > + { "Latitude E6440", 0x29 }, > + { "Latitude E6440 ATG", 0x29 }, > + { "Latitude E6540", 0x29 }, > + /* > + * Additional individual entries were added after verification. > + */ > + { "Latitude 5480", 0x29 }, > + { "Vostro V131", 0x1d }, > + { "Vostro 5568", 0x29 }, > +}; > + > +static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) > +{ > + struct i2c_board_info info = { }; > + struct i2c_adapter *adap = NULL; > + const char *dmi_product_name; > + u8 addr = 0; > + int i; > + > + bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801); > + if (!adap) > + return; > + > + dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); > + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { Before doing this array iteration it is needed to check for Dell vendor like it was before: if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) return false; Or put vendor name into the devices list and check for it (in case you want to extend list also for non-Dell machines). > + if (strcmp(dmi_product_name, > + dell_lis3lv02d_devices[i].dmi_product_name) == 0) { > + addr = dell_lis3lv02d_devices[i].i2c_addr; > + break; > + } > + } > + > + if (!addr) { > + dev_warn(smo8800->dev, > + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); > + goto put_adapter; > + } > + > + info.addr = addr; > + strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); > + > + smo8800->i2c_dev = i2c_new_client_device(adap, &info); > + if (IS_ERR(smo8800->i2c_dev)) { > + dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev), > + "registering accel i2c_client\n"); > + smo8800->i2c_dev = NULL; > + } else { > + dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", > + info.type, info.addr); > + } > +put_adapter: > + i2c_put_adapter(adap); > +} > + > static int smo8800_probe(struct platform_device *device) > { > int err; > @@ -126,10 +237,12 @@ static int smo8800_probe(struct platform_device *device) > return err; > smo8800->irq = err; > > + smo8800_instantiate_i2c_client(smo8800); Now after looking at this change again I see there a problem. If i2c-801 driver initialize i2c-801 device after this smo8800 is called then accelerometer i2c device would not happen. Also it has same problem if PCI i801 device is reloaded or reset. With the current approach this was not an issue as during i801 initialization was smo i2c device automatically created and lis driver was able to bind and initialize it at any time. Before parent driver created its own direct children devices. After this change, the child driver is trying to find who is the parent of its device and injects its device to the parent in the device tree hierarchy. > + > err = misc_register(&smo8800->miscdev); > if (err) { > dev_err(&device->dev, "failed to register misc dev: %d\n", err); > - return err; > + goto error_unregister_i2c_client; > } > > err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > @@ -150,6 +263,8 @@ static int smo8800_probe(struct platform_device *device) > > error: > misc_deregister(&smo8800->miscdev); > +error_unregister_i2c_client: > + i2c_unregister_device(smo8800->i2c_dev); > return err; > } > > @@ -160,9 +275,9 @@ static void smo8800_remove(struct platform_device *device) > free_irq(smo8800->irq, smo8800); > misc_deregister(&smo8800->miscdev); > dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > + i2c_unregister_device(smo8800->i2c_dev); > } > > -/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */ > static const struct acpi_device_id smo8800_ids[] = { > { "SMO8800", 0 }, > { "SMO8801", 0 }, > @@ -189,3 +304,5 @@ module_platform_driver(smo8800_driver); > MODULE_DESCRIPTION("Dell Latitude freefall driver (ACPI SMO88XX)"); > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Sonal Santan, Pali Rohár"); > +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */ > +MODULE_SOFTDEP("pre: i2c-i801"); > -- > 2.43.0 >
On Sat, 2024-01-06 at 18:24 +0200, Andy Shevchenko wrote: > + Cc people from tags of 72921427d46b ("string.h: Add str_has_prefix() helper > function"). See below why. > > + if (!strstarts(adap->name, "SMBus I801 adapter")) > > + return 0; > > Bah, we have str_has_prefix() and this, much older one... > Steven? Others? I mean we can do something about this duplication, right? coccinelle? @@ expression a, b; @@ - strstarts(a, b) + str_has_prefix(a, b)
On Sat, 6 Jan 2024 18:24:34 +0200 Andy Shevchenko <andy@kernel.org> wrote: > > + if (!strstarts(adap->name, "SMBus I801 adapter")) > > + return 0; > > Bah, we have str_has_prefix() and this, much older one... > Steven? Others? I mean we can do something about this duplication, right? They are not really duplicate functions. Note that strstarts() is just a boolean (does this start with something) where as str_has_prefix() returns the length of the prefix. Yes, strstarts() can be swapped to str_has_prefix() but it can't go the other way around. One use case of the str_has_prefix() feature is in the histogram parsing: for (i = 0; i < hist_data->attrs->n_actions; i++) { str = hist_data->attrs->action_str[i]; if ((len = str_has_prefix(str, "onmatch("))) { char *action_str = str + len; data = onmatch_parse(tr, action_str); if (IS_ERR(data)) { ret = PTR_ERR(data); break; } } else if ((len = str_has_prefix(str, "onmax("))) { char *action_str = str + len; data = track_data_parse(hist_data, action_str, HANDLER_ONMAX); if (IS_ERR(data)) { ret = PTR_ERR(data); break; } } else if ((len = str_has_prefix(str, "onchange("))) { char *action_str = str + len; data = track_data_parse(hist_data, action_str, HANDLER_ONCHANGE); if (IS_ERR(data)) { ret = PTR_ERR(data); break; } Where we get the length of the prefix if there's a match, and use that to skip over the prefix. If you just need to know if something starts with a string, then "strstarts()" is perfectly fine to use. -- Steve
On Sun, 7 Jan 2024 11:09:17 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > for (i = 0; i < hist_data->attrs->n_actions; i++) { > str = hist_data->attrs->action_str[i]; > > if ((len = str_has_prefix(str, "onmatch("))) { > char *action_str = str + len; > > data = onmatch_parse(tr, action_str); > if (IS_ERR(data)) { > ret = PTR_ERR(data); > break; > } > } else if ((len = str_has_prefix(str, "onmax("))) { > char *action_str = str + len; > > data = track_data_parse(hist_data, action_str, > HANDLER_ONMAX); > if (IS_ERR(data)) { > ret = PTR_ERR(data); > break; > } > } else if ((len = str_has_prefix(str, "onchange("))) { > char *action_str = str + len; > > data = track_data_parse(hist_data, action_str, > HANDLER_ONCHANGE); > if (IS_ERR(data)) { > ret = PTR_ERR(data); > break; > } And this could possibly be consolidated to: for (i = 0; i < hist_data->attrs->n_actions; i++) { char *action_str; enum handler_id hid; ret = -EINVAL; str = hist_data->attrs->action_str[i]; if ((len = str_has_prefix(str, "onmatch("))) hid = HANDLER_ONMATCH; else if ((len = str_has_prefix(str, "onmax("))) hid = HANDLER_ONMAX; else if ((len = str_has_prefix(str, "onchange("))) hid = HANDLER_ONCHANGE; else break; action_str = str + len; switch (hid) { case HANDLER_ONMATCH: data = onmatch_parse(tr, action_str); break; default: data = track_data_parse(hist_data, action_str, hid); } if (IS_ERR(data)) { ret = PTR_ERR(data); break; } hist_data->actions[hist_data->n_actions++] = data; ret = 0; } I think I'll go make that change! -- Steve
On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > It is not necessary to handle the Dell specific instantiation of > i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource > inside the generic i801 I2C adapter driver. > > The kernel already instantiates platform_device-s for these ACPI devices > and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these > platform drivers. > > Move the i2c_client instantiation from the generic i2c-i801 driver to > the Dell specific dell-smo8800 driver. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses > - Add a comment documenting the IDF PCI device ids > --- > drivers/i2c/busses/i2c-i801.c | 126 +---------------------- > drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++- > 2 files changed, 123 insertions(+), 124 deletions(-) I'm looking at this change again and I'm not not sure if it is a good direction to do this movement. Some of the issues which this change introduces I described in the previous email. I somehow have not caught up why to do it. ACPI smo8800 device and i2c lis3lv02d are from the OS resource point of view totally different devices, not connected together in any way. In ACPI tables there is no link information that smo8800 belongs to lis3lv02d, and neither that it is on i2c. System tree of the devices structures also handle it in this way. If I'm looking at the current device design, it is a bus who instantiate devices (as children of the bus). In this case, this i2c has no information what is there connected (no Device Tree, no ACPI), so only static data hardcoded in kernel are available. And therefore it should be the bus who create or delete devices. If the whole idea of this patch series was to merge smo8800 device and lis3lv02d device into one device, the question is why to do it and why it is a good idea in this case? (Specially when firmware provide to use very little information). I just do not see this motivation. If it is going to fix some bug or required for some new feature or something else. I would like to know this one. Maybe I'm completely something missing and hence I'm wrong... I know that it is just a one device which provides interrupt and accelerometer axes, but these two things are from OS persepctive totally separated and there can be all combinations which of them are available and which not (BIOS has select option to disable ACPI device=interrupt, can be ON/OFF and kernel has or does not have DMI information of i2c bus for acelerometer axes). I can understand motivation to replace one i2c driver by another, if there is a new style of it. In this it is just needed to teach new iio lis driver to support some joystick emulation layer (can be generic, or only for lis, or only available for HP and Dell machines) and switch can be done without issue. I can also understand motivation that freefall code is in two drivers (old i2c lis driver and acpi smo8800). In this case it can be extracted somwhere into helper function, or maybe completely moves into platform/x86 as it is IIRC only for HP and Dell machines, and can ripped out from the old i2c lis driver (unless there is some other usage for it). I also know that it is not a clean to have some Dell DMI data list in the i801 bus driver and helper code not related to i801. So why not to move this code from i2c-i801.c source file to some other helper library and call just the helper function from i801. I would rather let i2c lis device and ACPI smo8800 device separated, this concept is less prone to errors, matches linux device model and is already in use for many years and verified that works fine.
Hi Hans,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.7]
[cannot apply to wsa/i2c/for-next next-20240112]
[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/platform-x86-dell-smo8800-Change-probe-ordering-a-bit/20240107-001715
base: linus/master
patch link: https://lore.kernel.org/r/20240106160935.45487-3-hdegoede%40redhat.com
patch subject: [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
config: x86_64-buildonly-randconfig-001-20240113 (https://download.01.org/0day-ci/archive/20240113/202401131227.HL4y41DY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131227.HL4y41DY-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/202401131227.HL4y41DY-lkp@intel.com/
All warnings (new ones prefixed by >>):
In function 'smo8800_instantiate_i2c_client',
inlined from 'smo8800_probe' at drivers/platform/x86/dell/dell-smo8800.c:240:2:
>> drivers/platform/x86/dell/dell-smo8800.c:188:21: warning: argument 1 null where non-null expected [-Wnonnull]
188 | if (strcmp(dmi_product_name,
| ^~~~~~~~~~~~~~~~~~~~~~~~
189 | dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/bitmap.h:12,
from include/linux/cpumask.h:12,
from include/linux/smp.h:13,
from include/linux/lockdep.h:14,
from include/linux/mutex.h:17,
from include/linux/kernfs.h:11,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/device/bus.h:17,
from drivers/platform/x86/dell/dell-smo8800.c:14:
include/linux/string.h: In function 'smo8800_probe':
include/linux/string.h:89:12: note: in a call to function 'strcmp' declared 'nonnull'
89 | extern int strcmp(const char *,const char *);
| ^~~~~~
vim +188 drivers/platform/x86/dell/dell-smo8800.c
173
174 static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
175 {
176 struct i2c_board_info info = { };
177 struct i2c_adapter *adap = NULL;
178 const char *dmi_product_name;
179 u8 addr = 0;
180 int i;
181
182 bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
183 if (!adap)
184 return;
185
186 dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
187 for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
> 188 if (strcmp(dmi_product_name,
189 dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
190 addr = dell_lis3lv02d_devices[i].i2c_addr;
191 break;
192 }
193 }
194
195 if (!addr) {
196 dev_warn(smo8800->dev,
197 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
198 goto put_adapter;
199 }
200
201 info.addr = addr;
202 strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
203
204 smo8800->i2c_dev = i2c_new_client_device(adap, &info);
205 if (IS_ERR(smo8800->i2c_dev)) {
206 dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev),
207 "registering accel i2c_client\n");
208 smo8800->i2c_dev = NULL;
209 } else {
210 dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
211 info.type, info.addr);
212 }
213 put_adapter:
214 i2c_put_adapter(adap);
215 }
216
Hi Hans, kernel test robot noticed the following build errors: [auto build test ERROR on linus/master] [also build test ERROR on v6.7] [cannot apply to wsa/i2c/for-next next-20240112] [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/platform-x86-dell-smo8800-Change-probe-ordering-a-bit/20240107-001715 base: linus/master patch link: https://lore.kernel.org/r/20240106160935.45487-3-hdegoede%40redhat.com patch subject: [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 config: i386-buildonly-randconfig-005-20240113 (https://download.01.org/0day-ci/archive/20240113/202401131552.PbjGXHjA-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131552.PbjGXHjA-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/202401131552.PbjGXHjA-lkp@intel.com/ All error/warnings (new ones prefixed by >>): drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_find_i801': >> drivers/platform/x86/dell/dell-smo8800.c:144:21: error: implicit declaration of function 'i2c_get_adapter'; did you mean 'i2c_get_adapdata'? [-Werror=implicit-function-declaration] 144 | *adap_ret = i2c_get_adapter(adap->nr); | ^~~~~~~~~~~~~~~ | i2c_get_adapdata >> drivers/platform/x86/dell/dell-smo8800.c:144:19: warning: assignment to 'struct i2c_adapter *' from 'int' makes pointer from integer without a cast [-Wint-conversion] 144 | *adap_ret = i2c_get_adapter(adap->nr); | ^ drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_instantiate_i2c_client': >> drivers/platform/x86/dell/dell-smo8800.c:204:28: error: implicit declaration of function 'i2c_new_client_device' [-Werror=implicit-function-declaration] 204 | smo8800->i2c_dev = i2c_new_client_device(adap, &info); | ^~~~~~~~~~~~~~~~~~~~~ >> drivers/platform/x86/dell/dell-smo8800.c:204:26: warning: assignment to 'struct i2c_client *' from 'int' makes pointer from integer without a cast [-Wint-conversion] 204 | smo8800->i2c_dev = i2c_new_client_device(adap, &info); | ^ >> drivers/platform/x86/dell/dell-smo8800.c:214:9: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration] 214 | i2c_put_adapter(adap); | ^~~~~~~~~~~~~~~ drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_probe': >> drivers/platform/x86/dell/dell-smo8800.c:267:9: error: implicit declaration of function 'i2c_unregister_device'; did you mean 'pci_unregister_driver'? [-Werror=implicit-function-declaration] 267 | i2c_unregister_device(smo8800->i2c_dev); | ^~~~~~~~~~~~~~~~~~~~~ | pci_unregister_driver cc1: some warnings being treated as errors vim +144 drivers/platform/x86/dell/dell-smo8800.c 129 130 static int smo8800_find_i801(struct device *dev, void *data) 131 { 132 struct i2c_adapter *adap, **adap_ret = data; 133 134 adap = i2c_verify_adapter(dev); 135 if (!adap) 136 return 0; 137 138 if (!strstarts(adap->name, "SMBus I801 adapter")) 139 return 0; 140 141 if (pci_match_id(i801_idf_ids, to_pci_dev(adap->dev.parent))) 142 return 0; /* Only register client on main SMBus channel */ 143 > 144 *adap_ret = i2c_get_adapter(adap->nr); 145 return 1; 146 } 147 148 /* 149 * Accelerometer's I2C address is not specified in DMI nor ACPI, 150 * so it is needed to define mapping table based on DMI product names. 151 */ 152 static const struct { 153 const char *dmi_product_name; 154 unsigned short i2c_addr; 155 } dell_lis3lv02d_devices[] = { 156 /* 157 * Dell platform team told us that these Latitude devices have 158 * ST microelectronics accelerometer at I2C address 0x29. 159 */ 160 { "Latitude E5250", 0x29 }, 161 { "Latitude E5450", 0x29 }, 162 { "Latitude E5550", 0x29 }, 163 { "Latitude E6440", 0x29 }, 164 { "Latitude E6440 ATG", 0x29 }, 165 { "Latitude E6540", 0x29 }, 166 /* 167 * Additional individual entries were added after verification. 168 */ 169 { "Latitude 5480", 0x29 }, 170 { "Vostro V131", 0x1d }, 171 { "Vostro 5568", 0x29 }, 172 }; 173 174 static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) 175 { 176 struct i2c_board_info info = { }; 177 struct i2c_adapter *adap = NULL; 178 const char *dmi_product_name; 179 u8 addr = 0; 180 int i; 181 182 bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801); 183 if (!adap) 184 return; 185 186 dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); 187 for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { 188 if (strcmp(dmi_product_name, 189 dell_lis3lv02d_devices[i].dmi_product_name) == 0) { 190 addr = dell_lis3lv02d_devices[i].i2c_addr; 191 break; 192 } 193 } 194 195 if (!addr) { 196 dev_warn(smo8800->dev, 197 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); 198 goto put_adapter; 199 } 200 201 info.addr = addr; 202 strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); 203 > 204 smo8800->i2c_dev = i2c_new_client_device(adap, &info); 205 if (IS_ERR(smo8800->i2c_dev)) { 206 dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev), 207 "registering accel i2c_client\n"); 208 smo8800->i2c_dev = NULL; 209 } else { 210 dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", 211 info.type, info.addr); 212 } 213 put_adapter: > 214 i2c_put_adapter(adap); 215 } 216 217 static int smo8800_probe(struct platform_device *device) 218 { 219 int err; 220 struct smo8800_device *smo8800; 221 222 smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL); 223 if (!smo8800) { 224 dev_err(&device->dev, "failed to allocate device data\n"); 225 return -ENOMEM; 226 } 227 228 smo8800->dev = &device->dev; 229 smo8800->miscdev.minor = MISC_DYNAMIC_MINOR; 230 smo8800->miscdev.name = "freefall"; 231 smo8800->miscdev.fops = &smo8800_misc_fops; 232 233 init_waitqueue_head(&smo8800->misc_wait); 234 235 err = platform_get_irq(device, 0); 236 if (err < 0) 237 return err; 238 smo8800->irq = err; 239 240 smo8800_instantiate_i2c_client(smo8800); 241 242 err = misc_register(&smo8800->miscdev); 243 if (err) { 244 dev_err(&device->dev, "failed to register misc dev: %d\n", err); 245 goto error_unregister_i2c_client; 246 } 247 248 err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, 249 smo8800_interrupt_thread, 250 IRQF_TRIGGER_RISING | IRQF_ONESHOT, 251 DRIVER_NAME, smo8800); 252 if (err) { 253 dev_err(&device->dev, 254 "failed to request thread for IRQ %d: %d\n", 255 smo8800->irq, err); 256 goto error; 257 } 258 259 dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", 260 smo8800->irq); 261 platform_set_drvdata(device, smo8800); 262 return 0; 263 264 error: 265 misc_deregister(&smo8800->miscdev); 266 error_unregister_i2c_client: > 267 i2c_unregister_device(smo8800->i2c_dev); 268 return err; 269 } 270
Hi Pali, Hans, On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: > On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > > It is not necessary to handle the Dell specific instantiation of > > i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource > > inside the generic i801 I2C adapter driver. > > > > The kernel already instantiates platform_device-s for these ACPI devices > > and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these > > platform drivers. > > > > Move the i2c_client instantiation from the generic i2c-i801 driver to > > the Dell specific dell-smo8800 driver. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > Changes in v2: > > - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses > > - Add a comment documenting the IDF PCI device ids > > --- > > drivers/i2c/busses/i2c-i801.c | 126 +---------------------- > > drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++- > > 2 files changed, 123 insertions(+), 124 deletions(-) > > I'm looking at this change again and I'm not not sure if it is a good > direction to do this movement. (...) Same feeling here. Having to lookup the parent i2c bus, which may or may not be present yet, doesn't feel good. I wouldn't object if everybody was happy with the move and moving the code was solving an actual issue, but that doesn't seem to be the case.
Hi Jean, On 2/13/24 17:30, Jean Delvare wrote: > Hi Pali, Hans, > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: >>> It is not necessary to handle the Dell specific instantiation of >>> i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource >>> inside the generic i801 I2C adapter driver. >>> >>> The kernel already instantiates platform_device-s for these ACPI devices >>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these >>> platform drivers. >>> >>> Move the i2c_client instantiation from the generic i2c-i801 driver to >>> the Dell specific dell-smo8800 driver. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Changes in v2: >>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses >>> - Add a comment documenting the IDF PCI device ids >>> --- >>> drivers/i2c/busses/i2c-i801.c | 126 +---------------------- >>> drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++- >>> 2 files changed, 123 insertions(+), 124 deletions(-) >> >> I'm looking at this change again and I'm not not sure if it is a good >> direction to do this movement. (...) > > Same feeling here. Having to lookup the parent i2c bus, which may or > may not be present yet, doesn't feel good. > > I wouldn't object if everybody was happy with the move and moving the > code was solving an actual issue, but that doesn't seem to be the case. I thought you would actually like getting this somewhat clunky code which basically works around the hw not being properly described in the ACPI tables out of the generic i2c-i801 code. I didn't get around to answer's Pali's concerns yet, so let me start by addressing those since you indicate that you share Pali's concerns: Pali wrote: > Now after looking at this change again I see there a problem. If i2c-801 > driver initialize i2c-801 device after this smo8800 is called then > accelerometer i2c device would not happen. That is a good point (which Jean also points out). But this can simply be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER if the i2c-i801 i2c-bus is not present yet (all designs using the dell-smo8800 driver will have an i2c-bus so waiting for this to show up should not cause regressions). If we can agree to move forward this series I'll fix this. Pali wrote: > Also it has same problem if PCI i801 device is reloaded or reset. The i801 device is not hotplugable, so normally this will never happen. If the user manually unbinds + rebinds the i2c-i801 driver them the i2c_client for the smo88xx device will indeed get removed and not re-added. But this will normally never happen and if a user is manually poking things then the user can also unbind + rebind the dell-mso8800 driver after the i2c-i801 rebind. So I don't really see this as an issue. With those remarks addressed let me try to explain why I think that moving this to the dell-smo8800 code is a good idea: 1. It is a SMO88xx ACPI device specific kludge and as such IMHO thus belongs in the driver for the SMO88xx ACPI platform_device. The i2c-i801 driver gets loaded on every x86 system and it is undesirable to have this extra code and the DMI table in RAM on all those other systems. 2. Further changes in this series, like adding support for probing for the i2c address of the lis3lv02d device on models not yet in the DMI table, will add a bunch of more code specific to SMO88xx ACPI devices. Making the problem of having SMO88xx specific code in the generic i2c-i801 driver even bigger. The current amount of SMO88xx specific code in the generic i2c-i801 driver might be considered acceptable but I'm afraid that the amount of code after this series will not be acceptable. 3. Some of the changes in this series are harder to implement inside the i2c-i801 code, like optionally instantiating an i2c_client for the IIO st_accel driver (*) so that the accelerometer gets presented to userspace as a standard IIO device like all modern accelerometer drivers do. This requires setting i2c_client.irq and that IRQ comes from the SMO88xx ACPI device. So this would require the i2c-i801 code to lookup the ACPI device and get the IRQ from there. Where as in the SMO88xx ACPI platform_device driver the IRQ is readily available. TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in the SMO88xx specific dell-smo8800 driver rather then in the generic i2c-i801 code. Regards, Hans *) Instead of an i2c_client for the somewhat weird (but still default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c driver
On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote: > On 2/13/24 17:30, Jean Delvare wrote: > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: FWIW, I agree with Hans on the location of the HW quirks. If there is a possible way to make the actual driver cleaner and collect quirks somewhere else, I'm full support for that. > >> I'm looking at this change again and I'm not not sure if it is a good > >> direction to do this movement. (...) > > > > Same feeling here. Having to lookup the parent i2c bus, which may or > > may not be present yet, doesn't feel good. > > > > I wouldn't object if everybody was happy with the move and moving the > > code was solving an actual issue, but that doesn't seem to be the case. > > I thought you would actually like getting this somewhat clunky code > which basically works around the hw not being properly described in > the ACPI tables out of the generic i2c-i801 code. > > I didn't get around to answer's Pali's concerns yet, so let me > start by addressing those since you indicate that you share Pali's > concerns: > > Pali wrote: > > Now after looking at this change again I see there a problem. If i2c-801 > > driver initialize i2c-801 device after this smo8800 is called then > > accelerometer i2c device would not happen. > > That is a good point (which Jean also points out). But this can simply > be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER > if the i2c-i801 i2c-bus is not present yet (all designs using the > dell-smo8800 driver will have an i2c-bus so waiting for this to show > up should not cause regressions). > > If we can agree to move forward this series I'll fix this. > > Pali wrote: > > Also it has same problem if PCI i801 device is reloaded or reset. > > The i801 device is not hotplugable, so normally this will never > happen. If the user manually unbinds + rebinds the i2c-i801 driver > them the i2c_client for the smo88xx device will indeed get removed > and not re-added. But this will normally never happen and if > a user is manually poking things then the user can also unbind + > rebind the dell-mso8800 driver after the i2c-i801 rebind. > So I don't really see this as an issue. > > With those remarks addressed let me try to explain why I think > that moving this to the dell-smo8800 code is a good idea: > > 1. It is a SMO88xx ACPI device specific kludge and as such IMHO > thus belongs in the driver for the SMO88xx ACPI platform_device. > > The i2c-i801 driver gets loaded on every x86 system and it is > undesirable to have this extra code and the DMI table in RAM > on all those other systems. > > 2. Further changes in this series, like adding support for > probing for the i2c address of the lis3lv02d device on models > not yet in the DMI table, will add a bunch of more code specific > to SMO88xx ACPI devices. Making the problem of having SMO88xx > specific code in the generic i2c-i801 driver even bigger. > The current amount of SMO88xx specific code in the > generic i2c-i801 driver might be considered acceptable but I'm > afraid that the amount of code after this series will not be > acceptable. > > 3. Some of the changes in this series are harder to implement inside > the i2c-i801 code, like optionally instantiating an i2c_client for > the IIO st_accel driver (*) so that the accelerometer gets presented > to userspace as a standard IIO device like all modern accelerometer > drivers do. > > This requires setting i2c_client.irq and that IRQ comes from > the SMO88xx ACPI device. So this would require the i2c-i801 code > to lookup the ACPI device and get the IRQ from there. Where as > in the SMO88xx ACPI platform_device driver the IRQ is readily > available. > > TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in > the SMO88xx specific dell-smo8800 driver rather then in > the generic i2c-i801 code. > > Regards, > > Hans > > > *) Instead of an i2c_client for the somewhat weird (but still > default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c > driver
On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote: > On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote: > > On 2/13/24 17:30, Jean Delvare wrote: > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: > > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > > FWIW, I agree with Hans on the location of the HW quirks. > If there is a possible way to make the actual driver cleaner > and collect quirks somewhere else, I'm full support for that. Location of the quirks can be moved outside of the i2c-i801.c source code relatively easily without need to change the way how parent--child relationship currently works. Relevant functions is_dell_system_with_lis3lv02d() and register_dell_lis3lv02d_i2c_device() does not use internals of i2c-i801 and could be moved into new file, lets say drivers/platform/x86/dell/dell-smo8800-plat.c Put this file under a new hidden "bool" config option which is auto enabled when CONFIG_DELL_SMO8800 is used. i2c-i801.c currently has code: if (is_dell_system_with_lis3lv02d()) register_dell_lis3lv02d_i2c_device(priv); This can be put into a new exported function, e.g. void dell_smo8800_scan_i2c(struct i2c_adapter *adapter); And i2c-i801.c would call it instead. register_dell_lis3lv02d_i2c_device just needs "adapter", it does not need whole i801 priv struct. With this simple change all dell smo8800 code would be in its subdir drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. This approach does not change any functionality, so should be absolutely safe. Future changes will be done only in drivers/platform/x86/dell/ subdir, touching i801 would not be needed at all.
On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote: > > On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote: > > > On 2/13/24 17:30, Jean Delvare wrote: > > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: > > > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > > > > FWIW, I agree with Hans on the location of the HW quirks. > > If there is a possible way to make the actual driver cleaner > > and collect quirks somewhere else, I'm full support for that. > > Location of the quirks can be moved outside of the i2c-i801.c source > code relatively easily without need to change the way how parent--child > relationship currently works. > > Relevant functions is_dell_system_with_lis3lv02d() and > register_dell_lis3lv02d_i2c_device() does not use internals of > i2c-i801 and could be moved into new file, lets say > drivers/platform/x86/dell/dell-smo8800-plat.c > Put this file under a new hidden "bool" config option which is auto > enabled when CONFIG_DELL_SMO8800 is used. > > i2c-i801.c currently has code: > > if (is_dell_system_with_lis3lv02d()) > register_dell_lis3lv02d_i2c_device(priv); > > This can be put into a new exported function, e.g. > void dell_smo8800_scan_i2c(struct i2c_adapter *adapter); > And i2c-i801.c would call it instead. > > register_dell_lis3lv02d_i2c_device just needs "adapter", it does not > need whole i801 priv struct. I'm wondering why we need all this. We have notifiers when a device is added / removed. We can provide a board_info for the device and attach it to the proper adapter, no? > With this simple change all dell smo8800 code would be in its subdir > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. > > This approach does not change any functionality, so should be absolutely > safe. > > Future changes will be done only in drivers/platform/x86/dell/ subdir, > touching i801 would not be needed at all. Still these exported functions are not the best solution we can do, right? We should be able to decouple them without need for the custom APIs.
On Saturday 17 February 2024 11:33:21 Hans de Goede wrote: > Hi Jean, > > On 2/13/24 17:30, Jean Delvare wrote: > > Hi Pali, Hans, > > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > >>> It is not necessary to handle the Dell specific instantiation of > >>> i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource > >>> inside the generic i801 I2C adapter driver. > >>> > >>> The kernel already instantiates platform_device-s for these ACPI devices > >>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these > >>> platform drivers. > >>> > >>> Move the i2c_client instantiation from the generic i2c-i801 driver to > >>> the Dell specific dell-smo8800 driver. > >>> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>> --- > >>> Changes in v2: > >>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses > >>> - Add a comment documenting the IDF PCI device ids > >>> --- > >>> drivers/i2c/busses/i2c-i801.c | 126 +---------------------- > >>> drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++- > >>> 2 files changed, 123 insertions(+), 124 deletions(-) > >> > >> I'm looking at this change again and I'm not not sure if it is a good > >> direction to do this movement. (...) > > > > Same feeling here. Having to lookup the parent i2c bus, which may or > > may not be present yet, doesn't feel good. > > > > I wouldn't object if everybody was happy with the move and moving the > > code was solving an actual issue, but that doesn't seem to be the case. > > I thought you would actually like getting this somewhat clunky code > which basically works around the hw not being properly described in > the ACPI tables out of the generic i2c-i801 code. > > I didn't get around to answer's Pali's concerns yet, so let me > start by addressing those since you indicate that you share Pali's > concerns: > > Pali wrote: > > Now after looking at this change again I see there a problem. If i2c-801 > > driver initialize i2c-801 device after this smo8800 is called then > > accelerometer i2c device would not happen. > > That is a good point (which Jean also points out). But this can simply > be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER > if the i2c-i801 i2c-bus is not present yet (all designs using the > dell-smo8800 driver will have an i2c-bus so waiting for this to show > up should not cause regressions). Adding EPROBE_DEFER just complicates the dependency and state model. I would really suggest to come up with a simpler solution, not too complicated where it is required to think a lot if is is correct and if all edge-cases are handled. > If we can agree to move forward this series I'll fix this. > > Pali wrote: > > Also it has same problem if PCI i801 device is reloaded or reset. > > The i801 device is not hotplugable, so normally this will never > happen. If the user manually unbinds + rebinds the i2c-i801 driver > them the i2c_client for the smo88xx device will indeed get removed > and not re-added. But this will normally never happen and if > a user is manually poking things then the user can also unbind + > rebind the dell-mso8800 driver after the i2c-i801 rebind. > So I don't really see this as an issue. Well, rmmod & modprobe is not the rare cases. Whatever developers say about rmmod (or modprobe -r or whatever is the way for unloading modules), this is something which is used by a lot of users and would be used. > With those remarks addressed let me try to explain why I think > that moving this to the dell-smo8800 code is a good idea: > > 1. It is a SMO88xx ACPI device specific kludge and as such IMHO > thus belongs in the driver for the SMO88xx ACPI platform_device. I'm not sure if it belongs to "SMO88xx ACPI platform_device" but for sure I agree with you that it does not belong to i801 code. I would say that it belongs to some SMO8800 glue code -- because it is not the classic ACPI driver too. But I'm not against to have SMO glue code and SMO ACPI driver in one file (maybe it is even better to have it). > The i2c-i801 driver gets loaded on every x86 system and it is > undesirable to have this extra code and the DMI table in RAM > on all those other systems. I think we can take an assumption that ACPI SMO device does not change it existence or ACPI enabled/disabled state during runtime. So we can scan for ACPI SMO device just once in function stored in __init section called during the kernel/module initialization and cache the result (bool if device was found + its i2c address). After function marked as __init finish its job then together with DMI tables can be discarded from RAM. With this way it does take extra memory on every x86 system. Also we can combine this with an SMO config option, so the whole code "glue" code would not be compiled when SMO driver is not enabled via Kconfig. > 2. Further changes in this series, like adding support for > probing for the i2c address of the lis3lv02d device on models > not yet in the DMI table, will add a bunch of more code specific > to SMO88xx ACPI devices. Making the problem of having SMO88xx > specific code in the generic i2c-i801 driver even bigger. > The current amount of SMO88xx specific code in the > generic i2c-i801 driver might be considered acceptable but I'm > afraid that the amount of code after this series will not be > acceptable. I think alternative approach which I described in the other email in this thread could be useful for this issue too (to move SMO code from i2c-i801.c source file). Together with above __init section approach it can also decrease memory usage. > 3. Some of the changes in this series are harder to implement inside > the i2c-i801 code, like optionally instantiating an i2c_client for > the IIO st_accel driver (*) so that the accelerometer gets presented > to userspace as a standard IIO device like all modern accelerometer > drivers do. This is something about which I'm not very convinced. IIO st_accel driver does not support freefall interface (or any other for signalling hard disk falls, which is used by userspace) and in dell systems, this hard disk protection is the primary usage of the accelerometer. In last two months I talked with two people, users of the accelerometers axis on dell and thinkpad machines. They were using it in games which were joystick-based (one game was tuxracer, second I do not remember name). So I'm not sure that replacing joystick driver by some new API would be really useful for users of accelerometer axis. Before such change I would propose to teach IIO st_accel driver (or what would be the replacement) to support joystick API for userspace. > This requires setting i2c_client.irq and that IRQ comes from > the SMO88xx ACPI device. So this would require the i2c-i801 code > to lookup the ACPI device and get the IRQ from there. Where as > in the SMO88xx ACPI platform_device driver the IRQ is readily > available. I understand this problem. But I would like to ask a question: WHY it is needed at all? The IRQ represents the free fall / hard disk fall event, which is slightly different thing than reporting accelerometer axis. Why IIO st_accel (or lis3lv02d) needs free fall IRQ? Hard disk fall interrupt on Dell machines can be handled by separate driver, not related to ACPI SMO8800 device. It would be much more easier to split these two different functionalities (reporting axes; and reporting hard disk fall event) into two separate drivers. And it would simplify whole logic related to instantiating free fall hard disk driver and accelerometer axes driver (either IIO st_accel or lis3lv02d or some other...). So from my side, I do not see a reason to "inject" IRQ number into driver which reads accelerometer axes. > TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in > the SMO88xx specific dell-smo8800 driver rather then in > the generic i2c-i801 code. I agree, that it does not belong to the i2c-i801.c source file. And I also would like to see movement. That is why I proposed alternative and simpler solution. > Regards, > > Hans > > > *) Instead of an i2c_client for the somewhat weird (but still > default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c > driver > > > > > > >
On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: > On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > > On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote: > > > On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote: > > > > On 2/13/24 17:30, Jean Delvare wrote: > > > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: > > > > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > > > > > > FWIW, I agree with Hans on the location of the HW quirks. > > > If there is a possible way to make the actual driver cleaner > > > and collect quirks somewhere else, I'm full support for that. > > > > Location of the quirks can be moved outside of the i2c-i801.c source > > code relatively easily without need to change the way how parent--child > > relationship currently works. > > > > Relevant functions is_dell_system_with_lis3lv02d() and > > register_dell_lis3lv02d_i2c_device() does not use internals of > > i2c-i801 and could be moved into new file, lets say > > drivers/platform/x86/dell/dell-smo8800-plat.c > > Put this file under a new hidden "bool" config option which is auto > > enabled when CONFIG_DELL_SMO8800 is used. > > > > i2c-i801.c currently has code: > > > > if (is_dell_system_with_lis3lv02d()) > > register_dell_lis3lv02d_i2c_device(priv); > > > > This can be put into a new exported function, e.g. > > void dell_smo8800_scan_i2c(struct i2c_adapter *adapter); > > And i2c-i801.c would call it instead. > > > > register_dell_lis3lv02d_i2c_device just needs "adapter", it does not > > need whole i801 priv struct. > > I'm wondering why we need all this. We have notifiers when a device is > added / removed. We can provide a board_info for the device and attach > it to the proper adapter, no? I do not know how flexible are notifiers. Can notifier call our callback when new "struct i2c_adapter *adapter" was instanced? > > With this simple change all dell smo8800 code would be in its subdir > > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. > > > > This approach does not change any functionality, so should be absolutely > > safe. > > > > Future changes will be done only in drivers/platform/x86/dell/ subdir, > > touching i801 would not be needed at all. > > Still these exported functions are not the best solution we can do, > right? We should be able to decouple them without need for the custom > APIs. Well, what I described here is a simple change which get rid of the one problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx logic (like adding a new device id) requires touching unrelated i2c-i801.c source file. I like small changes which can be easily reviewed and address one problem. Step by step. That is why I proposed it here. For decoupling it is needed to get newly instanced adapter (if the mentioned notifier is able to tell this information) and also it is needed to check if the adapter is the i801.
On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote: > On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: > > On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: ... > > I'm wondering why we need all this. We have notifiers when a device is > > added / removed. We can provide a board_info for the device and attach > > it to the proper adapter, no? > > I do not know how flexible are notifiers. Can notifier call our callback > when new "struct i2c_adapter *adapter" was instanced? You can follow notifications of *an* I2C adapter being added / removed. With that, you can filter which one is that. Based on that you may attach a saved (at __init as you talked about in the reply to Hans) board_info with all necessary information. Something like this (combined) https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515 https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194 > > > With this simple change all dell smo8800 code would be in its subdir > > > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. > > > > > > This approach does not change any functionality, so should be absolutely > > > safe. > > > > > > Future changes will be done only in drivers/platform/x86/dell/ subdir, > > > touching i801 would not be needed at all. > > > > Still these exported functions are not the best solution we can do, > > right? We should be able to decouple them without need for the custom > > APIs. > > Well, what I described here is a simple change which get rid of the one > problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx > logic (like adding a new device id) requires touching unrelated > i2c-i801.c source file. `get rid of one problem` --> `replace one by another (but maybe less critical, dunno) problem`. The new one is the spread of custom APIs for a single user, which also requires an additional, shared header file and all hell with the Kconfig dependencies. > I like small changes which can be easily reviewed and address one > problem. Step by step. That is why I proposed it here. > > For decoupling it is needed to get newly instanced adapter (if the > mentioned notifier is able to tell this information) and also it is > needed to check if the adapter is the i801. Yes.
Hi, On 2/27/24 23:37, Andy Shevchenko wrote: > On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote: >> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: >>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > > ... > >>> I'm wondering why we need all this. We have notifiers when a device is >>> added / removed. We can provide a board_info for the device and attach >>> it to the proper adapter, no? >> >> I do not know how flexible are notifiers. Can notifier call our callback >> when new "struct i2c_adapter *adapter" was instanced? > > You can follow notifications of *an* I2C adapter being added / > removed. With that, you can filter which one is that. Based on that > you may attach a saved (at __init as you talked about in the reply to > Hans) board_info with all necessary information. > > Something like this (combined) > https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515 > https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194 drivers/platform/x86/touchscreen_dmi.c actually already does something like this for i2c-clients. The problem is that this brings probe-ordering problems with it. If the i801 driver is loaded before the dell-smo8800 driver then the notifiers will not trigger since the i2c-adapter has already been created (1). So we would still need a "cold-plug" manual scan in smo8800_probe() anyways at which point we might as well just return -EPROBE_DEFER when the adapter is not there. As for Pali's suggestion of having the i2c-i801 code call a symbol exported by dell-smo8800 that will cause the dell-smo8800 driver to load on all x86 devices with an i2c-i801 controller (pretty much all of them). Slowing the boot and eating memory. So IMHO just doing the scan for the i2c-i801 adapter as already done in this version of the patch-set, extended with returning -EPROBE_DEFER if it is not found is the best solution. Yes this means losing the i2c_client for the lis3lv02d device if the i2c-i801 driver is manually unbound or rmmod-ed. But that requires explicit root user action and putting just the i801 driver back in place again also requires implicit root action. IMHO it is acceptable that in this exceptional case, which normal users will never hit, a rmmod + modprobe of dell-smo8800 is required to re-gain the lis3lv02d i2c_client. Regards, Hans 1) touchscreen_dmi is builtin only because of this and we really want to avoid adding more of that. Actually thinking more about this it would be nice to modify touchscreen_dmi to use a mix of registering the notifier + doing a scan after registering it for matching devices which are already present, so that touchscreen_dmi can become a module auto-loaded using the DMI info which it already contains. > >>>> With this simple change all dell smo8800 code would be in its subdir >>>> drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. >>>> >>>> This approach does not change any functionality, so should be absolutely >>>> safe. >>>> >>>> Future changes will be done only in drivers/platform/x86/dell/ subdir, >>>> touching i801 would not be needed at all. >>> >>> Still these exported functions are not the best solution we can do, >>> right? We should be able to decouple them without need for the custom >>> APIs. >> >> Well, what I described here is a simple change which get rid of the one >> problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx >> logic (like adding a new device id) requires touching unrelated >> i2c-i801.c source file. > > `get rid of one problem` --> `replace one by another (but maybe less > critical, dunno) problem`. The new one is the spread of custom APIs > for a single user, which also requires an additional, shared header > file and all hell with the Kconfig dependencies. > >> I like small changes which can be easily reviewed and address one >> problem. Step by step. That is why I proposed it here. >> >> For decoupling it is needed to get newly instanced adapter (if the >> mentioned notifier is able to tell this information) and also it is >> needed to check if the adapter is the i801. > > Yes. > >
Hi Pali, On 2/27/24 22:40, Pali Rohár wrote: > On Saturday 17 February 2024 11:33:21 Hans de Goede wrote: >> Hi Jean, >> >> On 2/13/24 17:30, Jean Delvare wrote: >>> Hi Pali, Hans, >>> >>> On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote: >>>> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: >>>>> It is not necessary to handle the Dell specific instantiation of >>>>> i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource >>>>> inside the generic i801 I2C adapter driver. >>>>> >>>>> The kernel already instantiates platform_device-s for these ACPI devices >>>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these >>>>> platform drivers. >>>>> >>>>> Move the i2c_client instantiation from the generic i2c-i801 driver to >>>>> the Dell specific dell-smo8800 driver. >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> Changes in v2: >>>>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses >>>>> - Add a comment documenting the IDF PCI device ids >>>>> --- >>>>> drivers/i2c/busses/i2c-i801.c | 126 +---------------------- >>>>> drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++- >>>>> 2 files changed, 123 insertions(+), 124 deletions(-) >>>> >>>> I'm looking at this change again and I'm not not sure if it is a good >>>> direction to do this movement. (...) >>> >>> Same feeling here. Having to lookup the parent i2c bus, which may or >>> may not be present yet, doesn't feel good. >>> >>> I wouldn't object if everybody was happy with the move and moving the >>> code was solving an actual issue, but that doesn't seem to be the case. >> >> I thought you would actually like getting this somewhat clunky code >> which basically works around the hw not being properly described in >> the ACPI tables out of the generic i2c-i801 code. >> >> I didn't get around to answer's Pali's concerns yet, so let me >> start by addressing those since you indicate that you share Pali's >> concerns: >> >> Pali wrote: >>> Now after looking at this change again I see there a problem. If i2c-801 >>> driver initialize i2c-801 device after this smo8800 is called then >>> accelerometer i2c device would not happen. >> >> That is a good point (which Jean also points out). But this can simply >> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER >> if the i2c-i801 i2c-bus is not present yet (all designs using the >> dell-smo8800 driver will have an i2c-bus so waiting for this to show >> up should not cause regressions). > > Adding EPROBE_DEFER just complicates the dependency and state model. > I would really suggest to come up with a simpler solution, not too > complicated where it is required to think a lot if is is correct and if > all edge-cases are handled. I'm not sure what you are worried about here. dell-smo8800 is a leave driver, nothing inside the kernel depends on it being loaded or not. So there are no -EPROBE_DEFER complexities here, yes -EPROBE_DEFER can become a bit hairy with complex dependency graphs, but this is a very KISS case. Are there any specific scenarios you are actually worried about in this specific case? >> If we can agree to move forward this series I'll fix this. >> >> Pali wrote: >>> Also it has same problem if PCI i801 device is reloaded or reset. >> >> The i801 device is not hotplugable, so normally this will never >> happen. If the user manually unbinds + rebinds the i2c-i801 driver >> them the i2c_client for the smo88xx device will indeed get removed >> and not re-added. But this will normally never happen and if >> a user is manually poking things then the user can also unbind + >> rebind the dell-mso8800 driver after the i2c-i801 rebind. >> So I don't really see this as an issue. > > Well, rmmod & modprobe is not the rare cases. Whatever developers say > about rmmod (or modprobe -r or whatever is the way for unloading > modules), this is something which is used by a lot of users and would be > used. Many modules actually have bugs in there remove paths and crash, this is really not a common case. Sometimes users use rmmod + modprobe surrounding suspend/resume for e.g. a wifi driver to work around suspend/resume problems but I have never heard of this being used for a driver like i2c-i801. And again if users are manually meddling with this, the they can also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801. >> With those remarks addressed let me try to explain why I think >> that moving this to the dell-smo8800 code is a good idea: >> >> 1. It is a SMO88xx ACPI device specific kludge and as such IMHO >> thus belongs in the driver for the SMO88xx ACPI platform_device. > > I'm not sure if it belongs to "SMO88xx ACPI platform_device" but for > sure I agree with you that it does not belong to i801 code. I would say > that it belongs to some SMO8800 glue code -- because it is not the > classic ACPI driver too. But I'm not against to have SMO glue code and > SMO ACPI driver in one file (maybe it is even better to have it). We are trying to get rid of acpi_driver drivers using only platform_driver drivers for the platform_devices created for ACPI devices / fwnodes which do not have another physical device. Also we only want this workaround when the SMO88xx ACPI fwnode is missing the I2cSerialBusV2 resource for the i2c_client and conveniently the platform_device will only be created for ACPI fwnodes without the I2cSerialBusV2 resource for proper ACPI fwnodes which have the I2C resource an i2c_client will be created instead. So putting the workaround in the platform_driver automatically ensures that it only runs when the ACPI fwnode is incomplete. > >> The i2c-i801 driver gets loaded on every x86 system and it is >> undesirable to have this extra code and the DMI table in RAM >> on all those other systems. > > I think we can take an assumption that ACPI SMO device does not change > it existence or ACPI enabled/disabled state during runtime. So we can > scan for ACPI SMO device just once in function stored in __init section > called during the kernel/module initialization and cache the result > (bool if device was found + its i2c address). After function marked as > __init finish its job then together with DMI tables can be discarded > from RAM. With this way it does take extra memory on every x86 system. > Also we can combine this with an SMO config option, so the whole code > "glue" code would not be compiled when SMO driver is not enabled via > Kconfig. This approach does not work because i2c-i801.c registers a PCI driver, there is no guarantee that the adapter has already been probed and an i2c_adapter has been created before it. A PCI driver's probe() function must not be __init and thus any code which it calls also must not be __init. So the majority of the smo88xx handling can not be __init. Regards, Hans
On Wed, Feb 28, 2024 at 3:10 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 2/27/24 22:40, Pali Rohár wrote: > > On Saturday 17 February 2024 11:33:21 Hans de Goede wrote: > >> On 2/13/24 17:30, Jean Delvare wrote: ... > >> The i801 device is not hotplugable, so normally this will never > >> happen. If the user manually unbinds + rebinds the i2c-i801 driver > >> them the i2c_client for the smo88xx device will indeed get removed > >> and not re-added. But this will normally never happen and if > >> a user is manually poking things then the user can also unbind + > >> rebind the dell-mso8800 driver after the i2c-i801 rebind. > >> So I don't really see this as an issue. > > > > Well, rmmod & modprobe is not the rare cases. Whatever developers say > > about rmmod (or modprobe -r or whatever is the way for unloading > > modules), this is something which is used by a lot of users and would be > > used. > > Many modules actually have bugs in there remove paths and crash, > this is really not a common case. Sometimes users use rmmod + modprobe > surrounding suspend/resume for e.g. a wifi driver to work around > suspend/resume problems but I have never heard of this being used > for a driver like i2c-i801. Hmm... The whole thing of reworking the p2sb was done due to rebounding the i2c-i801 IIUC.
On Wednesday 28 February 2024 13:50:27 Hans de Goede wrote: > Hi, > > On 2/27/24 23:37, Andy Shevchenko wrote: > > On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote: > >> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: > >>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > > > > ... > > > >>> I'm wondering why we need all this. We have notifiers when a device is > >>> added / removed. We can provide a board_info for the device and attach > >>> it to the proper adapter, no? > >> > >> I do not know how flexible are notifiers. Can notifier call our callback > >> when new "struct i2c_adapter *adapter" was instanced? > > > > You can follow notifications of *an* I2C adapter being added / > > removed. With that, you can filter which one is that. Based on that > > you may attach a saved (at __init as you talked about in the reply to > > Hans) board_info with all necessary information. > > > > Something like this (combined) > > https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515 > > https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194 > > drivers/platform/x86/touchscreen_dmi.c actually already does something > like this for i2c-clients. The problem is that this brings probe-ordering > problems with it. If the i801 driver is loaded before the dell-smo8800 > driver then the notifiers will not trigger since the i2c-adapter has > already been created (1). > > So we would still need a "cold-plug" manual scan in smo8800_probe() > anyways at which point we might as well just return -EPROBE_DEFER > when the adapter is not there. And that it example why the current existing solution is better, it does not have such problems like the proposed. > As for Pali's suggestion of having the i2c-i801 code call a symbol > exported by dell-smo8800 I did not suggest that! Please do not make false statements about me. > that will cause the dell-smo8800 driver > to load on all x86 devices with an i2c-i801 controller (pretty > much all of them). Slowing the boot and eating memory.
On Wednesday 28 February 2024 14:10:14 Hans de Goede wrote: > >>> Now after looking at this change again I see there a problem. If i2c-801 > >>> driver initialize i2c-801 device after this smo8800 is called then > >>> accelerometer i2c device would not happen. > >> > >> That is a good point (which Jean also points out). But this can simply > >> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER > >> if the i2c-i801 i2c-bus is not present yet (all designs using the > >> dell-smo8800 driver will have an i2c-bus so waiting for this to show > >> up should not cause regressions). > > > > Adding EPROBE_DEFER just complicates the dependency and state model. > > I would really suggest to come up with a simpler solution, not too > > complicated where it is required to think a lot if is is correct and if > > all edge-cases are handled. > > I'm not sure what you are worried about here. dell-smo8800 is > a leave driver, nothing inside the kernel depends on it being > loaded or not. So there are no -EPROBE_DEFER complexities here, > yes -EPROBE_DEFER can become a bit hairy with complex dependency > graphs, but this is a very KISS case. > > Are there any specific scenarios you are actually worried about > in this specific case? -EPROBE_DEFER restarts and schedule probing of the device later. It does not inform device manager when it can try do it. So it can try probing it many more times until it decide to not try it again. This asynchronous design is broken and I do not see reason why to use it in another driver, in case we have a cleaner solution how to initialize and probe device without -EPROBE_DEFER. This is for sure not a KISS case but a case with lot of corner cases... and your proposed patch is just an example of it as you forgot about at least one corner case. Current solution does not have edge cases... this can be marked as KISS design. > >> If we can agree to move forward this series I'll fix this. > >> > >> Pali wrote: > >>> Also it has same problem if PCI i801 device is reloaded or reset. > >> > >> The i801 device is not hotplugable, so normally this will never > >> happen. If the user manually unbinds + rebinds the i2c-i801 driver > >> them the i2c_client for the smo88xx device will indeed get removed > >> and not re-added. But this will normally never happen and if > >> a user is manually poking things then the user can also unbind + > >> rebind the dell-mso8800 driver after the i2c-i801 rebind. > >> So I don't really see this as an issue. > > > > Well, rmmod & modprobe is not the rare cases. Whatever developers say > > about rmmod (or modprobe -r or whatever is the way for unloading > > modules), this is something which is used by a lot of users and would be > > used. > > Many modules actually have bugs in there remove paths and crash, > this is really not a common case. Sometimes users use rmmod + modprobe > surrounding suspend/resume for e.g. a wifi driver to work around > suspend/resume problems but I have never heard of this being used > for a driver like i2c-i801. > > And again if users are manually meddling with this, the they can > also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801. Argument that other modules have bugs in some code path does not mean to introduce bugs also into other modules. I do not take it. > >> The i2c-i801 driver gets loaded on every x86 system and it is > >> undesirable to have this extra code and the DMI table in RAM > >> on all those other systems. > > > > I think we can take an assumption that ACPI SMO device does not change > > it existence or ACPI enabled/disabled state during runtime. So we can > > scan for ACPI SMO device just once in function stored in __init section > > called during the kernel/module initialization and cache the result > > (bool if device was found + its i2c address). After function marked as > > __init finish its job then together with DMI tables can be discarded > > from RAM. With this way it does take extra memory on every x86 system. > > Also we can combine this with an SMO config option, so the whole code > > "glue" code would not be compiled when SMO driver is not enabled via > > Kconfig. > > This approach does not work because i2c-i801.c registers a PCI driver, > there is no guarantee that the adapter has already been probed and > an i2c_adapter has been created before it. A PCI driver's probe() > function must not be __init and thus any code which it calls also > must not be __init. > > So the majority of the smo88xx handling can not be __init. This argument is wrong. smo88xx has nothing with PCI, has even nothing with i2c. The detection is purely ACPI based and this can be called at any time after ACPI initialization. Detection does not need PCI. There is no reason why it cannot be called in __init section after ACPI is done.
Hi, On 2/29/24 21:46, Pali Rohár wrote: > On Wednesday 28 February 2024 13:50:27 Hans de Goede wrote: >> Hi, >> >> On 2/27/24 23:37, Andy Shevchenko wrote: >>> On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote: >>>> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: >>>>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: >>> >>> ... >>> >>>>> I'm wondering why we need all this. We have notifiers when a device is >>>>> added / removed. We can provide a board_info for the device and attach >>>>> it to the proper adapter, no? >>>> >>>> I do not know how flexible are notifiers. Can notifier call our callback >>>> when new "struct i2c_adapter *adapter" was instanced? >>> >>> You can follow notifications of *an* I2C adapter being added / >>> removed. With that, you can filter which one is that. Based on that >>> you may attach a saved (at __init as you talked about in the reply to >>> Hans) board_info with all necessary information. >>> >>> Something like this (combined) >>> https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515 >>> https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194 >> >> drivers/platform/x86/touchscreen_dmi.c actually already does something >> like this for i2c-clients. The problem is that this brings probe-ordering >> problems with it. If the i801 driver is loaded before the dell-smo8800 >> driver then the notifiers will not trigger since the i2c-adapter has >> already been created (1). >> >> So we would still need a "cold-plug" manual scan in smo8800_probe() >> anyways at which point we might as well just return -EPROBE_DEFER >> when the adapter is not there. > > And that it example why the current existing solution is better, it does > not have such problems like the proposed. > >> As for Pali's suggestion of having the i2c-i801 code call a symbol >> exported by dell-smo8800 > > I did not suggest that! Please do not make false statements about me. In: https://lore.kernel.org/platform-driver-x86/20240227210429.l5o52wuexqqmrpol@pali/ You write: "Location of the quirks can be moved outside of the i2c-i801.c source code relatively easily without need to change the way how parent--child relationship currently works. Relevant functions is_dell_system_with_lis3lv02d() and register_dell_lis3lv02d_i2c_device() does not use internals of i2c-i801 and could be moved into new file, lets say drivers/platform/x86/dell/dell-smo8800-plat.c" Ok I see now you suggest moving the code to a new file, sorry for misreading that. But the point is that moving the code does not help because since there is a symbol used from the new code it will still get loaded on all machines were the i2c-i801.c driver gets loaded. So it will still be taking up RAM and increases boot time (loading an extra module consumes time) on machines which do not have any SMO88xx devices. And that point is still valid even independent of the code is moved to the existing dell-smo8800 module or to a new dell-smo8800-plat module. Regards, Hans
On Saturday 02 March 2024 12:02:39 Hans de Goede wrote: > But the point is that moving the code does not help because > since there is a symbol used from the new code it will still > get loaded on all machines were the i2c-i801.c driver gets > loaded. So it will still be taking up RAM and increases > boot time (loading an extra module consumes time) on machines > which do not have any SMO88xx devices. > > And that point is still valid even independent of the code > is moved to the existing dell-smo8800 module or to a new > dell-smo8800-plat module. This is silly argument if you are opposing to adding trivial exported function with input argument struct i2c_adapter *adapter and with body if (smo88xx_detected) i2c_new_client_device(adapter, &info); with two static global variables: struct i2c_board_info info; bool smo88xx_detected; will be compiled and loaded on all x86 machines and taking too much RAM. Because that design with notifiers and other things would eat much more RAM and would be also slower. As I said in previous emails, detection (and so filling those two above static global variables) can be filled in the __init section and so would not take after bootup. For detection it is is needed to just call dmi_match(), acpi_get_devices() and dmi_get_system_info() which can be done in __init section. I do not see reason why not.
Hi, On 2/29/24 21:57, Pali Rohár wrote: > On Wednesday 28 February 2024 14:10:14 Hans de Goede wrote: >>>>> Now after looking at this change again I see there a problem. If i2c-801 >>>>> driver initialize i2c-801 device after this smo8800 is called then >>>>> accelerometer i2c device would not happen. >>>> >>>> That is a good point (which Jean also points out). But this can simply >>>> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER >>>> if the i2c-i801 i2c-bus is not present yet (all designs using the >>>> dell-smo8800 driver will have an i2c-bus so waiting for this to show >>>> up should not cause regressions). >>> >>> Adding EPROBE_DEFER just complicates the dependency and state model. >>> I would really suggest to come up with a simpler solution, not too >>> complicated where it is required to think a lot if is is correct and if >>> all edge-cases are handled. >> >> I'm not sure what you are worried about here. dell-smo8800 is >> a leave driver, nothing inside the kernel depends on it being >> loaded or not. So there are no -EPROBE_DEFER complexities here, >> yes -EPROBE_DEFER can become a bit hairy with complex dependency >> graphs, but this is a very KISS case. >> >> Are there any specific scenarios you are actually worried about >> in this specific case? > > -EPROBE_DEFER restarts and schedule probing of the device later. It does > not inform device manager when it can try do it. So it can try probing > it many more times until it decide to not try it again. "until it decide to not try it again" is not how the kernel's EPROBE_DEFER mechanism works. It will queue a new re-probe of all devices on the deferred probe list whenever another driver's probe() method succeeds. So once i801_probe() returns success, the dell-smo8800 driver's probe() will be tried again and at that point the i2c-i801 i2c_adapter exists and it will succeed. Yes the dell-smo8800 driver's probe() may be called multiple times before i801_probe(), but that is not an issue. It is guaranteed that the dell-smo8800 driver's probe() will be called at least once after i801_probe() succeeds. > This > asynchronous design is broken and I do not see reason why to use it in > another driver EPROBE_DEFER is used in other cases on x86 platforms too and it is used a whole lot on ARM platforms. If you consider EPROBE_DEFER fundamentally broken then that is a whole other discussion and frankly that is out of scope for this discussion. EPROBE_DEFER is a widely used and proven mechanism. Arguing that this patch cannot move forward because EPROBE_DEFER has generic issues really is out of scope. >>>> If we can agree to move forward this series I'll fix this. >>>> >>>> Pali wrote: >>>>> Also it has same problem if PCI i801 device is reloaded or reset. >>>> >>>> The i801 device is not hotplugable, so normally this will never >>>> happen. If the user manually unbinds + rebinds the i2c-i801 driver >>>> them the i2c_client for the smo88xx device will indeed get removed >>>> and not re-added. But this will normally never happen and if >>>> a user is manually poking things then the user can also unbind + >>>> rebind the dell-mso8800 driver after the i2c-i801 rebind. >>>> So I don't really see this as an issue. >>> >>> Well, rmmod & modprobe is not the rare cases. Whatever developers say >>> about rmmod (or modprobe -r or whatever is the way for unloading >>> modules), this is something which is used by a lot of users and would be >>> used. >> >> Many modules actually have bugs in there remove paths and crash, >> this is really not a common case. Sometimes users use rmmod + modprobe >> surrounding suspend/resume for e.g. a wifi driver to work around >> suspend/resume problems but I have never heard of this being used >> for a driver like i2c-i801. >> >> And again if users are manually meddling with this, the they can >> also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801. > > Argument that other modules have bugs in some code path does not mean to > introduce bugs also into other modules. I do not take it. My remark about many modules having bugs in their remove() path was to counter your argument that people do manual rmmod-s all the time. But how many people do or do not do manual rmmods is not the fundamental point here. The fundamental point is that if users make manual rmmod calls then they already need to also manually undo the results of the rmmod call. So now they will also need to reload dell-smo8800 driver as part of the manual undoing. I really don't see a problem with that. Users should not be unloading (and 99% is not unloading) the i2c-i801 driver in the first place. >>>> The i2c-i801 driver gets loaded on every x86 system and it is >>>> undesirable to have this extra code and the DMI table in RAM >>>> on all those other systems. >>> >>> I think we can take an assumption that ACPI SMO device does not change >>> it existence or ACPI enabled/disabled state during runtime. So we can >>> scan for ACPI SMO device just once in function stored in __init section >>> called during the kernel/module initialization and cache the result >>> (bool if device was found + its i2c address). After function marked as >>> __init finish its job then together with DMI tables can be discarded >>> from RAM. With this way it does take extra memory on every x86 system. >>> Also we can combine this with an SMO config option, so the whole code >>> "glue" code would not be compiled when SMO driver is not enabled via >>> Kconfig. >> >> This approach does not work because i2c-i801.c registers a PCI driver, >> there is no guarantee that the adapter has already been probed and >> an i2c_adapter has been created before it. A PCI driver's probe() >> function must not be __init and thus any code which it calls also >> must not be __init. >> >> So the majority of the smo88xx handling can not be __init. > > This argument is wrong. smo88xx has nothing with PCI, has even nothing > with i2c. The detection is purely ACPI based and this can be called at > any time after ACPI initialization. Detection does not need PCI. There > is no reason why it cannot be called in __init section after ACPI is > done. My patch series adds support for probing the i2c-address to make it easier for users to check what the address of the lis3lv02d chip on their laptop model is. This probing requires access to the actual i2c_adapter which is a PCI device. So this can only run after the PCI-driver for the i2c-i801 bus has bound, which means after the probe() from the PCI driver so it cannot be __init code. Pali I'm getting the feeling that you have dug in your heels that: 1. Current approach is good 2. Hans' new approach is bad And that you are not really given my arguments why moving the code out of the i2c-i801 driver is a good idea a fair hearing. I would like you to try and take some distance from this and look at this with more of a helicopter view. As I mentioned earlier in the thread and as Andy has agreed with my main motivation for moving the handling of the i2c_client instantation is that this is a SMO88xx ACPI device specific kludge and as such IMHO thus belongs in the driver for the SMO88xx ACPI platform_device. Had I been involved in (and have the knowledge of kernel internals I have now) the original i2c-i801.c SMO88xx ACPI device changes then I would likely have nacked them. Putting this sort of highly device specific code into generic drivers like the i2c-i801 code does not scale. What if tomorrow we find some other ACPI device with similar issues are we then going to add yet another kludge to the generic shared i2c-i801 code ? Also note that the i2c-i801.c code already is triggered by the presence of certain ACPI hw-ids and we already have a mechanism to only load code based on ACPI hw-ids (1), that is have a platform_driver with an acpi_match table for those ids, which is exactly the mechanism my new approach is using. From a design perspective the handling of all of this *very obviously* belongs in a driver actually binding to these ACPI ids and my suggested changes are actually following this, what IMHO is the only proper way to handle this. Now if there were big problems with my suggested approach then I could understand your reluctance. But the only real problem you have pointed out is that if people *manually* rmmod i2c-i801 that then after *manually* modprobing i2c-i801 again the i2c_client for the lis3lv02d chip is not automatically re-instated, instead they will need to also manually reload the dell-smo8800 driver. Which IMHO really is not an issue since they are already manually messing with drivers anyways. And note that even that problem could be fixed by using bus-notifiers as Andy suggested. IMHO using notifiers here is overkill. But if you are ok with moving this code out of i2c-i801 and intel dell-smo8800 if I use notifiers in the next version so that things will keep working even after a *manual* rmmod of i2c-i801 then I'll do so for v3 of the patchset. Regards, Hans 1) The fact alone that the old approach requires manually syncing the 2 copies of the ACPI hw-id tables already indicates that the i2c-i801 code is not the right place for this functionality.
On Sat, Mar 2, 2024 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 2/29/24 21:57, Pali Rohár wrote: ... > But the only real problem you have pointed out is that > if people *manually* rmmod i2c-i801 that then after *manually* > modprobing i2c-i801 again the i2c_client for the lis3lv02d chip > is not automatically re-instated, instead they will need to > also manually reload the dell-smo8800 driver. Which IMHO > really is not an issue since they are already manually messing > with drivers anyways. Actually we probably can add some code to the i2c core to list the clients that were instantiated in a way that can't be done again after removing the respective bus driver. But this is just an idea on how to inform users what exactly can go wrong after the rmmod/modprobe cycle, I haven't investigated this anyhow. > And note that even that problem could be fixed by using > bus-notifiers as Andy suggested. IMHO using notifiers here is > overkill. But if you are ok with moving this code out of i2c-i801 > and intel dell-smo8800 if I use notifiers in the next version so > that things will keep working even after a *manual* rmmod of > i2c-i801 then I'll do so for v3 of the patchset.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 070999139c6d..595e263ba623 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -975,6 +975,10 @@ static const struct i2c_algorithm smbus_algorithm = { #define FEATURES_ICH4 (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \ FEATURE_HOST_NOTIFY) +/* + * NOTE: If new models with FEATURE_IDF are added please also update + * i801_idf_ids[] in drivers/platform/x86/dell-smo8800.c + */ static const struct pci_device_id i801_ids[] = { { PCI_DEVICE_DATA(INTEL, 82801AA_3, 0) }, { PCI_DEVICE_DATA(INTEL, 82801AB_3, 0) }, @@ -1141,125 +1145,6 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) } } -/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */ -static const char *const acpi_smo8800_ids[] = { - "SMO8800", - "SMO8801", - "SMO8810", - "SMO8811", - "SMO8820", - "SMO8821", - "SMO8830", - "SMO8831", -}; - -static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, - u32 nesting_level, - void *context, - void **return_value) -{ - struct acpi_device_info *info; - acpi_status status; - char *hid; - int i; - - status = acpi_get_object_info(obj_handle, &info); - if (ACPI_FAILURE(status)) - return AE_OK; - - if (!(info->valid & ACPI_VALID_HID)) - goto smo88xx_not_found; - - hid = info->hardware_id.string; - if (!hid) - goto smo88xx_not_found; - - i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid); - if (i < 0) - goto smo88xx_not_found; - - kfree(info); - - *return_value = NULL; - return AE_CTRL_TERMINATE; - -smo88xx_not_found: - kfree(info); - return AE_OK; -} - -static bool is_dell_system_with_lis3lv02d(void) -{ - void *err = ERR_PTR(-ENOENT); - - if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) - return false; - - /* - * Check that ACPI device SMO88xx is present and is functioning. - * Function acpi_get_devices() already filters all ACPI devices - * which are not present or are not functioning. - * ACPI device SMO88xx represents our ST microelectronics lis3lv02d - * accelerometer but unfortunately ACPI does not provide any other - * information (like I2C address). - */ - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); - - return !IS_ERR(err); -} - -/* - * Accelerometer's I2C address is not specified in DMI nor ACPI, - * so it is needed to define mapping table based on DMI product names. - */ -static const struct { - const char *dmi_product_name; - unsigned short i2c_addr; -} dell_lis3lv02d_devices[] = { - /* - * Dell platform team told us that these Latitude devices have - * ST microelectronics accelerometer at I2C address 0x29. - */ - { "Latitude E5250", 0x29 }, - { "Latitude E5450", 0x29 }, - { "Latitude E5550", 0x29 }, - { "Latitude E6440", 0x29 }, - { "Latitude E6440 ATG", 0x29 }, - { "Latitude E6540", 0x29 }, - /* - * Additional individual entries were added after verification. - */ - { "Latitude 5480", 0x29 }, - { "Vostro V131", 0x1d }, - { "Vostro 5568", 0x29 }, -}; - -static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) -{ - struct i2c_board_info info; - const char *dmi_product_name; - int i; - - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); - for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { - if (strcmp(dmi_product_name, - dell_lis3lv02d_devices[i].dmi_product_name) == 0) - break; - } - - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { - dev_warn(&priv->pci_dev->dev, - "Accelerometer lis3lv02d is present on SMBus but its" - " address is unknown, skipping registration\n"); - return; - } - - memset(&info, 0, sizeof(struct i2c_board_info)); - info.addr = dell_lis3lv02d_devices[i].i2c_addr; - strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); - i2c_new_client_device(&priv->adapter, &info); -} - /* Register optional slaves */ static void i801_probe_optional_slaves(struct i801_priv *priv) { @@ -1279,9 +1164,6 @@ static void i801_probe_optional_slaves(struct i801_priv *priv) if (dmi_name_in_vendors("FUJITSU")) dmi_walk(dmi_check_onboard_devices, &priv->adapter); - if (is_dell_system_with_lis3lv02d()) - register_dell_lis3lv02d_i2c_device(priv); - /* Instantiate SPD EEPROMs unless the SMBus is multiplexed */ #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) if (!priv->mux_pdev) diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c index 87339cc78880..c674e3392270 100644 --- a/drivers/platform/x86/dell/dell-smo8800.c +++ b/drivers/platform/x86/dell/dell-smo8800.c @@ -4,18 +4,23 @@ * * Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com> * Copyright (C) 2014 Pali Rohár <pali@kernel.org> + * Copyright (C) 2023 Hans de Goede <hansg@kernel.org> * * This is loosely based on lis3lv02d driver. */ #define DRIVER_NAME "smo8800" +#include <linux/device/bus.h> +#include <linux/dmi.h> #include <linux/fs.h> +#include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/miscdevice.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/pci.h> #include <linux/platform_device.h> #include <linux/uaccess.h> @@ -26,6 +31,7 @@ struct smo8800_device { unsigned long misc_opened; /* whether the device is open */ wait_queue_head_t misc_wait; /* Wait queue for the misc dev */ struct device *dev; /* acpi device */ + struct i2c_client *i2c_dev; /* i2c_client for lis3lv02d */ }; static irqreturn_t smo8800_interrupt_quick(int irq, void *data) @@ -103,6 +109,111 @@ static const struct file_operations smo8800_misc_fops = { .release = smo8800_misc_release, }; +/* + * On 2 older PCH generations, Patsburg (for Sandy Bridge and Ivybridge) and + * Wellsburg (for Haswell and Broadwell), the PCH has 3 extra i2c-i801 + * compatible SMBusses called 'Integrated Device Function' busses. These have + * FEATURE_IDF set in the i801_ids[] table in i2c-i801.c. + * The ST microelectronics accelerometer is connected to the main SMBus + * so the IDF controllers should be skipped. + */ +static const struct pci_device_id i801_idf_ids[] = { + { PCI_VDEVICE(INTEL, 0x1d70) }, /* Patsburg IFD0 */ + { PCI_VDEVICE(INTEL, 0x1d71) }, /* Patsburg IFD1 */ + { PCI_VDEVICE(INTEL, 0x1d72) }, /* Patsburg IFD2 */ + { PCI_VDEVICE(INTEL, 0x8d7d) }, /* Wellsburg MS0 */ + { PCI_VDEVICE(INTEL, 0x8d7e) }, /* Wellsburg MS1 */ + { PCI_VDEVICE(INTEL, 0x8d7f) }, /* Wellsburg MS2 */ + {} +}; + +static int smo8800_find_i801(struct device *dev, void *data) +{ + struct i2c_adapter *adap, **adap_ret = data; + + adap = i2c_verify_adapter(dev); + if (!adap) + return 0; + + if (!strstarts(adap->name, "SMBus I801 adapter")) + return 0; + + if (pci_match_id(i801_idf_ids, to_pci_dev(adap->dev.parent))) + return 0; /* Only register client on main SMBus channel */ + + *adap_ret = i2c_get_adapter(adap->nr); + return 1; +} + +/* + * Accelerometer's I2C address is not specified in DMI nor ACPI, + * so it is needed to define mapping table based on DMI product names. + */ +static const struct { + const char *dmi_product_name; + unsigned short i2c_addr; +} dell_lis3lv02d_devices[] = { + /* + * Dell platform team told us that these Latitude devices have + * ST microelectronics accelerometer at I2C address 0x29. + */ + { "Latitude E5250", 0x29 }, + { "Latitude E5450", 0x29 }, + { "Latitude E5550", 0x29 }, + { "Latitude E6440", 0x29 }, + { "Latitude E6440 ATG", 0x29 }, + { "Latitude E6540", 0x29 }, + /* + * Additional individual entries were added after verification. + */ + { "Latitude 5480", 0x29 }, + { "Vostro V131", 0x1d }, + { "Vostro 5568", 0x29 }, +}; + +static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) +{ + struct i2c_board_info info = { }; + struct i2c_adapter *adap = NULL; + const char *dmi_product_name; + u8 addr = 0; + int i; + + bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801); + if (!adap) + return; + + dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { + if (strcmp(dmi_product_name, + dell_lis3lv02d_devices[i].dmi_product_name) == 0) { + addr = dell_lis3lv02d_devices[i].i2c_addr; + break; + } + } + + if (!addr) { + dev_warn(smo8800->dev, + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); + goto put_adapter; + } + + info.addr = addr; + strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); + + smo8800->i2c_dev = i2c_new_client_device(adap, &info); + if (IS_ERR(smo8800->i2c_dev)) { + dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev), + "registering accel i2c_client\n"); + smo8800->i2c_dev = NULL; + } else { + dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", + info.type, info.addr); + } +put_adapter: + i2c_put_adapter(adap); +} + static int smo8800_probe(struct platform_device *device) { int err; @@ -126,10 +237,12 @@ static int smo8800_probe(struct platform_device *device) return err; smo8800->irq = err; + smo8800_instantiate_i2c_client(smo8800); + err = misc_register(&smo8800->miscdev); if (err) { dev_err(&device->dev, "failed to register misc dev: %d\n", err); - return err; + goto error_unregister_i2c_client; } err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, @@ -150,6 +263,8 @@ static int smo8800_probe(struct platform_device *device) error: misc_deregister(&smo8800->miscdev); +error_unregister_i2c_client: + i2c_unregister_device(smo8800->i2c_dev); return err; } @@ -160,9 +275,9 @@ static void smo8800_remove(struct platform_device *device) free_irq(smo8800->irq, smo8800); misc_deregister(&smo8800->miscdev); dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); + i2c_unregister_device(smo8800->i2c_dev); } -/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */ static const struct acpi_device_id smo8800_ids[] = { { "SMO8800", 0 }, { "SMO8801", 0 }, @@ -189,3 +304,5 @@ module_platform_driver(smo8800_driver); MODULE_DESCRIPTION("Dell Latitude freefall driver (ACPI SMO88XX)"); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Sonal Santan, Pali Rohár"); +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */ +MODULE_SOFTDEP("pre: i2c-i801");
It is not necessary to handle the Dell specific instantiation of i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource inside the generic i801 I2C adapter driver. The kernel already instantiates platform_device-s for these ACPI devices and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these platform drivers. Move the i2c_client instantiation from the generic i2c-i801 driver to the Dell specific dell-smo8800 driver. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses - Add a comment documenting the IDF PCI device ids --- drivers/i2c/busses/i2c-i801.c | 126 +---------------------- drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++- 2 files changed, 123 insertions(+), 124 deletions(-)