On Wed, Jul 17, 2019 at 04:08:59PM +0000, Kamlakant Patel wrote:
In general I think this is ok. A few issues, though:
> 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.
Text in the header needs to be <=75 characters wide, per kernel style.
>
> 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);
> +}
Space here.
> /*
> * 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)
ssif_client_match() or something like that would probably be a better
name here.
> +{
> + 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)
> +{
It would be better if is_xxx() functions don't have side effects. That
can surprise the user. A different function name would probably be
better.
> + 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;
I'm trying to figure out the purpose of is_probed. Wouldn't
checkng client be good enough?
Then you don't need all this, and the check_present call
could just return false if client is not set.
> + 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);
You are duplicating a bunch of code here. Make a function.
Also, hard-coded additions may be an issue here, as they are
not SMBIOS, but they already should have addr_info items.
I really should rename ssif_infos to ssif_addr_infos and
document it better :(.
I'm wishing there was a simpler way to do this, but I don't
see one.
-corey
> +
> + 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
