On Wed, Jul 24, 2019 at 03:46:49PM -0500, Corey Minyard wrote:
Hi Corey,
> 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.
Sure, I will name it like that.
>
> > +{
> > + 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.
I will try to put a better function name here.
>
> > + 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.
I had added is_probed to keep track of probed clients, but as you
suggested checking the client should be good enough. I will try to
implement in this way and send an updated patch.
>
> > + 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.
>
Thanks for your comments and suggestions on this.
I will send an updated patch.
Thanks,
Kamlakant Patel
> -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
