On 1/24/19 7:14 PM, Corey Minyard wrote: > On 1/24/19 4:24 AM, Kamlakant Patel wrote: >> Hi Corey, >> >> Thanks for your comments, please find my reply inline. >> >> On 1/23/19 2:16 AM, Corey Minyard wrote: >>> External Email >>> >>> ---------------------------------------------------------------------- >>> On 12/28/18 6:29 AM, Kamlakant Patel wrote: >>>> We have SSIF interface present in both DMI and ACPI tables, sometimes >>>> ssif_probe is called simultaneously from i2c interface (from ACPI) >>>> and while loading ipmi_ssif driver(from DMI table) at kernel boot. >>>> Both try to register the same SSIF interface simultaneously, where it >>>> hits a race. >>>> >>>> In cases where ACPI and SMBIOS both are available, we need to prefer ACPI >>>> over SMBIOS so that ACPI functions work properly if they use IPMI. >>>> So if we get an ACPI interface and have already registered an SMBIOS >>>> interface >>>> at the same address, we need to remove the SMBIOS one and add the ACPI one. >>> >>> Sorry, I completely forgot about this. >>> >>> This is mostly good, a few comments inline. >>> >>> >>>> Signed-off-by: Kamlakant Patel <[email protected]> >>>> --- >>>> drivers/char/ipmi/ipmi_ssif.c | 52 +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 52 insertions(+) >>>> >>>> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c >>>> index ca9528c4f183..8d60e5cd2e17 100644 >>>> --- a/drivers/char/ipmi/ipmi_ssif.c >>>> +++ b/drivers/char/ipmi/ipmi_ssif.c >>>> @@ -1146,6 +1146,7 @@ MODULE_PARM_DESC(trydmi, "Setting this to zero will >>>> disable the default scan of >>>> >>>> static DEFINE_MUTEX(ssif_infos_mutex); >>>> static LIST_HEAD(ssif_infos); >>>> +static LIST_HEAD(ssif_clients); >>> There is already ssif_infos, how is this different? >> The ssif_infos has a list of clients present in SMBIOS table. >> Where as ssif_clients is to keep list of all probed clients. > > Ah, that's right, ssif_infos is a list of things that have been registered, > not a list of things that are running. My naming there is not so good. > > >>> >>>> >>>> #define IPMI_SSIF_ATTR(name) \ >>>> static ssize_t ipmi_##name##_show(struct device *dev, >>>> \ >>>> @@ -1523,6 +1524,43 @@ static void test_multipart_messages(struct >>>> i2c_client *client, >>>> #define GLOBAL_ENABLES_MASK (IPMI_BMC_EVT_MSG_BUFF | >>>> IPMI_BMC_RCV_MSG_INTR | \ >>>> IPMI_BMC_EVT_MSG_INTR) >>>> >>>> +/* >>>> + * Prefer ACPI over SMBIOS, if both are available. >>>> + * So if we get an ACPI interface and have already registered a SMBIOS >>>> + * interface at the same address, remove the SMBIOS and add the ACPI one. >>>> + */ >>>> +static int ssif_is_registered(struct i2c_client *client) >>>> +{ >>>> + struct ssif_addr_info *info; >>>> + int rv = 0; >>>> + >>>> + list_for_each_entry(info, &ssif_clients, link) { >>>> + if ((info->binfo.addr == client->addr) && >>>> + (!strcmp(info->adapter_name, client->adapter->name))) { >>>> + if (!ACPI_HANDLE(&info->client->dev)) { >>> Instead of this check, can you do "info->client->addr_source == >>> SI_SMBIOS)"? You wouldn't want to override something the user >>> hand-specified, for instance. >> I found that addr_source is not part of client but we have addr_src >> present in ssif_addr_info. I will make use of this and add a check >> "info->addr_src == SI_SMBIOS". >>> >>>> + /* Remove SMBIOS interface */ >>>> + ssif_remove(info->client); > > > Looking at this, are you sure this code works? Your > ssif_is-registered() function > is called from ssif_probe(), which is called by the i2c code when it > registers a > new device. However, if i2c has a device at that address, it should not > call > the probe function, it should ignore the duplicate. So ACPI won't override > DMI if it is already registered. > > Conversely, if ACPI gets registered first, the the DMI registration > comes in, > then the i2c code should ignore the DMI one and everything should be > happy (except ACPI doesn't have the BMC slave address). > We get this issue while kernel boots on our ThunderX2 multi-core processor where ssif_probe is called simultaneously from SSIF code and i2c driver. Please see the dmesg log below with no change in default driver:
[root@localhost ~]# dmesg | grep ipmi [ 38.774743] ipmi device interface [ 38.805006] ipmi_ssif: IPMI SSIF Interface driver [ 38.861979] ipmi_ssif i2c-IPI0001:06: ssif_probe CPU 99 *** [ 38.863655] ipmi_ssif 0-000e: ssif_probe CPU 14 *** [ 38.863658] ipmi_ssif: Trying SMBIOS-specified SSIF interface at i2c address 0xe, adapter xlp9xx-i2c, slave address 0x0 [ 38.869500] ipmi_ssif: Trying ACPI-specified SSIF interface at i2c address 0xe, adapter xlp9xx-i2c, slave address 0x0 [ 38.914530] ipmi_ssif: Unable to clear message flags: -22 7 c7 [ 38.952429] ipmi_ssif: Unable to clear message flags: -22 7 00 [ 38.994734] ipmi_ssif: Error getting global enables: -22 7 00 [ 39.015877] ipmi_ssif 0-000e: IPMI message handler: Found new BMC (man_id: 0x00b3d1, prod_id: 0x0001, dev_id: 0x20) [ 39.377645] ipmi_ssif i2c-IPI0001:06: IPMI message handler: Found new BMC (man_id: 0x00b3d1, prod_id: 0x0001, dev_id: 0x20) [ 39.387863] ipmi_ssif 0-000e: IPMI message handler: BMC returned incorrect response, expected netfn 7 cmd 42, got netfn 7 cmd 1 [ 39.791562] ipmi_si: IPMI System Interface driver Here commands and responses get overwritten and we get errors. And below log is with a mutex_lock(&ssif_infos_mutex) around probe process in the ssif_probe(). In this case both gets registered, SMBIOS as well as ACPI. [root@localhost ~]# dmesg | grep ipmi [ 38.875639] ipmi device interface [ 38.907923] ipmi_ssif: IPMI SSIF Interface driver [ 38.953028] ipmi_ssif 0-000e: ssif_probe CPU 159 *** [ 38.959509] ipmi_ssif: Trying SMBIOS-specified SSIF interface at i2c address 0xe, adapter xlp9xx-i2c, slave address 0x0 [ 38.973051] ipmi_ssif 0-000e: IPMI message handler: Found new BMC (man_id: 0x00b3d1, prod_id: 0x0001, dev_id: 0x20) [ 39.208817] ipmi_ssif i2c-IPI0001:06: ssif_probe 176 *** [ 39.512633] ipmi_ssif: Trying ACPI-specified SSIF interface at i2c address 0xe, adapter xlp9xx-i2c, slave address 0x0 [ 39.533371] ipmi_ssif 0-000e: IPMI message handler: BMC returned incorrect response, expected netfn 7 cmd 42, got netfn 7 cmd 57 [ 39.683159] ipmi_ssif i2c-IPI0001:06: IPMI message handler: Found new BMC (man_id: 0x00b3d1, prod_id: 0x0001, dev_id: 0x20) [ 39.964695] ipmi_si: IPMI System Interface driver Here because of the lock both device gets registered cleanly. But we get two instances of the same ipmi device. > I'm also thinking that ssif_remove() as it is is wrong. If you remove the > adapter driver, it should not remove the item from the ssif_info list, > because if you then re-add the adapter it will be gone and not handled. > Yes, use of ssif_remove() is not done correct here. > Anyway, the trouble is that ACPI devices come in straight from the > i2c driver and DMI devices come in through the SSIF code. It seems to > me that you would have to get the ACPI devices to come through the > SSIF code, too, to make this work right. Since we do not have an ACPI interface available for SSIF as of now. So, with the current scenario I feel what we can do is register the device which comes first either SMBIOS or ACPI and avoid duplicate. Please suggest. Thanks, Kamlakant Patel _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
