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. > > >> >> #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); >> + rv = 0; >> + } else >> + rv = -EEXIST; >> + goto exit; >> + } >> + } >> + >> + info = kzalloc(sizeof(*info), GFP_KERNEL); >> + if (!info){ >> + rv = -ENOMEM; >> + goto exit; >> + } >> + >> + info->client = client; >> + info->adapter_name = client->adapter->name; >> + info->binfo.addr = client->addr; >> + list_add_tail(&info->link, &ssif_clients); >> +exit: >> + return rv; >> +} >> + >> static int ssif_probe(struct i2c_client *client, const struct >> i2c_device_id *id) >> { >> unsigned char msg[3]; >> @@ -1534,6 +1572,13 @@ 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); >> + /* Check if the client is already registered. */ >> + if (ssif_is_registered(client)) { >> + mutex_unlock(&ssif_infos_mutex); >> + return 0; >> + } >> + >> resp = kmalloc(IPMI_MAX_MSG_LENGTH, GFP_KERNEL); >> if (!resp) >> return -ENOMEM; >> @@ -1759,6 +1804,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: >> @@ -1844,6 +1890,12 @@ static void free_ssif_clients(void) >> kfree(info->adapter_name); >> kfree(info); >> } >> + >> + list_for_each_entry_safe(info, tmp, &ssif_clients, link) { >> + list_del(&info->link); >> + kfree(info); >> + } >> + > > Assuming you need a separate ssif_clients, clients can be dynamically > removed, too, (ssif_remove()) you will need to handle list removal in > that case, too. Yes, I missed it. I will send a v2 patch with all the changes. Thanks, Kamlakant Patel > > -corey > > >> mutex_unlock(&ssif_infos_mutex); >> } >> > > > _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
