Re: [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

Hans de Goede
2024-03-02T11:38:32Z

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.