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@xxxxxxxxxx> >>>>> --- >>>>> 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
