On Wed, Aug 21, 2019 at 12:04:33PM +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.
>
> 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 <[email protected]>
> ---
> Hi Corey,
>
> I have done a few improvements on the code based on internal review comments.
> Could you please pick this patch instead of previous one.
Ok, got it, thanks.
-corey
>
> Thanks,
> Kamlakant Patel
>
> Changes since v2:
> - Handle mutex_unlock for all error paths.
> - Removed unnecessary goto from ssif_check_and_remove.
> - Removed unnecessary continue from ssif_check_and_remove.
> - Removed unnecessary ssif_client_match function.
> - Removed unnecessary NULL check.
> - Moved ssif_add_infos to ssif_probe to avoid to make it more readable.
>
> 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, 77 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 305fa505..ee60892 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1428,6 +1428,10 @@ 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->addr_src == SI_SMBIOS)
> + info->adapter_name = kstrdup(adapter_name,
> + GFP_KERNEL);
> +
> if (info->adapter_name || adapter_name) {
> if (!info->adapter_name != !adapter_name) {
> /* One is NULL and one is not */
> @@ -1603,6 +1607,60 @@ 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);
> +
> + ipmi_unregister_smi(ssif_info->intf);
> + kfree(ssif_info);
> +}
> +
> +static int ssif_add_infos(struct i2c_client *client)
> +{
> + struct ssif_addr_info *info;
> +
> + 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 (!info->client)
> + return 0;
> + if (!strcmp(info->adapter_name, client->adapter->name) &&
> + info->binfo.addr == client->addr) {
> + 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);
> + return 0;
> + }
> + }
> + }
> + return 0;
> +}
> +
> static int ssif_probe(struct i2c_client *client, const struct i2c_device_id
> *id)
> {
> unsigned char msg[3];
> @@ -1614,13 +1672,17 @@ 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)
> + if (!resp) {
> + mutex_unlock(&ssif_infos_mutex);
> return -ENOMEM;
> + }
>
> ssif_info = kzalloc(sizeof(*ssif_info), GFP_KERNEL);
> if (!ssif_info) {
> kfree(resp);
> + mutex_unlock(&ssif_infos_mutex);
> return -ENOMEM;
> }
>
> @@ -1639,6 +1701,19 @@ static int ssif_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> }
> }
>
> + rv = ssif_check_and_remove(client, ssif_info);
> + /* If rv is 0 and addr source is not SI_ACPI, continue probing */
> + if (!rv && ssif_info->addr_source == SI_ACPI) {
> + rv = ssif_add_infos(client);
> + if (rv) {
> + dev_err(&client->dev, "Out of memory!, exiting ..\n");
> + goto out;
> + }
> + } else if (rv) {
> + dev_err(&client->dev, "Not probing, Interface already
> present\n");
> + goto out;
> + }
> +
> slave_addr = find_slave_address(client, slave_addr);
>
> dev_info(&client->dev,
> @@ -1851,6 +1926,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
[email protected]
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
