On Fri, Aug 09, 2019 at 10:05:18AM +0000, Kamlakant Patel wrote:
> 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.
Sorry this took so long. But it looks good and passes basic tests, I've
added to my next tree.
Thanks,
-corey
>
> 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.
>
> 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:
> - Adds ACPI entry also to the 'ssif_infos' list.
> - Checks the list if SMBIOS is already registered, removes it and adds
> ACPI.
> - If ACPI is already registered, it ignores SMBIOS.
> - Adds mutex lock throughout the probe process to avoid race.
>
> Signed-off-by: Kamlakant Patel <kamlaka...@marvell.com>
> ---
> Changes since v1:
> - Avoid using separate list for handling ACPI entries.
> - This patch adds ACPI entry also to the ssif_infos list.
>
> drivers/char/ipmi/ipmi_ssif.c | 78
> +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 305fa505..6ca3ebe 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1603,6 +1603,76 @@ 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)
>
> +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);
> +}
> +
> +static int ssif_client_match(struct ssif_addr_info *info,
> + struct i2c_client *client)
> +{
> + if (!info->client)
> + return false;
> +
> + if (!strcmp(info->adapter_name, client->adapter->name) &&
> + (info->binfo.addr == client->addr))
> + return true;
> +
> + return false;
> +}
> +
> +static int ssif_update_infos(struct ssif_addr_info *info,
> + struct i2c_client *client)
> +{
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> + info->addr_src = SI_ACPI;
> + 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;
> +}
> +
> +/*
> + * 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_check_and_remove(struct i2c_client *client,
> + struct ssif_info *ssif_info)
> +{
> + struct ssif_addr_info *info;
> +
> + list_for_each_entry(info, &ssif_infos, link) {
> + if (ssif_client_match(info, client)) {
> + if (info->addr_src == SI_SMBIOS &&
> + ssif_info->addr_source == SI_SMBIOS)
> + return 0;
> + if (info->addr_src == SI_ACPI)
> + return -EEXIST;
> +
> + 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);
> + goto update_list;
> + }
> + } else
> + continue;
> + }
> + update_list:
> + return ssif_update_infos(info, client);
> +}
> +
> static int ssif_probe(struct i2c_client *client, const struct i2c_device_id
> *id)
> {
> unsigned char msg[3];
> @@ -1614,6 +1684,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;
> @@ -1639,6 +1710,12 @@ static int ssif_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> }
> }
>
> + if (ssif_check_and_remove(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,
> @@ -1851,6 +1928,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:
> --
> 1.8.3.1
>
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
