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

[Hans de Goede <hdegoede@redhat.com>]

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