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.
