Re: [PATCH] ipmi:ssif: Fix double probe from tryacpi and trydmi

[Jiandi An <anjiandi@codeaurora.org>]

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.