Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C

[Brendan Higgins via Openipmi-developer]

On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard  wrote:
> On 08/04/2017 08:18 PM, Brendan Higgins wrote:
>>
>> This patchset introduces IPMI Block Transfer over I2C (BT-I2C), which has
>> the
>> same semantics as IPMI Block Transfer except it done over I2C.
>>
>> For the OpenBMC people, this is based on an RFC:
>> https://lists.ozlabs.org/pipermail/openbmc/2016-September/004505.html
>>
>> The documentation discusses the reason for this in greater detail, suffice
>> it to
>> say SSIF cannot be correctly implemented on some naive I2C devices. There
>> are
>> some additional reasons why we don't like SSIF, but those are again
>> covered in
>> the documentation for all those who are interested.
>
>
> I'm not terribly excited about this.  A few notes:

I was afraid so, alas.

>
> SMBus alerts are fairly broken in Linux right now.  I have a patch to fix
> this at:
> https://github.com/cminyard/linux-ipmi/commit/48136176ce1890f99857c73e0ace5bd8dfb61fbf
> I haven't been able to get much traction getting anyone to care.

Yeah, I have some work I would like to do there as well.

>
> The lack of a NACK could be worked around fairly easily in the current
> driver.  It looks like you
> are just returning a message too short to be valid.  That's easy.  I think
> it's a rather major
> deficiency in the hardware to not be able to NACK something, but that is
> what it is.

Right, we actually have multiple pieces of hardware that do not support
NACKing correctly. The most frustrating piece is the Aspeed chip which
does not provide and facility for arbitrary NACKs to be generated on the
slave side.

>
> What you have is not really BT over I2C.  You have just added a sequence
> number to the
> IPMI messages and dropped the SMBus command.  Other interfaces have sequence
> numbers,
> too.  Calling it BT is a little over the top.

Fair point, maybe ISIF (I2C System Interface)? I don't have strong feelings
about the name.

>
> Do you really need the performance required by having multiple outstanding
> messages?
> That adds a lot of complexity, if it's unnecessary it's just a waste.  The
> IPMI work on top
> of interfaces does not really require that much performance, it's just
> reading sensors,
> FRU data, and such.  Perhaps you have a reason, but I fail to see
> why it's really that big a deal.  The BT interface has this ability, but the
> driver does not
> take advantage of it and nobody has complained.
>

Yes, we do have some platforms which only have IPMI as a standard interface
and we are abusing some OEM commands to do some things that we probably
should not do with IPMI like doing firmware updates. Also, we have some
commands which take a really long time to complete (> 1s). Admittedly, this is
abusing IPMI to solve problems which should probably be solved elsewhere;
nevertheless, it is a feature we are actually using. And having an option to use
sequence numbers if definitely nice from our perspective.

We will probably want to improve the block transfer driver at some
point as well.

> And I don't understand the part about OpenBMC making use of sequence
> numbers.
> Why does that matter for this interface?  It's the host side that would care
> about
> that, the host would stick the numbers in and the slave would return it.  If
> you are
> using sequence numbers in OpenBMC, which sounds quite reasonable, I would
> think
> it would be a bad idea to to trust that the host would give you good
> sequence
> numbers.
>

I think, I illustrated the use case above, but to reiterate, the
desire is to have
multiple messages in flight at the same time because some messages take
a long time to service.

> Plus, with multiple outstanding messages, you really need to limit it.  A
> particular BMC
> may not be able to handle it the full 256, and the ability to have that many
> messages
> outstanding is probably not a good thing.
>

It is going to depend on the BMC of course; nevertheless, I would be willing
to implement a configurable limit.

> If you really need multiple outstanding messages, the host side IPMI message
> handler
> needs to change to allow that.  It's doable, and I know how, I just haven't
> seen the
> need.
>

Sure, we would also like SMBus alert support, but I figured it was probably
best to discuss this with you before we go too far down that path.

> I would agree that the multi-part messages in SSIF is a big pain and and a
> lot of
> unnecessary complexity.  I believe it is there to accommodate host hardware
> that is
> limited.  But SMBus can have 255 byte messages and there's no arbitrary
> limit on
> I2C.  It is the way of IPMI to support the least common denominator.
>
> Your interface will only work on Linux.  Other OSes (unless they choose to
> implement this
> driver) will be unable to use your BMC.  Of course there's the NACK issue,
> but that's a
> big difference, and it would probably still work with existing drivers on
> other OSes.

I hope I did not send the message that we are planning 

[Openipmi-developer] [RFC v1 1/4] ipmi_bmc: framework for BT IPMI on BMCs

[Brendan Higgins via Openipmi-developer]

From: Benjamin Fair 

This patch introduces a framework for writing IPMI drivers which run on
a Board Management Controller. It is similar in function to OpenIPMI.
The framework handles registering devices and routing messages.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
 drivers/char/ipmi_bmc/Makefile   |   1 +
 drivers/char/ipmi_bmc/ipmi_bmc.c | 294 +++
 include/linux/ipmi_bmc.h | 184 
 3 files changed, 479 insertions(+)
 create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc.c

diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
index 8bff32b55c24..9c7cd48d899f 100644
--- a/drivers/char/ipmi_bmc/Makefile
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -2,5 +2,6 @@
 # Makefile for the ipmi bmc drivers.
 #
 
+obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
 obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
diff --git a/drivers/char/ipmi_bmc/ipmi_bmc.c b/drivers/char/ipmi_bmc/ipmi_bmc.c
new file mode 100644
index ..c1324ac9a83c
--- /dev/null
+++ b/drivers/char/ipmi_bmc/ipmi_bmc.c
@@ -0,0 +1,294 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define PFX "IPMI BMC core: "
+
+struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx()
+{
+   static struct ipmi_bmc_ctx global_ctx;
+
+   return _ctx;
+}
+
+int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
+  struct bt_msg *bt_response)
+{
+   struct ipmi_bmc_bus *bus;
+   int ret = -ENODEV;
+
+   rcu_read_lock();
+   bus = rcu_dereference(ctx->bus);
+
+   if (bus)
+   ret = bus->send_response(bus, bt_response);
+
+   rcu_read_unlock();
+   return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_send_response);
+
+bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx)
+{
+   struct ipmi_bmc_bus *bus;
+   bool ret = false;
+
+   rcu_read_lock();
+   bus = rcu_dereference(ctx->bus);
+
+   if (bus)
+   ret = bus->is_response_open(bus);
+
+   rcu_read_unlock();
+   return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_is_response_open);
+
+int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
+struct ipmi_bmc_device *device_in)
+{
+   struct ipmi_bmc_device *device;
+
+   mutex_lock(>drivers_mutex);
+   /* Make sure it hasn't already been registered. */
+   list_for_each_entry(device, >devices, link) {
+   if (device == device_in) {
+   mutex_unlock(>drivers_mutex);
+   return -EINVAL;
+   }
+   }
+
+   list_add_rcu(_in->link, >devices);
+   mutex_unlock(>drivers_mutex);
+
+   return 0;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_device);
+
+int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
+  struct ipmi_bmc_device *device_in)
+{
+   struct ipmi_bmc_device *device;
+   bool found = false;
+
+   mutex_lock(>drivers_mutex);
+   /* Make sure it is currently registered. */
+   list_for_each_entry(device, >devices, link) {
+   if (device == device_in) {
+   found = true;
+   break;
+   }
+   }
+   if (!found) {
+   mutex_unlock(>drivers_mutex);
+   return -ENXIO;
+   }
+
+   list_del_rcu(_in->link);
+   mutex_unlock(>drivers_mutex);
+   synchronize_rcu();
+
+   return 0;
+}
+EXPORT_SYMBOL(ipmi_bmc_unregister_device);
+
+int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
+struct ipmi_bmc_device *device)
+{
+   int ret;
+
+   mutex_lock(>drivers_mutex);
+   if (!ctx->default_device) {
+   ctx->default_device = device;
+   ret = 0;
+   } else {
+   ret = -EBUSY;
+   }
+   mutex_unlock(>drivers_mutex);
+
+   return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_default_device);
+
+int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
+  struct ipmi_bmc_device *device)
+{
+   int ret;
+
+   mutex_lock(>drivers_mutex);
+   if (ctx->default_device == device) {
+   ctx->default_device = NULL;
+   ret = 0;
+   } else {
+   ret = -ENXIO;
+   }
+ 

[Openipmi-developer] [RFC v1 3/4] ipmi_bmc: bt-i2c: port driver to IPMI BMC framework

[Brendan Higgins via Openipmi-developer]

From: Benjamin Fair 

Instead of handling interaction with userspace and providing a file
interface, rely on the IPMI BMC framework to do this. This simplifies
the logic and eliminates duplicate code.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
 drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c | 202 +---
 1 file changed, 28 insertions(+), 174 deletions(-)

diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
index 686b83fa42a4..6665aa9d4300 100644
--- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
@@ -14,102 +14,51 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
-#include 
 #include 
 #include 
-#include 
 
 #define PFX "IPMI BMC BT-I2C: "
 
-/*
- * TODO: This is "bt-host" to match the bt-host driver; however, I think this 
is
- * unclear in the context of a CPU side driver. Should probably name this
- * and the DEVICE_NAME in bt-host to something like "bt-bmc" or "bt-slave".
- */
-#define DEVICE_NAME"ipmi-bt-host"
-
-static const unsigned long request_queue_max_len = 256;
-
-struct bt_request_elem {
-   struct list_headlist;
-   struct bt_msg   request;
-};
-
 struct bt_i2c_slave {
+   struct ipmi_bmc_bus bus;
struct i2c_client   *client;
-   struct miscdevice   miscdev;
+   struct ipmi_bmc_ctx *bmc_ctx;
struct bt_msg   request;
-   struct list_headrequest_queue;
-   atomic_trequest_queue_len;
struct bt_msg   response;
boolresponse_in_progress;
size_t  msg_idx;
spinlock_t  lock;
-   wait_queue_head_t   wait_queue;
-   struct mutexfile_mutex;
 };
 
-static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool non_blocking,
- struct bt_msg *bt_request)
+static bool bt_i2c_is_response_open(struct ipmi_bmc_bus *bus)
 {
-   int res;
+   struct bt_i2c_slave *bt_slave;
+   bool response_in_progress;
unsigned long flags;
-   struct bt_request_elem *queue_elem;
-
-   if (!non_blocking) {
-try_again:
-   res = wait_event_interruptible(
-   bt_slave->wait_queue,
-   atomic_read(_slave->request_queue_len));
-   if (res)
-   return res;
-   }
 
-   spin_lock_irqsave(_slave->lock, flags);
-   if (!atomic_read(_slave->request_queue_len)) {
-   spin_unlock_irqrestore(_slave->lock, flags);
-   if (non_blocking)
-   return -EAGAIN;
-   goto try_again;
-   }
+   bt_slave = container_of(bus, struct bt_i2c_slave, bus);
 
-   if (list_empty(_slave->request_queue)) {
-   pr_err(PFX "request_queue was empty despite nonzero 
request_queue_len\n");
-   return -EIO;
-   }
-   queue_elem = list_first_entry(_slave->request_queue,
- struct bt_request_elem, list);
-   memcpy(bt_request, _elem->request, sizeof(*bt_request));
-   list_del(_elem->list);
-   kfree(queue_elem);
-   atomic_dec(_slave->request_queue_len);
+   spin_lock_irqsave(_slave->lock, flags);
+   response_in_progress = bt_slave->response_in_progress;
spin_unlock_irqrestore(_slave->lock, flags);
-   return 0;
+
+   return !response_in_progress;
 }
 
-static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
-   struct bt_msg *bt_response)
+static int bt_i2c_send_response(struct ipmi_bmc_bus *bus,
+   struct bt_msg *bt_response)
 {
-   int res;
+   struct bt_i2c_slave *bt_slave;
unsigned long flags;
 
-   if (!non_blocking) {
-try_again:
-   res = wait_event_interruptible(bt_slave->wait_queue,
-  !bt_slave->response_in_progress);
-   if (res)
-   return res;
-   }
+   bt_slave = container_of(bus, struct bt_i2c_slave, bus);
 
spin_lock_irqsave(_slave->lock, flags);
if (bt_slave->response_in_progress) {
spin_unlock_irqrestore(_slave->lock, flags);
-   if (non_blocking)
-   return -EAGAIN;
-   goto try_again;
+   return -EAGAIN;
}
 
memcpy(_slave->response, bt_response, sizeof(*bt_response));
@@ -118,106 +67,13 @@ static int send_bt_response(struct bt_i2c_slave 
*bt_slave, bool non_blocking,
return 0;
 }
 
-static inline struct bt_i2c_slave *to_bt_i2c_slave(struct file *file)
-{
-   return container_of(file->private_data, struct bt_i2c_slave, miscdev);
-}
-
-static ssize_t bt_read(struct file 

Re: [Openipmi-developer] [PATCH] ipmi: fix unsigned long underflow

[Corey Minyard]

On 08/07/2017 03:41 AM, Weilong Chen wrote:

Hi Minyard,

I test this patch, it works.

Thanks.



Thanks, I added a Tested-by: for you and it's queued for the next release.
If it's urgent I can send it in now.

I also added this for the stable kernels 3.16 and later.

-corey


On 2017/7/30 10:20, miny...@acm.org wrote:

From: Corey Minyard 

When I set the timeout to a specific value such as 500ms, the timeout
event will not happen in time due to the overflow in function
check_msg_timeout:
...
ent->timeout -= timeout_period;
if (ent->timeout > 0)
return;
...

The type of timeout_period is long, but ent->timeout is unsigned long.
This patch makes the type consistent.

Reported-by: Weilong Chen 
Signed-off-by: Corey Minyard 
---
I like to keep things consistent (though I obviously messed up here)
and keep variables that should be positive unsigned.

But you are right, there is a bug here and some inconsistency.
This patch changes timeout_period to be unsigned and fixes the
check.  Can you try this out?

 drivers/char/ipmi/ipmi_msghandler.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c 
b/drivers/char/ipmi/ipmi_msghandler.c

index 810b138..c82d9fd 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4030,7 +4030,8 @@ smi_from_recv_msg(ipmi_smi_t intf, struct 
ipmi_recv_msg *recv_msg,

 }

 static void check_msg_timeout(ipmi_smi_t intf, struct seq_table *ent,
-  struct list_head *timeouts, long timeout_period,
+  struct list_head *timeouts,
+  unsigned long timeout_period,
   int slot, unsigned long *flags,
   unsigned int *waiting_msgs)
 {
@@ -4043,8 +4044,8 @@ static void check_msg_timeout(ipmi_smi_t intf, 
struct seq_table *ent,

 if (!ent->inuse)
 return;

-ent->timeout -= timeout_period;
-if (ent->timeout > 0) {
+if (timeout_period < ent->timeout) {
+ent->timeout -= timeout_period;
 (*waiting_msgs)++;
 return;
 }
@@ -4110,7 +4111,8 @@ static void check_msg_timeout(ipmi_smi_t intf, 
struct seq_table *ent,

 }
 }

-static unsigned int ipmi_timeout_handler(ipmi_smi_t intf, long 
timeout_period)

+static unsigned int ipmi_timeout_handler(ipmi_smi_t intf,
+ unsigned long timeout_period)
 {
 struct list_head timeouts;
 struct ipmi_recv_msg *msg, *msg2;






--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [Openipmi-developer] [PATCH] ipmi: fix unsigned long underflow

[Weilong Chen]

Hi Minyard,

I test this patch, it works.

Thanks.

On 2017/7/30 10:20, miny...@acm.org wrote:

From: Corey Minyard 

When I set the timeout to a specific value such as 500ms, the timeout
event will not happen in time due to the overflow in function
check_msg_timeout:
...
ent->timeout -= timeout_period;
if (ent->timeout > 0)
return;
...

The type of timeout_period is long, but ent->timeout is unsigned long.
This patch makes the type consistent.

Reported-by: Weilong Chen 
Signed-off-by: Corey Minyard 
---
I like to keep things consistent (though I obviously messed up here)
and keep variables that should be positive unsigned.

But you are right, there is a bug here and some inconsistency.
This patch changes timeout_period to be unsigned and fixes the
check.  Can you try this out?

 drivers/char/ipmi/ipmi_msghandler.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c 
b/drivers/char/ipmi/ipmi_msghandler.c
index 810b138..c82d9fd 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4030,7 +4030,8 @@ smi_from_recv_msg(ipmi_smi_t intf, struct ipmi_recv_msg 
*recv_msg,
 }

 static void check_msg_timeout(ipmi_smi_t intf, struct seq_table *ent,
- struct list_head *timeouts, long timeout_period,
+ struct list_head *timeouts,
+ unsigned long timeout_period,
  int slot, unsigned long *flags,
  unsigned int *waiting_msgs)
 {
@@ -4043,8 +4044,8 @@ static void check_msg_timeout(ipmi_smi_t intf, struct 
seq_table *ent,
if (!ent->inuse)
return;

-   ent->timeout -= timeout_period;
-   if (ent->timeout > 0) {
+   if (timeout_period < ent->timeout) {
+   ent->timeout -= timeout_period;
(*waiting_msgs)++;
return;
}
@@ -4110,7 +4111,8 @@ static void check_msg_timeout(ipmi_smi_t intf, struct 
seq_table *ent,
}
 }

-static unsigned int ipmi_timeout_handler(ipmi_smi_t intf, long timeout_period)
+static unsigned int ipmi_timeout_handler(ipmi_smi_t intf,
+unsigned long timeout_period)
 {
struct list_head timeouts;
struct ipmi_recv_msg *msg, *msg2;




--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer