On 1/24/19 4:24 AM, Kamlakant Patel wrote:
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.
Ah, that's right, ssif_infos is a list of things that have been registered,
not a list of things that are running. My naming there is not so good.
#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);
Looking at this, are you sure this code works? Your
ssif_is-registered() function
is called from ssif_probe(), which is called by the i2c code when it
registers a
new device. However, if i2c has a device at that address, it should not
call
the probe function, it should ignore the duplicate. So ACPI won't override
DMI if it is already registered.
Conversely, if ACPI gets registered first, the the DMI registration
comes in,
then the i2c code should ignore the DMI one and everything should be
happy (except ACPI doesn't have the BMC slave address).
I'm also thinking that ssif_remove() as it is is wrong. If you remove the
adapter driver, it should not remove the item from the ssif_info list,
because if you then re-add the adapter it will be gone and not handled.
Anyway, the trouble is that ACPI devices come in straight from the
i2c driver and DMI devices come in through the SSIF code. It seems to
me that you would have to get the ACPI devices to come through the
SSIF code, too, to make this work right.
-corey
_______________________________________________
Openipmi-developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
