| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d3ff0287-56fa-de95-6de9-61c84d848b1c@codeaurora.org>
Date: Thu, 8 Mar 2018 12:18:41 -0600
From: Jiandi An <anjiandi@...eaurora.org>
To: minyard@....org, arnd@...db.de, gregkh@...uxfoundation.org
Cc: openipmi-developer@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipmi:ssif: Fix double probe from tryacpi and trydmi
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@...eaurora.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.
Powered by blists - more mailing lists