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

[Corey Minyard]

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?

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