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.
