Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Cédric Le Goater]

On 08/10/2017 04:31 AM, Jeremy Kerr wrote:
> Hi Brendan,
> 
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
> 
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

The ioctl is part of the kernel user API now. We should keep it.

Thanks,

C.  

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Cédric Le Goater]

On 08/10/2017 04:31 AM, Jeremy Kerr wrote:
> Hi Brendan,
> 
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
> 
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

The ioctl is part of the kernel user API now. We should keep it.

Thanks,

C.  

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Brendan Higgins]

On Wed, Aug 9, 2017 at 7:31 PM, Jeremy Kerr  wrote:
> Hi Brendan,
>
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
>
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

Whoops, I did not realize that anyone was using it. Yeah, adding it back in
should not be too hard.

>
> Cheers,
>
>
> Jeremy

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Brendan Higgins]

On Wed, Aug 9, 2017 at 7:31 PM, Jeremy Kerr  wrote:
> Hi Brendan,
>
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
>
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

Whoops, I did not realize that anyone was using it. Yeah, adding it back in
should not be too hard.

>
> Cheers,
>
>
> Jeremy

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Jeremy Kerr]

Hi Brendan,

> The driver was handling interaction with userspace on its own. This
> patch changes it to use the functionality of the ipmi_bmc framework
> instead.
> 
> Note that this removes the ability for the BMC to set SMS_ATN by making
> an ioctl. If this functionality is required, it can be added back in
> with a later patch.

As Chris has mentioned, we do use this actively at the moment, so I'd
prefer if we could not drop the support for SMS_ATN. However, using a
different interface should be fine, if that helps.

Cheers,


Jeremy

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Jeremy Kerr]

Hi Brendan,

> The driver was handling interaction with userspace on its own. This
> patch changes it to use the functionality of the ipmi_bmc framework
> instead.
> 
> Note that this removes the ability for the BMC to set SMS_ATN by making
> an ioctl. If this functionality is required, it can be added back in
> with a later patch.

As Chris has mentioned, we do use this actively at the moment, so I'd
prefer if we could not drop the support for SMS_ATN. However, using a
different interface should be fine, if that helps.

Cheers,


Jeremy

[RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Brendan Higgins]

From: Benjamin Fair 

The driver was handling interaction with userspace on its own. This
patch changes it to use the functionality of the ipmi_bmc framework
instead.

Note that this removes the ability for the BMC to set SMS_ATN by making
an ioctl. If this functionality is required, it can be added back in
with a later patch.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
 drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c | 258 +
 include/uapi/linux/bt-bmc.h|  18 --
 2 files changed, 74 insertions(+), 202 deletions(-)
 delete mode 100644 include/uapi/linux/bt-bmc.h

diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
index 70d434bc1cbf..7c8082c511ee 100644
--- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
@@ -7,25 +7,19 @@
  * 2 of the License, or (at your option) any later version.
  */
 
-#include 
-#include 
 #include 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
 
-/*
- * This is a BMC device used to communicate to the host
- */
-#define DEVICE_NAME"ipmi-bt-host"
+#define DEVICE_NAME "ipmi-bmc-bt-aspeed"
 
 #define BT_IO_BASE 0xe4
 #define BT_IRQ 10
@@ -61,18 +55,17 @@
 #define BT_BMC_BUFFER_SIZE 256
 
 struct bt_bmc {
+   struct ipmi_bmc_bus bus;
struct device   dev;
-   struct miscdevice   miscdev;
+   struct ipmi_bmc_ctx *bmc_ctx;
+   struct bt_msg   request;
struct regmap   *map;
int offset;
int irq;
-   wait_queue_head_t   queue;
struct timer_list   poll_timer;
-   struct mutexmutex;
+   spinlock_t  lock;
 };
 
-static atomic_t open_count = ATOMIC_INIT(0);
-
 static const struct regmap_config bt_regmap_cfg = {
.reg_bits = 32,
.val_bits = 32,
@@ -158,27 +151,28 @@ static ssize_t bt_writen(struct bt_bmc *bt_bmc, u8 *buf, 
size_t n)
return n;
 }
 
+/* TODO(benjaminfair): support ioctl BT_BMC_IOCTL_SMS_ATN */
 static void set_sms_atn(struct bt_bmc *bt_bmc)
 {
bt_outb(bt_bmc, BT_CTRL_SMS_ATN, BT_CTRL);
 }
 
-static struct bt_bmc *file_bt_bmc(struct file *file)
+/* Called with bt_bmc->lock held */
+static bool __is_request_avail(struct bt_bmc *bt_bmc)
 {
-   return container_of(file->private_data, struct bt_bmc, miscdev);
+   return bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN;
 }
 
-static int bt_bmc_open(struct inode *inode, struct file *file)
+static bool is_request_avail(struct bt_bmc *bt_bmc)
 {
-   struct bt_bmc *bt_bmc = file_bt_bmc(file);
+   unsigned long flags;
+   bool result;
 
-   if (atomic_inc_return(_count) == 1) {
-   clr_b_busy(bt_bmc);
-   return 0;
-   }
+   spin_lock_irqsave(_bmc->lock, flags);
+   result = __is_request_avail(bt_bmc);
+   spin_unlock_irqrestore(_bmc->lock, flags);
 
-   atomic_dec(_count);
-   return -EBUSY;
+   return result;
 }
 
 /*
@@ -194,67 +188,43 @@ static int bt_bmc_open(struct inode *inode, struct file 
*file)
  *Length  NetFn/LUN  Seq Cmd Data
  *
  */
-static ssize_t bt_bmc_read(struct file *file, char __user *buf,
-  size_t count, loff_t *ppos)
+static void get_request(struct bt_bmc *bt_bmc)
 {
-   struct bt_bmc *bt_bmc = file_bt_bmc(file);
-   u8 len;
-   int len_byte = 1;
-   u8 kbuffer[BT_BMC_BUFFER_SIZE];
-   ssize_t ret = 0;
-   ssize_t nread;
+   u8 *request_buf = (u8 *) _bmc->request;
+   unsigned long flags;
 
-   if (!access_ok(VERIFY_WRITE, buf, count))
-   return -EFAULT;
+   spin_lock_irqsave(_bmc->lock, flags);
 
-   WARN_ON(*ppos);
-
-   if (wait_event_interruptible(bt_bmc->queue,
-bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))
-   return -ERESTARTSYS;
-
-   mutex_lock(_bmc->mutex);
-
-   if (unlikely(!(bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))) {
-   ret = -EIO;
-   goto out_unlock;
+   if (!__is_request_avail(bt_bmc)) {
+   spin_unlock_irqrestore(_bmc->lock, flags);
+   return;
}
 
set_b_busy(bt_bmc);
clr_h2b_atn(bt_bmc);
clr_rd_ptr(bt_bmc);
 
-   /*
-* The BT frames start with the message length, which does not
-* include the length byte.
-*/
-   kbuffer[0] = bt_read(bt_bmc);
-   len = kbuffer[0];
-
-   /* We pass the length back to userspace as well */
-   if (len + 1 > count)
-   len = count - 1;
-
-   while (len) {
-   nread = min_t(ssize_t, len, sizeof(kbuffer) - len_byte);
-
-   bt_readn(bt_bmc, kbuffer + 

[RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Brendan Higgins]

From: Benjamin Fair 

The driver was handling interaction with userspace on its own. This
patch changes it to use the functionality of the ipmi_bmc framework
instead.

Note that this removes the ability for the BMC to set SMS_ATN by making
an ioctl. If this functionality is required, it can be added back in
with a later patch.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
 drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c | 258 +
 include/uapi/linux/bt-bmc.h|  18 --
 2 files changed, 74 insertions(+), 202 deletions(-)
 delete mode 100644 include/uapi/linux/bt-bmc.h

diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
index 70d434bc1cbf..7c8082c511ee 100644
--- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
@@ -7,25 +7,19 @@
  * 2 of the License, or (at your option) any later version.
  */
 
-#include 
-#include 
 #include 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
 
-/*
- * This is a BMC device used to communicate to the host
- */
-#define DEVICE_NAME"ipmi-bt-host"
+#define DEVICE_NAME "ipmi-bmc-bt-aspeed"
 
 #define BT_IO_BASE 0xe4
 #define BT_IRQ 10
@@ -61,18 +55,17 @@
 #define BT_BMC_BUFFER_SIZE 256
 
 struct bt_bmc {
+   struct ipmi_bmc_bus bus;
struct device   dev;
-   struct miscdevice   miscdev;
+   struct ipmi_bmc_ctx *bmc_ctx;
+   struct bt_msg   request;
struct regmap   *map;
int offset;
int irq;
-   wait_queue_head_t   queue;
struct timer_list   poll_timer;
-   struct mutexmutex;
+   spinlock_t  lock;
 };
 
-static atomic_t open_count = ATOMIC_INIT(0);
-
 static const struct regmap_config bt_regmap_cfg = {
.reg_bits = 32,
.val_bits = 32,
@@ -158,27 +151,28 @@ static ssize_t bt_writen(struct bt_bmc *bt_bmc, u8 *buf, 
size_t n)
return n;
 }
 
+/* TODO(benjaminfair): support ioctl BT_BMC_IOCTL_SMS_ATN */
 static void set_sms_atn(struct bt_bmc *bt_bmc)
 {
bt_outb(bt_bmc, BT_CTRL_SMS_ATN, BT_CTRL);
 }
 
-static struct bt_bmc *file_bt_bmc(struct file *file)
+/* Called with bt_bmc->lock held */
+static bool __is_request_avail(struct bt_bmc *bt_bmc)
 {
-   return container_of(file->private_data, struct bt_bmc, miscdev);
+   return bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN;
 }
 
-static int bt_bmc_open(struct inode *inode, struct file *file)
+static bool is_request_avail(struct bt_bmc *bt_bmc)
 {
-   struct bt_bmc *bt_bmc = file_bt_bmc(file);
+   unsigned long flags;
+   bool result;
 
-   if (atomic_inc_return(_count) == 1) {
-   clr_b_busy(bt_bmc);
-   return 0;
-   }
+   spin_lock_irqsave(_bmc->lock, flags);
+   result = __is_request_avail(bt_bmc);
+   spin_unlock_irqrestore(_bmc->lock, flags);
 
-   atomic_dec(_count);
-   return -EBUSY;
+   return result;
 }
 
 /*
@@ -194,67 +188,43 @@ static int bt_bmc_open(struct inode *inode, struct file 
*file)
  *Length  NetFn/LUN  Seq Cmd Data
  *
  */
-static ssize_t bt_bmc_read(struct file *file, char __user *buf,
-  size_t count, loff_t *ppos)
+static void get_request(struct bt_bmc *bt_bmc)
 {
-   struct bt_bmc *bt_bmc = file_bt_bmc(file);
-   u8 len;
-   int len_byte = 1;
-   u8 kbuffer[BT_BMC_BUFFER_SIZE];
-   ssize_t ret = 0;
-   ssize_t nread;
+   u8 *request_buf = (u8 *) _bmc->request;
+   unsigned long flags;
 
-   if (!access_ok(VERIFY_WRITE, buf, count))
-   return -EFAULT;
+   spin_lock_irqsave(_bmc->lock, flags);
 
-   WARN_ON(*ppos);
-
-   if (wait_event_interruptible(bt_bmc->queue,
-bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))
-   return -ERESTARTSYS;
-
-   mutex_lock(_bmc->mutex);
-
-   if (unlikely(!(bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))) {
-   ret = -EIO;
-   goto out_unlock;
+   if (!__is_request_avail(bt_bmc)) {
+   spin_unlock_irqrestore(_bmc->lock, flags);
+   return;
}
 
set_b_busy(bt_bmc);
clr_h2b_atn(bt_bmc);
clr_rd_ptr(bt_bmc);
 
-   /*
-* The BT frames start with the message length, which does not
-* include the length byte.
-*/
-   kbuffer[0] = bt_read(bt_bmc);
-   len = kbuffer[0];
-
-   /* We pass the length back to userspace as well */
-   if (len + 1 > count)
-   len = count - 1;
-
-   while (len) {
-   nread = min_t(ssize_t, len, sizeof(kbuffer) - len_byte);
-
-   bt_readn(bt_bmc, kbuffer + len_byte, nread);
-
-   if (copy_to_user(buf, kbuffer, nread +