Re: [PATCH v1 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver

2021-04-02 Thread Quan Nguyen 

On 31/03/2021 14:21, Joel Stanley wrote:

On Mon, 29 Mar 2021 at 12:18, Quan Nguyen  wrote:


The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
in-band IPMI communication with their host in management (BMC) side.

This commits adds support specifically for Aspeed AST2500 which commonly
used as Board Management Controllers.

Signed-off-by: Quan Nguyen 


I don't have any SSIF or IPMI related feedback on your patch, but some
general things I noticed when reading it.



Thank you, Joel,
I'm really appreciate your comments for this patch series.

And, as there is a compilation error detected by kernel robot test, I 
was hurry to send out v2 just to fix that just before your email 
arrived. So I'd like to address all comments in my upcoming v3.




---
  drivers/char/ipmi/Kconfig   |  22 +
  drivers/char/ipmi/Makefile  |   2 +
  drivers/char/ipmi/ssif_bmc.c| 645 
  drivers/char/ipmi/ssif_bmc.h|  92 
  drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++
  5 files changed, 893 insertions(+)
  create mode 100644 drivers/char/ipmi/ssif_bmc.c
  create mode 100644 drivers/char/ipmi/ssif_bmc.h
  create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c


It would make sense to split the aspeed implementation into a separate
patch form the ssif framework.


Yes, will do in next version


+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .


You should omit the licence text; it is replaced by the SPDX tags.


My bad, will remove in next version


+static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool 
non_blocking)
+{
+   unsigned long flags;
+   int ret;
+
+   if (!non_blocking) {
+retry:
+   ret = wait_event_interruptible(ssif_bmc->wait_queue,
+  !ssif_bmc->response_in_progress);
+   if (ret)
+   return ret;
+   }
+
+   spin_lock_irqsave(_bmc->lock, flags);


What's the lock doing here? We've just waited for response_in_progress
to be false, so we then take the lock to check what value it is?
Should it also be sending some data in this function?

The lock is to make sure ssif_bmc->response_in_progress are completely 
processed, ie: when the lock already released.
My concern is that reference to that value without acquiring lock may 
not be true as it is under other possess.


In fact, the lock here is for the whole data pointed by ssif_bmc 
pointer. Hence, every reference/modify to this data must be done with 
the lock acquired.



+   if (ssif_bmc->response_in_progress) {
+   spin_unlock_irqrestore(_bmc->lock, flags);
+   if (non_blocking)
+   return -EAGAIN;
+
+   goto retry;


The goto threw me, so I tried re-writing it without. Again, I don't
quite follow what the spinlock is doing.

while (1) {
 if (blocking) {
 ret = wait_event_interruptible();
 if (ret)
  return ret;
 }

  spin_lock_irqsave()
  if (ssif_bmc->response_in_progress) {
  spin_lock_irqrestore()
  if (!blocking)
  return -EAGAIN;
  } else {
 spin_lock_irqrestore()
 break;
  }
}


I'm afraid we would need to re-acquire the lock before modifying 
ssif_bmc->is_singlepart_read and ssif_bmc->response_in_progress below.



+   }
+
+   /*
+* Check the response data length from userspace to determine the type
+* of the response message whether it is single-part or multi-part.
+*/
+   ssif_bmc->is_singlepart_read =
+   (ssif_msg_len(_bmc->response) <= 
(MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
+   true : false; /* 1: byte of length */


I don't follow the 1: byte of length comment, what is it telling me?

The ternary operator is a bit messy here, I'd go for a good old if statement.

The comment indeed does not provide any info here. Will change back to 
if else statement and add more meaningful comment if necessary in next 
version.



+
+   ssif_bmc->response_in_progress = true;
+   spin_unlock_irqrestore(_bmc->lock, flags);
+
+   return 0;
+}



+/*

Re: [PATCH v1 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver

2021-03-31 Thread Joel Stanley 
On Mon, 29 Mar 2021 at 12:18, Quan Nguyen  wrote:
>
> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
> in-band IPMI communication with their host in management (BMC) side.
>
> This commits adds support specifically for Aspeed AST2500 which commonly
> used as Board Management Controllers.
>
> Signed-off-by: Quan Nguyen 

I don't have any SSIF or IPMI related feedback on your patch, but some
general things I noticed when reading it.

> ---
>  drivers/char/ipmi/Kconfig   |  22 +
>  drivers/char/ipmi/Makefile  |   2 +
>  drivers/char/ipmi/ssif_bmc.c| 645 
>  drivers/char/ipmi/ssif_bmc.h|  92 
>  drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++
>  5 files changed, 893 insertions(+)
>  create mode 100644 drivers/char/ipmi/ssif_bmc.c
>  create mode 100644 drivers/char/ipmi/ssif_bmc.h
>  create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

It would make sense to split the aspeed implementation into a separate
patch form the ssif framework.

> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -0,0 +1,645 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .

You should omit the licence text; it is replaced by the SPDX tags.

> +static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool 
> non_blocking)
> +{
> +   unsigned long flags;
> +   int ret;
> +
> +   if (!non_blocking) {
> +retry:
> +   ret = wait_event_interruptible(ssif_bmc->wait_queue,
> +  
> !ssif_bmc->response_in_progress);
> +   if (ret)
> +   return ret;
> +   }
> +
> +   spin_lock_irqsave(_bmc->lock, flags);

What's the lock doing here? We've just waited for response_in_progress
to be false, so we then take the lock to check what value it is?
Should it also be sending some data in this function?

> +   if (ssif_bmc->response_in_progress) {
> +   spin_unlock_irqrestore(_bmc->lock, flags);
> +   if (non_blocking)
> +   return -EAGAIN;
> +
> +   goto retry;

The goto threw me, so I tried re-writing it without. Again, I don't
quite follow what the spinlock is doing.

while (1) {
if (blocking) {
ret = wait_event_interruptible();
if (ret)
 return ret;
}

 spin_lock_irqsave()
 if (ssif_bmc->response_in_progress) {
 spin_lock_irqrestore()
 if (!blocking)
 return -EAGAIN;
 } else {
spin_lock_irqrestore()
break;
 }
}


> +   }
> +
> +   /*
> +* Check the response data length from userspace to determine the type
> +* of the response message whether it is single-part or multi-part.
> +*/
> +   ssif_bmc->is_singlepart_read =
> +   (ssif_msg_len(_bmc->response) <= 
> (MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
> +   true : false; /* 1: byte of length */

I don't follow the 1: byte of length comment, what is it telling me?

The ternary operator is a bit messy here, I'd go for a good old if statement.

> +
> +   ssif_bmc->response_in_progress = true;
> +   spin_unlock_irqrestore(_bmc->lock, flags);
> +
> +   return 0;
> +}

> +/* Handle SSIF message that will be sent to user */
> +static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t 
> count, loff_t *ppos)
> +{
> +   struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
> +   struct ssif_msg msg;
> +   unsigned long flags;
> +   ssize_t ret;
> +
> +   mutex_lock(_bmc->file_mutex);

->file_mutex is protecting the device against more than one user of
the character device? Can you enforce that in open() instead?

> +
> +   ret = receive_ssif_bmc_request(ssif_bmc, file->f_flags & O_NONBLOCK);
> +   if (ret < 0)
> +   goto out;
> +
> +   spin_lock_irqsave(_bmc->lock, flags);
> +   count = min_t(ssize_t, count, ssif_msg_len(_bmc->request));

count is user controlled, so I assume ssif_msg_len will always be less
than or equal to struct ssif_msg?

> +   memcpy(, _bmc->request, count);
> +   ssif_bmc->request_available = false;
> +

[PATCH v1 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver

2021-03-29 Thread Quan Nguyen 
The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
in-band IPMI communication with their host in management (BMC) side.

This commits adds support specifically for Aspeed AST2500 which commonly
used as Board Management Controllers.

Signed-off-by: Quan Nguyen 
---
 drivers/char/ipmi/Kconfig   |  22 +
 drivers/char/ipmi/Makefile  |   2 +
 drivers/char/ipmi/ssif_bmc.c| 645 
 drivers/char/ipmi/ssif_bmc.h|  92 
 drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++
 5 files changed, 893 insertions(+)
 create mode 100644 drivers/char/ipmi/ssif_bmc.c
 create mode 100644 drivers/char/ipmi/ssif_bmc.h
 create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 07847d9a459a..d67fd204409a 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -133,6 +133,28 @@ config ASPEED_BT_IPMI_BMC
  found on Aspeed SOCs (AST2400 and AST2500). The driver
  implements the BMC side of the BT interface.
 
+config SSIF_IPMI_BMC
+   tristate "SSIF IPMI BMC driver"
+   help
+ This enables the IPMI SMBus system interface (SSIF) at the
+ management (BMC) side.
+
+ The driver implements the BMC side of the SMBus system
+ interface (SSIF).
+
+config ASPEED_SSIF_IPMI_BMC
+   depends on ARCH_ASPEED || COMPILE_TEST
+   depends on I2C
+   select SSIF_IPMI_BMC
+   select I2C_SLAVE
+   tristate "Aspeed SSIF IPMI BMC driver"
+   help
+ Provides a driver for the SSIF IPMI interface found on
+ Aspeed AST2500 SoC.
+
+ The driver implements the BMC side of the SMBus system
+ interface (SSIF), specific for Aspeed AST2500 SoC.
+
 config IPMB_DEVICE_INTERFACE
tristate 'IPMB Interface handler'
depends on I2C
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 0822adc2ec41..05b993f7335b 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -27,3 +27,5 @@ obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
 obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
 obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
 obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
+obj-$(CONFIG_SSIF_IPMI_BMC) += ssif_bmc.o
+obj-$(CONFIG_ASPEED_SSIF_IPMI_BMC) += ssif_bmc_aspeed.o
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
new file mode 100644
index ..ae6e8750c795
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "ssif_bmc.h"
+
+/*
+ * Call in WRITE context
+ */
+static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool 
non_blocking)
+{
+   unsigned long flags;
+   int ret;
+
+   if (!non_blocking) {
+retry:
+   ret = wait_event_interruptible(ssif_bmc->wait_queue,
+  !ssif_bmc->response_in_progress);
+   if (ret)
+   return ret;
+   }
+
+   spin_lock_irqsave(_bmc->lock, flags);
+   if (ssif_bmc->response_in_progress) {
+   spin_unlock_irqrestore(_bmc->lock, flags);
+   if (non_blocking)
+   return -EAGAIN;
+
+   goto retry;
+   }
+
+   /*
+* Check the response data length from userspace to determine the type
+* of the response message whether it is single-part or multi-part.
+*/
+   ssif_bmc->is_singlepart_read =
+   (ssif_msg_len(_bmc->response) <= 
(MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
+   true : false; /* 1: byte of length */
+
+   ssif_bmc->response_in_progress = true;
+   spin_unlock_irqrestore(_bmc->lock, flags);
+
+   return 0;
+}
+
+/*
+ * Call in READ context
+ */
+static int receive_ssif_bmc_request(struct ssif_bmc_ctx *ssif_bmc, bool 
non_blocking)
+{
+   unsigned long flags;
+   int ret;
+
+   if (!non_blocking) {
+retry:
+   ret = wait_event_interruptible(ssif_bmc->wait_queue,
+

[PATCH v1 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver

2021-03-29 Thread Quan Nguyen 
The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
in-band IPMI communication with their host in management (BMC) side.

This commits adds support specifically for Aspeed AST2500 which commonly
used as Board Management Controllers.

Signed-off-by: Quan Nguyen 
---
 drivers/char/ipmi/Kconfig   |  22 +
 drivers/char/ipmi/Makefile  |   2 +
 drivers/char/ipmi/ssif_bmc.c| 645 
 drivers/char/ipmi/ssif_bmc.h|  92 
 drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++
 5 files changed, 893 insertions(+)
 create mode 100644 drivers/char/ipmi/ssif_bmc.c
 create mode 100644 drivers/char/ipmi/ssif_bmc.h
 create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 07847d9a459a..d67fd204409a 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -133,6 +133,28 @@ config ASPEED_BT_IPMI_BMC
  found on Aspeed SOCs (AST2400 and AST2500). The driver
  implements the BMC side of the BT interface.
 
+config SSIF_IPMI_BMC
+   tristate "SSIF IPMI BMC driver"
+   help
+ This enables the IPMI SMBus system interface (SSIF) at the
+ management (BMC) side.
+
+ The driver implements the BMC side of the SMBus system
+ interface (SSIF).
+
+config ASPEED_SSIF_IPMI_BMC
+   depends on ARCH_ASPEED || COMPILE_TEST
+   depends on I2C
+   select SSIF_IPMI_BMC
+   select I2C_SLAVE
+   tristate "Aspeed SSIF IPMI BMC driver"
+   help
+ Provides a driver for the SSIF IPMI interface found on
+ Aspeed AST2500 SoC.
+
+ The driver implements the BMC side of the SMBus system
+ interface (SSIF), specific for Aspeed AST2500 SoC.
+
 config IPMB_DEVICE_INTERFACE
tristate 'IPMB Interface handler'
depends on I2C
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 0822adc2ec41..05b993f7335b 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -27,3 +27,5 @@ obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
 obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
 obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
 obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
+obj-$(CONFIG_SSIF_IPMI_BMC) += ssif_bmc.o
+obj-$(CONFIG_ASPEED_SSIF_IPMI_BMC) += ssif_bmc_aspeed.o
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
new file mode 100644
index ..ae6e8750c795
--- /dev/null
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -0,0 +1,645 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The driver for BMC side of SSIF interface
+ *
+ * Copyright (c) 2021, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "ssif_bmc.h"
+
+/*
+ * Call in WRITE context
+ */
+static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool 
non_blocking)
+{
+   unsigned long flags;
+   int ret;
+
+   if (!non_blocking) {
+retry:
+   ret = wait_event_interruptible(ssif_bmc->wait_queue,
+  !ssif_bmc->response_in_progress);
+   if (ret)
+   return ret;
+   }
+
+   spin_lock_irqsave(_bmc->lock, flags);
+   if (ssif_bmc->response_in_progress) {
+   spin_unlock_irqrestore(_bmc->lock, flags);
+   if (non_blocking)
+   return -EAGAIN;
+
+   goto retry;
+   }
+
+   /*
+* Check the response data length from userspace to determine the type
+* of the response message whether it is single-part or multi-part.
+*/
+   ssif_bmc->is_singlepart_read =
+   (ssif_msg_len(_bmc->response) <= 
(MAX_PAYLOAD_PER_TRANSACTION + 1)) ?
+   true : false; /* 1: byte of length */
+
+   ssif_bmc->response_in_progress = true;
+   spin_unlock_irqrestore(_bmc->lock, flags);
+
+   return 0;
+}
+
+/*
+ * Call in READ context
+ */
+static int receive_ssif_bmc_request(struct ssif_bmc_ctx *ssif_bmc, bool 
non_blocking)
+{
+   unsigned long flags;
+   int ret;
+
+   if (!non_blocking) {
+retry:
+   ret = wait_event_interruptible(ssif_bmc->wait_queue,
+