Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 63BD86016D
Subject: Re: [PATCH] ipmi: Remove ACPI SPMI probing from the SSIF (I2C) driver
To:     Corey Minyard <cminyard@mvista.com>, minyard@acm.org
Cc:     openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
References: <1520543298-18163-1-git-send-email-minyard@acm.org>
 <ea095e0f-023b-dca2-90da-cc95782a6fb0@codeaurora.org>
 <e16a9133-e8d7-537c-7cf4-e0a4caa6fdee@mvista.com>
From:   Jiandi An <anjiandi@codeaurora.org>
Message-ID: <e572ebc2-f122-64ec-3417-92b0fd6371c3@codeaurora.org>
Date:   Sun, 11 Mar 2018 22:40:24 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <e16a9133-e8d7-537c-7cf4-e0a4caa6fdee@mvista.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 03/09/2018 08:42 PM, Corey Minyard wrote:
> On 03/09/2018 04:28 PM, Jiandi An wrote:
>>
>>
>> On 03/08/2018 03:08 PM, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> The IPMI spec states:
>>>
>>>    The purpose of the SPMI Table is to provide a mechanism that can
>>>    be used by the OSPM (an ACPI term for “OS Operating System-directed
>>>    configuration and Power Management” essentially meaning an ACPI-aware
>>>    OS or OS loader) very early in the boot process, e.g., before the
>>>    ability to execute ACPI control methods in the OS is available.
>>>
>>> When we are probing IPMI in Linux, ACPI control methods are available,
>>> so we shouldn't be probing using SPMI.  It could cause some confusion
>>> during the probing process.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Cc: Jiandi An <anjiandi@codeaurora.org>
>>> ---
>>>
>>> Jiandi, this just yanks out the SPMI code.  Your platform
>>> should have an ACPI control method in it's namespace specifying the
>>> IPMI interface.  If it doesn't, that's a bug in your platform and
>>> really needs to be fixed.  Finding IPMI devices with SMBIOS tables
>>> is kind of risky since there is no real way to know which I2C bus
>>> has the IPMI device if your system has more than one I2C bus. With
>>> an ACPI control method, the IPMI control method will be inside the
>>> I2C bus control method, so it will be unambiguous.
>>
>> Thanks Corey.  I tested this patch.  It works for us through the ACPI
>> control method.
> 
> Thanks.  May I add a "Tested-by" for you?

Tested-by: Jiandi An <anjiandi@codeaurora.org>

> 
>> By the way, FYI the ipmi_si driver is also having the
>> SPMI probe code.
>>
>> [   17.370835] ipmi_si: probing via SPMI
>>
> 
> Yes, I've removed that in a separate patch.
> 
> -corey
> 
>> Thanks
>> - Jiandi
>>
>>>
>>>   drivers/char/ipmi/ipmi_ssif.c | 105 
>>> ------------------------------------------
>>>   1 file changed, 105 deletions(-)
>>>
>>> diff --git a/drivers/char/ipmi/ipmi_ssif.c 
>>> b/drivers/char/ipmi/ipmi_ssif.c
>>> index 4cff4cd..206257b 100644
>>> --- a/drivers/char/ipmi/ipmi_ssif.c
>>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>>> @@ -1994,108 +1994,6 @@ static const struct acpi_device_id 
>>> ssif_acpi_match[] = {
>>>       { },
>>>   };
>>>   MODULE_DEVICE_TABLE(acpi, ssif_acpi_match);
>>> -
>>> -/*
>>> - * Once we get an ACPI failure, we don't try any more, because we go
>>> - * through the tables sequentially.  Once we don't find a table, there
>>> - * are no more.
>>> - */
>>> -static int acpi_failure;
>>> -
>>> -/*
>>> - * Defined in the IPMI 2.0 spec.
>>> - */
>>> -struct SPMITable {
>>> -    s8    Signature[4];
>>> -    u32    Length;
>>> -    u8    Revision;
>>> -    u8    Checksum;
>>> -    s8    OEMID[6];
>>> -    s8    OEMTableID[8];
>>> -    s8    OEMRevision[4];
>>> -    s8    CreatorID[4];
>>> -    s8    CreatorRevision[4];
>>> -    u8    InterfaceType;
>>> -    u8    IPMIlegacy;
>>> -    s16    SpecificationRevision;
>>> -
>>> -    /*
>>> -     * Bit 0 - SCI interrupt supported
>>> -     * Bit 1 - I/O APIC/SAPIC
>>> -     */
>>> -    u8    InterruptType;
>>> -
>>> -    /*
>>> -     * If bit 0 of InterruptType is set, then this is the SCI
>>> -     * interrupt in the GPEx_STS register.
>>> -     */
>>> -    u8    GPE;
>>> -
>>> -    s16    Reserved;
>>> -
>>> -    /*
>>> -     * If bit 1 of InterruptType is set, then this is the I/O
>>> -     * APIC/SAPIC interrupt.
>>> -     */
>>> -    u32    GlobalSystemInterrupt;
>>> -
>>> -    /* The actual register address. */
>>> -    struct acpi_generic_address addr;
>>> -
>>> -    u8    UID[4];
>>> -
>>> -    s8      spmi_id[1]; /* A '\0' terminated array starts here. */
>>> -};
>>> -
>>> -static int try_init_spmi(struct SPMITable *spmi)
>>> -{
>>> -    unsigned short myaddr;
>>> -
>>> -    if (num_addrs >= MAX_SSIF_BMCS)
>>> -        return -1;
>>> -
>>> -    if (spmi->IPMIlegacy != 1) {
>>> -        pr_warn("IPMI: Bad SPMI legacy: %d\n", spmi->IPMIlegacy);
>>> -        return -ENODEV;
>>> -    }
>>> -
>>> -    if (spmi->InterfaceType != 4)
>>> -        return -ENODEV;
>>> -
>>> -    if (spmi->addr.space_id != ACPI_ADR_SPACE_SMBUS) {
>>> -        pr_warn(PFX "Invalid ACPI SSIF I/O Address type: %d\n",
>>> -            spmi->addr.space_id);
>>> -        return -EIO;
>>> -    }
>>> -
>>> -    myaddr = spmi->addr.address & 0x7f;
>>> -
>>> -    return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
>>> -}
>>> -
>>> -static void spmi_find_bmc(void)
>>> -{
>>> -    acpi_status      status;
>>> -    struct SPMITable *spmi;
>>> -    int              i;
>>> -
>>> -    if (acpi_disabled)
>>> -        return;
>>> -
>>> -    if (acpi_failure)
>>> -        return;
>>> -
>>> -    for (i = 0; ; i++) {
>>> -        status = acpi_get_table(ACPI_SIG_SPMI, i+1,
>>> -                    (struct acpi_table_header **)&spmi);
>>> -        if (status != AE_OK)
>>> -            return;
>>> -
>>> -        try_init_spmi(spmi);
>>> -    }
>>> -}
>>> -#else
>>> -static void spmi_find_bmc(void) { }
>>>   #endif
>>>     #ifdef CONFIG_DMI
>>> @@ -2200,9 +2098,6 @@ static int init_ipmi_ssif(void)
>>>           ssif_i2c_driver.driver.acpi_match_table    =
>>>               ACPI_PTR(ssif_acpi_match);
>>>   -    if (ssif_tryacpi)
>>> -        spmi_find_bmc();
>>> -
>>>       if (ssif_trydmi) {
>>>           rv = platform_driver_register(&ipmi_driver);
>>>           if (rv)
>>>
>>
> 

-- 
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.