It is possible that SSIF interface entry is present in both DMI and ACPI tables.
In SMP systems, in such cases it is possible that ssif_probe could be called
simultaneously from i2c interface (from ACPI) and from DMI on different CPUs at
kernel boot. Both try to register same SSIF interface simultaneously and result
in race.
In such cases where ACPI and SMBIOS both IPMI entries 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 at the
same
address, we need to remove the SMBIOS one and add the ACPI one.
Log:
[ 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
...
[NOTE] : Added custom prints to explain the problem.
In the above log ssif_probe is executed simultaneously on two different CPUs.
This patch fixes this issue in following way:
1. Adds ACPI entry also to the 'ssif_infos' list.
2. Checks the list if SMBIOS is already registered, removes it and adds ACPI.
3. If ACPI is already registered, it ignores SMBIOS.
4. Adds mutex lock throughout the probe process to avoid race.
Signed-off-by: Kamlakant Patel <kamlaka...@marvell.com>
---
drivers/char/ipmi/ipmi_ssif.c | 89 +++++++++++++++++++++++++++++++++--
1 file changed, 84 insertions(+), 5 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 8b5aec5430f1..763279b5e17c 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -185,6 +185,7 @@ struct ssif_addr_info {
char *adapter_name;
int debug;
int slave_addr;
+ bool is_probed;
enum ipmi_addr_src addr_src;
union ipmi_smi_info_union addr_info;
struct device *dev;
@@ -1312,6 +1313,7 @@ static int ssif_remove(struct i2c_client *client)
list_for_each_entry(addr_info, &ssif_infos, link) {
if (addr_info->client == client) {
+ addr_info->is_probed = 0;
addr_info->client = NULL;
break;
}
@@ -1414,7 +1416,8 @@ static int strcmp_nospace(char *s1, char *s2)
return 0;
}
-static struct ssif_addr_info *ssif_info_find(unsigned short addr,
+static struct ssif_addr_info *ssif_info_find(struct i2c_client *client,
+ unsigned short addr,
char *adapter_name,
bool match_null_name)
{
@@ -1423,6 +1426,13 @@ static struct ssif_addr_info *ssif_info_find(unsigned
short addr,
restart:
list_for_each_entry(info, &ssif_infos, link) {
if (info->binfo.addr == addr) {
+ if (!info->client && info->addr_src == SI_SMBIOS &&
+ client) {
+ info->client = client;
+ info->adapter_name =
kstrdup(client->adapter->name,
+ GFP_KERNEL);
+ }
+
if (info->adapter_name || adapter_name) {
if (!info->adapter_name != !adapter_name) {
/* One is NULL and one is not */
@@ -1592,12 +1602,73 @@ static void test_multipart_messages(struct i2c_client
*client,
return;
}
+static void ssif_remove_dup(struct i2c_client *client)
+{
+ struct ssif_info *ssif_info = i2c_get_clientdata(client);
+
+ if (!ssif_info)
+ return;
+ ipmi_unregister_smi(ssif_info->intf);
+ kfree(ssif_info);
+}
/*
* Global enables we care about.
*/
#define GLOBAL_ENABLES_MASK (IPMI_BMC_EVT_MSG_BUFF | IPMI_BMC_RCV_MSG_INTR | \
IPMI_BMC_EVT_MSG_INTR)
+static int ssif_check_present(struct ssif_addr_info *info,
+ struct i2c_client *client)
+{
+ if (!strcmp(info->adapter_name, client->adapter->name) &&
+ (info->binfo.addr == client->addr))
+ return true;
+
+ return false;
+}
+
+/*
+ * 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_info *ssif_info)
+{
+ struct ssif_addr_info *info;
+
+ list_for_each_entry(info, &ssif_infos, link) {
+ if (info->is_probed) {
+ if (ssif_check_present(info, client)) {
+ if (info->addr_src == SI_ACPI) {
+ return -EEXIST;
+ } else if (ssif_info->addr_source == SI_ACPI &&
+ info->addr_src == SI_SMBIOS) {
+ dev_info(&client->dev,
+ "Removing %s-specified SSIF
interface in favor of ACPI\n",
+
ipmi_addr_src_to_str(info->addr_src));
+ ssif_remove_dup(info->client);
+ }
+ }
+ } else if (ssif_info->addr_source == SI_SMBIOS) {
+ info->is_probed = true;
+ return 0;
+ }
+ }
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+ info->addr_src = ssif_info->addr_source;
+ info->is_probed = true;
+ info->client = client;
+ info->adapter_name = kstrdup(client->adapter->name, GFP_KERNEL);
+ info->binfo.addr = client->addr;
+ list_add_tail(&info->link, &ssif_infos);
+
+ return 0;
+}
+
static int ssif_probe(struct i2c_client *client, const struct i2c_device_id
*id)
{
unsigned char msg[3];
@@ -1609,6 +1680,7 @@ static int ssif_probe(struct i2c_client *client, const
struct i2c_device_id *id)
u8 slave_addr = 0;
struct ssif_addr_info *addr_info = NULL;
+ mutex_lock(&ssif_infos_mutex);
resp = kmalloc(IPMI_MAX_MSG_LENGTH, GFP_KERNEL);
if (!resp)
return -ENOMEM;
@@ -1620,8 +1692,8 @@ static int ssif_probe(struct i2c_client *client, const
struct i2c_device_id *id)
}
if (!check_acpi(ssif_info, &client->dev)) {
- addr_info = ssif_info_find(client->addr, client->adapter->name,
- true);
+ addr_info = ssif_info_find(client, client->addr,
+ client->adapter->name, true);
if (!addr_info) {
/* Must have come in through sysfs. */
ssif_info->addr_source = SI_HOTMOD;
@@ -1633,7 +1705,13 @@ static int ssif_probe(struct i2c_client *client, const
struct i2c_device_id *id)
slave_addr = addr_info->slave_addr;
}
}
-
+ /* Check if SSIF is already registered */
+ if (ssif_is_registered(client, ssif_info)) {
+ kfree(resp);
+ kfree(ssif_info);
+ mutex_unlock(&ssif_infos_mutex);
+ return 0;
+ }
slave_addr = find_slave_address(client, slave_addr);
dev_info(&client->dev,
@@ -1846,6 +1924,7 @@ static int ssif_probe(struct i2c_client *client, const
struct i2c_device_id *id)
kfree(ssif_info);
}
kfree(resp);
+ mutex_unlock(&ssif_infos_mutex);
return rv;
out_remove_attr:
@@ -1878,7 +1957,7 @@ static int new_ssif_client(int addr, char *adapter_name,
int rv = 0;
mutex_lock(&ssif_infos_mutex);
- if (ssif_info_find(addr, adapter_name, false)) {
+ if (ssif_info_find(NULL, addr, adapter_name, false)) {
rv = -EEXIST;
goto out_unlock;
}
--
2.17.1
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
