Re: [PATCH] ipmi: Remove ACPI SPMI probing from the SSIF (I2C) driver

[Jiandi An]

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@xxxxxxx wrote:
> > > From: Corey Minyard <cminyard@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Cc: Jiandi An <anjiandi@xxxxxxxxxxxxxx>
> > > ---
> > >
> > > 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@xxxxxxxxxxxxxx>

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