Messages in this thread | | | Subject | Re: [PATCH] ipmi:ssif: Fix double probe from tryacpi and trydmi | From | Jiandi An <> | Date | Thu, 8 Mar 2018 12:18:41 -0600 |
| |
On 03/08/2018 08:10 AM, Corey Minyard wrote: > On 03/07/2018 05:59 PM, Jiandi An wrote: >> >> >> On 03/07/2018 07:34 AM, Corey Minyard wrote: >>> On 03/06/2018 11:49 PM, Jiandi An wrote: >>>> IPMI SSIF driver's parameter tryacpi and trydmi both >>>> are set to true. The addition of IPMI DMI driver to >>>> create platform device for each IPMI device causes >>>> SSIF probe to be done twice on the same SMB I2C address >>>> for BMC. Fix is to not call trydmi if tryacpi is able >>>> to find I2C address for BMC from SPMI ACPI table and >>>> probe successfully. >>> >>> Why are you trying to do this? Is something bad happening? >>> >>> SPMI is not the preferred mechanism for detecting a device, >>> the preferred mechanism should be the acpi match table or >>> OF, followed by DMI, followed by SPMI. In fact, it might be >>> best to just remove SPMI. >>> >>> -corey >> >> >> On our ARM64 platform, SSIF is the IPMI interface for host to >> BMC communication and it is described in ACPI SPMI table including >> the I2C address. The driver would get the SSIF device from >> IPI0001 ssif_acpi_match[] and probe. It worked fine with no issues. >> >> Then it was reported dmidecode does not show the correct SSIF >> device information including correct I2C address. So SSIF device >> description is also added in SMBIOS table. It worked fine with no >> issues until this patch. >> >> 0944d88 ipmi: Convert DMI handling over to a platform device >> >> We would see error message indicating trydmi via >> platform_driver_register fails with -EEXIST during boot. >> >> [ 9.385758] ipmi_ssif: probe of dmi-ipmi-ssif.0 failed with error -17 >> >> This is because tryacpi ran first and successfully completed >> new_ssif_client() (which adds address to ssif_info) and eventually >> ssif_probe() >> >> ssif_tryacpi >> spmi_find_bmc() >> try_init_spmi() >> new_ssif_client() >> >> Since both tryacpi and trydmi are set to true as module parameter >> with no permission and not exposed to /sys/module/ipmi_ssif/parameters, >> it triggers the following path down to dmi_ipmi_probe() and >> new_ssif_client() which fails ssif_info_find() finds the address >> added to ssif_info already in the ssif_tryacpi path. >> >> ssif_trydmi >> platform_driver_register(&ipmi_driver) >> __platform_driver_register() >> driver_register() >> bus_add_driver() >> driver_attach() >> bus_for_each_dev() >> __driver_attach() >> driver_probe_device() >> ssif_platform_probe() >> dmi_ipmi_probe() >> new_ssif_client() >> >> Are you suggesting to not do tryacpi at all and just rely on >> trydmi? > > Basically, yes. SPMI is really designed for early detection of interfaces > before ACPI proper comes up. You should have the IPMI device in your > ACPI tree.
You meant to say I should have the IPMI SSIF device in my SMBIOS table? Or do you mean to say I should have the IPMI SSIF device in my ACPI SPMI table but you will remove SPMI support from the IPMI driver?
Do you want me to remove the ssif_tryacpi logic and tryacpi module parameter all together in that patch?
Thanks -Jiandi
> > My inclination is to remove SPMI support from the IPMI driver. > > -corey > >> >> I was looking at the following patch to understand more about >> the added ipmi_dmi driver. >> >> 9f88145 ipmi: Create a platform device for a DMI-specified IPMI interface >> >> It's creating a platform device for each IPMI device in the DMI >> table including SSIF device, for auto-loading IPMI devices from >> SMBIOS tables. >> >> Are you suggesting removing SSIF device description from ACPI >> SPMI table and remove ssif_tryacpi logic all together? >> >> But the commit description mentions ... >> >> "This also adds the ability to extract the slave address from >> the SMBIOS tables, so that when the driver uses ACPI-specified >> interfaces, it can still extract the slave address from SMBIOS." >> >> This made me think SSIF driver can still use ACPI-specified >> interface. It made me think it implies SSIF device can be >> described in ACPI SPMI table and SMBIOS table. Both spec >> did not say they cannot. >> >> What's your recommended way of fixing this double probing? >> >> Thanks >> >> >>> >>>> Signed-off-by: Jiandi An <anjiandi@codeaurora.org> >>>> --- >>>> drivers/char/ipmi/ipmi_ssif.c | 35 >>>> ++++++++++++++++++++++++----------- >>>> 1 file changed, 24 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/char/ipmi/ipmi_ssif.c >>>> b/drivers/char/ipmi/ipmi_ssif.c >>>> index 9d3b0fa..5c57363 100644 >>>> --- a/drivers/char/ipmi/ipmi_ssif.c >>>> +++ b/drivers/char/ipmi/ipmi_ssif.c >>>> @@ -1981,29 +1981,41 @@ static int try_init_spmi(struct SPMITable >>>> *spmi) >>>> return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL); >>>> } >>>> -static void spmi_find_bmc(void) >>>> +static int spmi_find_bmc(void) >>>> { >>>> acpi_status status; >>>> struct SPMITable *spmi; >>>> int i; >>>> + int rc = 0; >>>> if (acpi_disabled) >>>> - return; >>>> + return -EPERM; >>>> if (acpi_failure) >>>> - return; >>>> + return -ENODEV; >>>> for (i = 0; ; i++) { >>>> status = acpi_get_table(ACPI_SIG_SPMI, i+1, >>>> (struct acpi_table_header **)&spmi); >>>> - if (status != AE_OK) >>>> - return; >>>> + if (status != AE_OK) { >>>> + if (i == 0) >>>> + return -ENODEV; >>>> + else >>>> + return 0; >>>> + } >>>> - try_init_spmi(spmi); >>>> + rc = try_init_spmi(spmi); >>>> + if (rc) >>>> + return rc; >>>> } >>>> + >>>> + return 0; >>>> } >>>> #else >>>> -static void spmi_find_bmc(void) { } >>>> +static int spmi_find_bmc(void) >>>> +{ >>>> + return -ENODEV; >>>> +} >>>> #endif >>>> #ifdef CONFIG_DMI >>>> @@ -2104,12 +2116,13 @@ static int init_ipmi_ssif(void) >>>> addr[i]); >>>> } >>>> - if (ssif_tryacpi) >>>> + if (ssif_tryacpi) { >>>> ssif_i2c_driver.driver.acpi_match_table = >>>> ACPI_PTR(ssif_acpi_match); >>>> - >>>> - if (ssif_tryacpi) >>>> - spmi_find_bmc(); >>>> + rv = spmi_find_bmc(); >>>> + if (!rv) >>>> + ssif_trydmi = false; >>>> + } >>>> if (ssif_trydmi) { >>>> rv = platform_driver_register(&ipmi_driver); >>> >>> >> >
-- Jiandi An Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |