Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver

From: Wang, Haiyue
Date: Wed Jan 17 2018 - 09:31:18 EST

Next message: Wang, Haiyue: "Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver"
Previous message: Josh Poimboeuf: "Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()"
In reply to: Corey Minyard: "Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver"
Next in thread: Corey Minyard: "Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2018-01-17 06:12, Corey Minyard wrote:

On 01/16/2018 02:59 PM, Corey Minyard wrote:

On 01/16/2018 05:43 AM, Haiyue Wang wrote:

The KCS (Keyboard Controller Style) interface is used to perform in-band
IPMI communication between a server host and its BMC (BaseBoard Management
Controllers).

This driver exposes the KCS interface on ASpeed SOCs (AST2400 and AST2500)
as a character device. Such SOCs are commonly used as BMCs and this driver
implements the BMC side of the KCS interface.

I thought we were going to unify the BMC ioctl interface?Â My preference would be to
create a file named include/uapi/linux/ipmi-bmc.h and add the following:

#define __IPMI_BMC_IOCTL_MAGICÂÂÂ 0xb1
#define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)

to make it the same as BT.Â Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
IPMI_BMC_IOCTL_SMS_SET_ATN.Â Then add the KCS ioctls in ipmi-bmc.h and
use that.Â That way we stay backward compatible but we are unified.

Since more KCS interfaces may come around, can you make the name more
specific?Â (I made this same error on bt-bmc,c, it should probably be renamed.)

How about these IOCTL definitions ? Is it more specific ?

#define IPMI_BMC_IOCTL_SET_SMS_ATNÂÂÂ _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
#define IPMI_BMC_IOCTL_CLEAR_SMS_ATNÂ _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
#define IPMI_BMC_IOCTL_FORCE_ABORTÂÂÂ _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)

More comments inline, as I'll go ahead and review this.

Signed-off-by: Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>
---
Â .../devicetree/bindings/ipmi/aspeed-kcs-bmc.txtÂÂÂ |Â 26 +
Â drivers/char/ipmi/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 9 +
Â drivers/char/ipmi/MakefileÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
Â drivers/char/ipmi/kcs-bmc.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | 744 +++++++++++++++++++++
Â include/uapi/linux/kcs-bmc.hÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 14 +
Â 5 files changed, 794 insertions(+)
Â create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
Â create mode 100644 drivers/char/ipmi/kcs-bmc.c
Â create mode 100644 include/uapi/linux/kcs-bmc.h

diff --git a/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
new file mode 100644
index 0000000..dd0c73d
--- /dev/null
+++ b/Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt
@@ -0,0 +1,26 @@
+* Aspeed KCS (Keyboard Controller Style) IPMI interface
+
+The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
+(BaseBoard Management Controllers) and the KCS interface can be
+used to perform in-band IPMI communication with their host.
+
+Required properties:
+- compatible : should be one of
+ÂÂÂ "aspeed,ast2400-kcs-bmc"
+ÂÂÂ "aspeed,ast2500-kcs-bmc"
+- interrupts : interrupt generated by the controller
+- kcs_chan : The LPC channel number in the controller
+- kcs_addr : The host CPU IO map address
+
+
+Example:
+
+ÂÂÂ kcs3: kcs3@0 {
+ÂÂÂÂÂÂÂ compatible = "aspeed,ast2500-kcs-bmc";
+ÂÂÂÂÂÂÂ reg = <0x0 0x80>;
+ÂÂÂÂÂÂÂ interrupts = <8>;
+ÂÂÂÂÂÂÂ kcs_chan = <3>;
+ÂÂÂÂÂÂÂ kcs_addr = <0xCA2>;
+ÂÂÂÂÂÂÂ status = "okay";
+ÂÂÂ };
+
diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 3544abc..36132f8 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -104,3 +104,12 @@ config ASPEED_BT_IPMI_BMC
ÂÂÂÂÂÂÂ Provides a driver for the BT (Block Transfer) IPMI interface
ÂÂÂÂÂÂÂ found on Aspeed SOCs (AST2400 and AST2500). The driver
ÂÂÂÂÂÂÂ implements the BMC side of the BT interface.
+
+config ASPEED_KCS_IPMI_BMC
+ÂÂÂ depends on ARCH_ASPEED || COMPILE_TEST
+ÂÂÂ select REGMAP_MMIO
+ÂÂÂ tristate "KCS IPMI bmc driver"
+ÂÂÂ help
+ÂÂÂÂÂ Provides a driver for the KCS (Keyboard Controller Style) IPMI
+ÂÂÂÂÂ interface found on Aspeed SOCs (AST2400 and AST2500). The driver
+ÂÂÂÂÂ implements the BMC side of the KCS interface.
\ No newline at end of file
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 33b899f..f217bae 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -22,3 +22,4 @@ obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
Â obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
Â obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
Â obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
+obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs-bmc.o
\ No newline at end of file
diff --git a/drivers/char/ipmi/kcs-bmc.c b/drivers/char/ipmi/kcs-bmc.c
new file mode 100644
index 0000000..d6eab0b
--- /dev/null
+++ b/drivers/char/ipmi/kcs-bmc.c
@@ -0,0 +1,744 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2015-2018, Intel Corporation.
+
+#include <linux/atomic.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kcs-bmc.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+
+#define KCS_MSG_BUFSIZÂÂÂÂÂ 1024
+#define KCS_CHANNEL_MAXÂÂÂÂ 4
+
+/*
+ * This is a BMC device used to communicate to the host
+ */
+#define DEVICE_NAMEÂÂÂÂ "ipmi-kcs-host"
+
+
+/* Different Phases of the KCS Module */
+#define KCS_PHASE_IDLEÂÂÂÂÂÂÂÂÂ 0x00
+#define KCS_PHASE_WRITEÂÂÂÂÂÂÂÂ 0x01
+#define KCS_PHASE_WRITE_ENDÂÂÂÂ 0x02
+#define KCS_PHASE_READÂÂÂÂÂÂÂÂÂ 0x03
+#define KCS_PHASE_ABORTÂÂÂÂÂÂÂÂ 0x04
+#define KCS_PHASE_ERRORÂÂÂÂÂÂÂÂ 0x05

It would be nicer to make the above an enum.

Done!

+
+/* Abort Phase */
+#define ABORT_PHASE_ERROR1ÂÂÂÂÂ 0x01
+#define ABORT_PHASE_ERROR2ÂÂÂÂÂ 0x02

Can the above just be folded into two separate phases in kcs_phase?
That would be a little cleaner.

Done, the code is truly cleaner, thanks! ;-)

+
+/* KCS Command Control codes. */
+#define KCS_GET_STATUSÂÂÂÂÂÂÂÂÂ 0x60
+#define KCS_ABORTÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x60
+#define KCS_WRITE_STARTÂÂÂÂÂÂÂÂ 0x61
+#define KCS_WRITE_ENDÂÂÂÂÂÂÂÂÂÂ 0x62
+#define KCS_READ_BYTEÂÂÂÂÂÂÂÂÂÂ 0x68
+
+/* Status bits.:
+ * - IDLE_STATE.Â Interface is idle. System software should not be expecting
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nor sending any data.
+ * - READ_STATE.Â BMC is transferring a packet to system software. System
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ software should be in the "Read Message" state.
+ * - WRITE_STATE. BMC is receiving a packet from system software. System
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ software should be writing a command to the BMC.
+ * - ERROR_STATE. BMC has detected a protocol violation at the interface level,
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ or the transfer has been aborted. System software can either
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ use the "Get_Status" control code to request the nature of
+ *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ the error, or it can just retry the command.
+ */
+#define KCS_IDLE_STATEÂÂÂÂÂÂÂÂÂÂ 0
+#define KCS_READ_STATEÂÂÂÂÂÂÂÂÂÂ 1
+#define KCS_WRITE_STATEÂÂÂÂÂÂÂÂÂ 2
+#define KCS_ERROR_STATEÂÂÂÂÂÂÂÂÂ 3
+
+/* KCS Error Codes */
+#define KCS_NO_ERRORÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00
+#define KCS_ABORTED_BY_COMMANDÂÂÂÂÂ 0x01
+#define KCS_ILLEGAL_CONTROL_CODEÂÂÂ 0x02
+#define KCS_LENGTH_ERRORÂÂÂÂÂÂÂÂÂÂÂ 0x06
+#define KCS_UNSPECIFIED_ERRORÂÂÂÂÂÂ 0xFF
+
+
+#define KCS_ZERO_DATAÂÂÂÂÂÂÂÂÂÂ 0
+
+/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
+#define KCS_STR_STATE(state)ÂÂÂÂÂÂÂ (state << 6)
+#define KCS_STR_STATE_MASKÂÂÂÂÂÂÂÂÂ KCS_STR_STATE(0x3)
+#define KCS_STR_CMD_DATÂÂÂÂÂÂÂÂÂÂÂÂ BIT(3)
+#define KCS_STR_SMS_ATNÂÂÂÂÂÂÂÂÂÂÂÂ BIT(2)
+#define KCS_STR_IBFÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(1)
+#define KCS_STR_OBFÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BIT(0)
+
+
+/* mapped to lpc-bmc@0 IO space */
+#define LPC_HICR0ÂÂÂÂÂÂÂÂÂÂÂ 0x000
+#defineÂÂÂÂ LPC_HICR0_LPC3EÂÂÂÂÂÂÂÂÂ BIT(7)
+#defineÂÂÂÂ LPC_HICR0_LPC2EÂÂÂÂÂÂÂÂÂ BIT(6)
+#defineÂÂÂÂ LPC_HICR0_LPC1EÂÂÂÂÂÂÂÂÂ BIT(5)
+#define LPC_HICR2ÂÂÂÂÂÂÂÂÂÂÂ 0x008
+#defineÂÂÂÂ LPC_HICR2_IBFIF3ÂÂÂÂÂÂÂÂ BIT(3)
+#defineÂÂÂÂ LPC_HICR2_IBFIF2ÂÂÂÂÂÂÂÂ BIT(2)
+#defineÂÂÂÂ LPC_HICR2_IBFIF1ÂÂÂÂÂÂÂÂ BIT(1)
+#define LPC_HICR4ÂÂÂÂÂÂÂÂÂÂÂ 0x010
+#defineÂÂÂÂ LPC_HICR4_LADR12ASÂÂÂÂÂÂ BIT(7)
+#defineÂÂÂÂ LPC_HICR4_KCSENBLÂÂÂÂÂÂÂ BIT(2)
+#define LPC_LADR3HÂÂÂÂÂÂÂÂÂÂ 0x014
+#define LPC_LADR3LÂÂÂÂÂÂÂÂÂÂ 0x018
+#define LPC_LADR12HÂÂÂÂÂÂÂÂÂ 0x01C
+#define LPC_LADR12LÂÂÂÂÂÂÂÂÂ 0x020
+#define LPC_IDR1ÂÂÂÂÂÂÂÂÂÂÂÂ 0x024
+#define LPC_IDR2ÂÂÂÂÂÂÂÂÂÂÂÂ 0x028
+#define LPC_IDR3ÂÂÂÂÂÂÂÂÂÂÂÂ 0x02C
+#define LPC_ODR1ÂÂÂÂÂÂÂÂÂÂÂÂ 0x030
+#define LPC_ODR2ÂÂÂÂÂÂÂÂÂÂÂÂ 0x034
+#define LPC_ODR3ÂÂÂÂÂÂÂÂÂÂÂÂ 0x038
+#define LPC_STR1ÂÂÂÂÂÂÂÂÂÂÂÂ 0x03C
+#define LPC_STR2ÂÂÂÂÂÂÂÂÂÂÂÂ 0x040
+#define LPC_STR3ÂÂÂÂÂÂÂÂÂÂÂÂ 0x044
+
+/* mapped to lpc-host@80 IO space */
+#define LPC_HICRBÂÂÂÂÂÂÂÂÂÂÂ 0x080
+#defineÂÂÂÂ LPC_HICRB_IBFIF4ÂÂÂÂÂÂÂÂ BIT(1)
+#defineÂÂÂÂ LPC_HICRB_LPC4EÂÂÂÂÂÂÂÂÂ BIT(0)
+#define LPC_LADR4ÂÂÂÂÂÂÂÂÂÂÂ 0x090
+#define LPC_IDR4ÂÂÂÂÂÂÂÂÂÂÂÂ 0x094
+#define LPC_ODR4ÂÂÂÂÂÂÂÂÂÂÂÂ 0x098
+#define LPC_STR4ÂÂÂÂÂÂÂÂÂÂÂÂ 0x09C
+
+
+/* IPMI 2.0 - 9.5, KCS Interface Registers */
+struct kcs_ioreg {
+ÂÂÂ u32 idr; /* Input Data Register */
+ÂÂÂ u32 odr; /* Output Data Register */
+ÂÂÂ u32 str; /* Status Register */
+};
+
+static const struct kcs_ioreg kcs_ioreg_map[KCS_CHANNEL_MAX] = {
+ÂÂÂ { .idr = LPC_IDR1, .odr = LPC_ODR1, .str = LPC_STR1 },
+ÂÂÂ { .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
+ÂÂÂ { .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
+ÂÂÂ { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
+};

One more thing...Â Why aren't the above values being set in the device tree?
Passing in a channel (and address) seems inflexible.Â Kind of goes against the
philosophy of device trees.

-corey

Change it by referring to Joel's suggestion, and defining IDR/ODR/STR registers together with other
registers for LPC KCS, looks like this made code be more easily maintained.

https://lists.ozlabs.org/pipermail/openbmc/2017-December/010095.html

+
+struct kcs_bmc {
+ÂÂÂ struct regmap *map;
+ÂÂÂ spinlock_tÂÂÂÂ lock;

This lock is only used in threads, as far as I can tell. Couldn't it just be a normal mutex?
But more on this later.

I missed this lock using in KCS ISR function, for AST2500 is single core CPU. The critical data such as
data_in_avail is shared between ISR and user thread, spinlock_t related API should be the right one ?
especially for SMP ?

static irqreturn_t kcs_bmc_irq(int irq, void *arg)
{
ÂÂÂ ....
ÂÂÂ spin_lock(&kcs_bmc->lock);Â // <-- MISSED

ÂÂÂ switch (sts) {
ÂÂÂ case KCS_STR_IBF | KCS_STR_CMD_DAT:
ÂÂÂ ÂÂÂ kcs_rx_cmd(kcs_bmc);
ÂÂÂ ÂÂÂ break;

ÂÂÂ case KCS_STR_IBF:
ÂÂÂ ÂÂÂ kcs_rx_data(kcs_bmc);
ÂÂÂ ÂÂÂ break;

ÂÂÂ default:
ÂÂÂ ÂÂÂ ret = IRQ_NONE;
ÂÂÂ ÂÂÂ break;
ÂÂÂ }

ÂÂÂ spin_unlock(&kcs_bmc->lock); // <-- MISSED

ÂÂÂ return ret;
}

+
+ÂÂÂ u32 chan;
+ÂÂÂ int running;
+
+ÂÂÂ u32 idr;
+ÂÂÂ u32 odr;
+ÂÂÂ u32 str;
+
+ÂÂÂ int kcs_phase;
+ÂÂÂ u8Â abort_phase;
+ÂÂÂ u8Â kcs_error;
+
+ÂÂÂ wait_queue_head_t queue;
+ÂÂÂ intÂ data_in_avail;

data_in_avail should be a bool.

You set data_in_avail after the data is ready, but you don't have a barrier.Â You
have similar problems with kcs_phase.

In fact, the locking in the driver doesn't seem quite correct. If this ever ran on
an SMP system, it is likely to not work correctly.Â You can assume that the interrupt
runs exclusively, but you cannot assume that the data operations are available in
order on another processor that handles a subsequent interrupt.

The easiest way to fix this would be to add the spin lock around the case statement
in the irq handler and add them in the poll and read functions (you would need to
leave it as a spinlock in that case).Â That would provide the proper barriers assuming
you put them in the right place (checking data_in_avail again inside the spinlock
in the read case, for instance).

Got it!

+ÂÂÂ intÂ data_in_idx;
+ÂÂÂ u8Â *data_in;
+
+ÂÂÂ intÂ data_out_idx;
+ÂÂÂ intÂ data_out_len;
+ÂÂÂ u8Â *data_out;
+
+ÂÂÂ struct miscdevice miscdev;
+};
+
+static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
+{
+ÂÂÂ u32 val = 0;
+ÂÂÂ int rc;
+
+ÂÂÂ rc = regmap_read(kcs_bmc->map, reg, &val);
+ÂÂÂ WARN(rc != 0, "regmap_read() failed: %d\n", rc);
+
+ÂÂÂ return rc == 0 ? (u8) val : 0;
+}
+
+static void kcs_outb(struct kcs_bmc *kcs_bmc, u8 data, u32 reg)
+{
+ÂÂÂ int rc;
+
+ÂÂÂ rc = regmap_write(kcs_bmc->map, reg, data);
+ÂÂÂ WARN(rc != 0, "regmap_write() failed: %d\n", rc);
+}
+
+static inline void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
+{
+ÂÂÂ regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_STATE_MASK,
+ÂÂÂÂÂÂÂÂÂÂÂ KCS_STR_STATE(state));
+}
+
+static inline void kcs_set_atn(struct kcs_bmc *kcs_bmc)
+{
+ÂÂÂ regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
+ÂÂÂÂÂÂÂÂÂÂÂ KCS_STR_SMS_ATN);
+}
+
+static inline void kcs_clear_atn(struct kcs_bmc *kcs_bmc)
+{
+ÂÂÂ regmap_update_bits(kcs_bmc->map, kcs_bmc->str, KCS_STR_SMS_ATN,
+ÂÂÂÂÂÂÂÂÂÂÂ 0);
+}
+
+/*
+ * AST_usrGuide_KCS.pdf
+ * 2. Background:
+ *ÂÂ we note D for Data, and C for Cmd/Status, default rules are
+ *ÂÂÂÂ A. KCS1 / KCS2 ( D / C:X / X+4 )
+ *ÂÂÂÂÂÂÂ D / C : CA0h / CA4h
+ *ÂÂÂÂÂÂÂ D / C : CA8h / CACh
+ *ÂÂÂÂ B. KCS3 ( D / C:XX2h / XX3h )
+ *ÂÂÂÂÂÂÂ D / C : CA2h / CA3h
+ *ÂÂÂÂÂÂÂ D / C : CB2h / CB3h
+ *ÂÂÂÂ C. KCS4
+ *ÂÂÂÂÂÂÂ D / C : CA4h / CA5h
+ */
+static void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
+{
+ÂÂÂ switch (kcs_bmc->chan) {
+ÂÂÂ case 1:
+ÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR4,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR4_LADR12AS, 0);
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case 2:
+ÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR4,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR12H, addr >> 8);
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR12L, addr & 0xFF);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case 3:
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR3H, addr >> 8);
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR3L, addr & 0xFF);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case 4:
+ÂÂÂÂÂÂÂ regmap_write(kcs_bmc->map, LPC_LADR4, ((addr + 1) << 16) |
+ÂÂÂÂÂÂÂÂÂÂÂ addr);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ default:

Shouldn't you return an error here?

For I checked the channel number on kcs_bmc_probe, still need this kind of error handling ?

ÂÂÂ rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
ÂÂÂ if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
ÂÂÂ ÂÂÂ dev_err(dev, "no valid 'kcs_chan' configured\n");
ÂÂÂ ÂÂÂ return -ENODEV;
ÂÂÂ }

+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+}
+
+static void kcs_enable_channel(struct kcs_bmc *kcs_bmc, int enable)
+{
+ÂÂÂ switch (kcs_bmc->chan) {
+ÂÂÂ case 1:
+ÂÂÂÂÂÂÂ if (enable) {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR2,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR0_LPC1E, 0);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR2,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR2_IBFIF1, 0);
+ÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case 2:
+ÂÂÂÂÂÂÂ if (enable) {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR2,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR0_LPC2E, 0);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR2,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR2_IBFIF2, 0);
+ÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case 3:
+ÂÂÂÂÂÂÂ if (enable) {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR2,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR4,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR0_LPC3E, 0);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR4,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR4_KCSENBL, 0);
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICR2,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICR2_IBFIF3, 0);
+ÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case 4:
+ÂÂÂÂÂÂÂ if (enable) {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICRB,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E);
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ regmap_update_bits(kcs_bmc->map, LPC_HICRB,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0);
+ÂÂÂÂÂÂÂ }

The above shouldn't have {}, did you run this through checkpatch?

Yes, I run the checkpatch, no this warning. ;-) But well, I will remove the '{}'.

+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ default:

Error here, too?

For I checked the channel number on kcs_bmc_probe, still need this kind of error handling ?

+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+}
+
+static void kcs_rx_data(struct kcs_bmc *kcs_bmc)
+{
+ÂÂÂ u8 data;
+
+ÂÂÂ switch (kcs_bmc->kcs_phase) {
+ÂÂÂ case KCS_PHASE_WRITE:
+ÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_WRITE_STATE);
+
+ÂÂÂÂÂÂÂ /* set OBF before reading data */
+ÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+
+ÂÂÂÂÂÂÂ if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ)
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_PHASE_WRITE_END:
+ÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_READ_STATE);
+
+ÂÂÂÂÂÂÂ if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ)
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_phase = KCS_PHASE_READ;
+ÂÂÂÂÂÂÂ if (kcs_bmc->running) {
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->data_in_avail = 1;
+ÂÂÂÂÂÂÂÂÂÂÂ wake_up_interruptible(&kcs_bmc->queue);
+ÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_PHASE_READ:
+ÂÂÂÂÂÂÂ if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len)
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_IDLE_STATE);
+
+ÂÂÂÂÂÂÂ data = kcs_inb(kcs_bmc, kcs_bmc->idr);
+ÂÂÂÂÂÂÂ if (data != KCS_READ_BYTE) {
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+ÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) {
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->kcs_phase = KCS_PHASE_IDLE;
+ÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, kcs_bmc->data_out[kcs_bmc->data_out_idx++],
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->odr);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_PHASE_ABORT:
+ÂÂÂÂÂÂÂ switch (kcs_bmc->abort_phase) {
+ÂÂÂÂÂÂÂ case ABORT_PHASE_ERROR1:
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_READ_STATE);
+
+ÂÂÂÂÂÂÂÂÂÂÂ /* Read the Dummy byte */
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, kcs_bmc->kcs_error, kcs_bmc->odr);
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->abort_phase = ABORT_PHASE_ERROR2;
+ÂÂÂÂÂÂÂÂÂÂÂ break;
+
+ÂÂÂÂÂÂÂ case ABORT_PHASE_ERROR2:
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_IDLE_STATE);
+
+ÂÂÂÂÂÂÂÂÂÂÂ /* Read the Dummy byte */
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->kcs_phase = KCS_PHASE_IDLE;
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->abort_phase = 0;
+ÂÂÂÂÂÂÂÂÂÂÂ break;
+
+ÂÂÂÂÂÂÂ default:
+ÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_PHASE_ERROR:

This is identical to the default.Â And the default should never happen, anyway.
If the default happens you have a software bug and need to report it.

For making code clean, I removed the KCS_PHASE_ERROR, just keep the 'default' handling.

+ÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+
+ÂÂÂÂÂÂÂ /* Read the Dummy byte */
+ÂÂÂÂÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+ÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+
+ÂÂÂÂÂÂÂ /* Read the Dummy byte */
+ÂÂÂÂÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+ÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+}
+
+static void kcs_rx_cmd(struct kcs_bmc *kcs_bmc)
+{
+ÂÂÂ u8 cmd;
+
+ÂÂÂ kcs_set_state(kcs_bmc, KCS_WRITE_STATE);
+
+ÂÂÂ /* Dummy data to generate OBF */
+ÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+
+ÂÂÂ cmd = kcs_inb(kcs_bmc, kcs_bmc->idr);

Wouldn't you want to read the command before you write the OBF?

The host SMS KCS state is : 'wait for IBF=0' --> 'wait for OBF=1' /Â 'clear OBF', for racing
condition, BMC needs prepare OBF=1, then clear IBF. ;-)

+ÂÂÂ switch (cmd) {
+ÂÂÂ case KCS_WRITE_START:
+ÂÂÂÂÂÂÂ kcs_bmc->data_in_avail = 0;
+ÂÂÂÂÂÂÂ kcs_bmc->data_in_idxÂÂ = 0;
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_phaseÂÂÂÂ = KCS_PHASE_WRITE;
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_errorÂÂÂÂ = KCS_NO_ERROR;
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_WRITE_END:
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_phase = KCS_PHASE_WRITE_END;
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_ABORT:
+ÂÂÂÂÂÂÂ if (kcs_bmc->kcs_error == KCS_NO_ERROR)
+ÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->kcs_error = KCS_ABORTED_BY_COMMAND;
+
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_phaseÂÂ = KCS_PHASE_ABORT;
+ÂÂÂÂÂÂÂ kcs_bmc->abort_phase = ABORT_PHASE_ERROR1;
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_error = KCS_ILLEGAL_CONTROL_CODE;
+ÂÂÂÂÂÂÂ kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+ÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, kcs_bmc->kcs_error, kcs_bmc->odr);
+ÂÂÂÂÂÂÂ kcs_bmc->kcs_phase = KCS_PHASE_ERROR;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+}
+
+/*
+ * Whenever the BMC is reset (from power-on or a hard reset), the State Bits
+ * are initialized to "11 - Error State". Doing so allows SMS to detect that
+ * the BMC has been reset and that any message in process has been terminated
+ * by the BMC.
+ */
+static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
+{
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ spin_lock_irqsave(&kcs_bmc->lock, flags);
+ÂÂÂ kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+
+ÂÂÂ /* Read the Dummy byte */
+ÂÂÂ kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+ÂÂÂ kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+ÂÂÂ kcs_bmc->kcs_phase = KCS_PHASE_ERROR;
+ÂÂÂ spin_unlock_irqrestore(&kcs_bmc->lock, flags);

You don't set data_in_avail() to zero here?

Done, added it.

+}
+
+static irqreturn_t kcs_bmc_irq(int irq, void *arg)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = arg;
+ÂÂÂ u32 sts;
+
+ÂÂÂ if (regmap_read(kcs_bmc->map, kcs_bmc->str, &sts) != 0)
+ÂÂÂÂÂÂÂ return IRQ_NONE;
+
+ÂÂÂ sts &= (KCS_STR_IBF | KCS_STR_CMD_DAT);
+
+ÂÂÂ switch (sts) {
+ÂÂÂ case KCS_STR_IBF | KCS_STR_CMD_DAT:
+ÂÂÂÂÂÂÂ kcs_rx_cmd(kcs_bmc);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_STR_IBF:
+ÂÂÂÂÂÂÂ kcs_rx_data(kcs_bmc);
+
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ return IRQ_NONE;
+ÂÂÂ }
+
+ÂÂÂ return IRQ_HANDLED;
+}
+
+static int kcs_bmc_config_irq(struct kcs_bmc *kcs_bmc,
+ÂÂÂÂÂÂÂÂÂÂÂ struct platform_device *pdev)
+{
+ÂÂÂ struct device *dev = &pdev->dev;
+ÂÂÂ int irq;
+
+ÂÂÂ irq = platform_get_irq(pdev, 0);
+ÂÂÂ if (irq < 0)
+ÂÂÂÂÂÂÂ return irq;
+
+ÂÂÂ return devm_request_irq(dev, irq, kcs_bmc_irq, IRQF_SHARED,
+ÂÂÂÂÂÂÂÂÂÂÂ dev_name(dev), kcs_bmc);
+}
+
+
+static inline struct kcs_bmc *file_kcs_bmc(struct file *filp)
+{
+ÂÂÂ return container_of(filp->private_data, struct kcs_bmc, miscdev);
+}
+
+static int kcs_bmc_open(struct inode *inode, struct file *filp)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ if (kcs_bmc->running)
+ÂÂÂÂÂÂÂ return -EBUSY;
+

The above is a race, it needs to be done inside the lock.

Fixed!

+ spin_lock_irqsave(&kcs_bmc->lock, flags);
+ÂÂÂ kcs_bmc->kcs_phaseÂÂÂÂ = KCS_PHASE_IDLE;
+ÂÂÂ kcs_bmc->runningÂÂÂÂÂÂ = 1;
+ÂÂÂ kcs_bmc->data_in_avail = 0;
+ÂÂÂ spin_unlock_irqrestore(&kcs_bmc->lock, flags);

What happens if the interface is not in a state where it can send a message?
The release code doesn't block until anything is done, so the interface might
not be in a place where you can use it.Â I think the init code handles it from
that side, but the release side is not handled.

Also, if release gets called, wouldn't you want to call kcs_force_abort() here
or in release()? That would be the equivalent of the BMC getting reset.

Yes, you are right. We meet this kind of bug that host is waiting BMC after it resets. So normally,
after the user ipmi stack is ready, it will call KCS_BMC_IOCTL_FORCE_ABORT, then the SMS can
get a right response.

+
+ÂÂÂ return 0;
+}
+
+static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+ÂÂÂ unsigned int mask = 0;
+
+ÂÂÂ poll_wait(filp, &kcs_bmc->queue, wait);
+
+ÂÂÂ if (kcs_bmc->data_in_avail)
+ÂÂÂÂÂÂÂ mask |= POLLIN;
+
+ÂÂÂ if (kcs_bmc->kcs_phase == KCS_PHASE_READ)
+ÂÂÂÂÂÂÂ mask |= POLLOUT;
+
+ÂÂÂ return mask;
+}
+
+static ssize_t kcs_bmc_read(struct file *filp, char *buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t count, loff_t *offset)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+ÂÂÂ int rv;
+

You probably ought to handle O_NONBLOCK here.Â (Same problem on BT, too.)

Got it, will add this.

+ÂÂÂ rv = wait_event_interruptible(kcs_bmc->queue,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ kcs_bmc->data_in_avail != 0);
+ÂÂÂ if (rv < 0)
+ÂÂÂÂÂÂÂ return -ERESTARTSYS;
+

This is a race condition for multiple users.

Got it, will fix this.

+ÂÂÂ kcs_bmc->data_in_avail = 0;
+
+ÂÂÂ if (count > kcs_bmc->data_in_idx)
+ÂÂÂÂÂÂÂ count = kcs_bmc->data_in_idx;
+
+ÂÂÂ if (copy_to_user(buf, kcs_bmc->data_in, count))
+ÂÂÂÂÂÂÂ return -EFAULT;
+
+ÂÂÂ return count;
+}
+
+static ssize_t kcs_bmc_write(struct file *filp, const char *buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t count, loff_t *offset)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ if (count < 1 || count > KCS_MSG_BUFSIZ)
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ if (copy_from_user(kcs_bmc->data_out, buf, count))
+ÂÂÂÂÂÂÂ return -EFAULT;
+
+ÂÂÂ spin_lock_irqsave(&kcs_bmc->lock, flags);
+ÂÂÂ if (kcs_bmc->kcs_phase == KCS_PHASE_READ) {

If you don't modify kcs_phase here, you have a race condition. You probably
need a KCS_WAIT_READ condition.Â Also, the nomenclature of "read" and "write"
here is a little confusing, since your phases are from the host's point of view,
not this driver's point of view.Â You might want to document that explicitly.

The race condition means that the user MAY write the duplicated response ?
Will write some comments for these phase definition.

+ÂÂÂÂÂÂÂ kcs_bmc->data_out_idx = 1;
+ÂÂÂÂÂÂÂ kcs_bmc->data_out_len = count;
+ÂÂÂÂÂÂÂ kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
+ÂÂÂ }

So if you write and the data isn't ready, you just drop the data on the floor silently?
Probably not the best design.Â You should probably return an error, as write can
only be called after read.

Yes, you are right, will return the right error code instead. ;-)

+ spin_unlock_irqrestore(&kcs_bmc->lock, flags);
+
+ÂÂÂ return count;
+}
+
+static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long arg)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+ÂÂÂ long ret = 0;
+
+ÂÂÂ switch (cmd) {
+ÂÂÂ case KCS_BMC_IOCTL_SET_ATN:
+ÂÂÂÂÂÂÂ kcs_set_atn(kcs_bmc);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_BMC_IOCTL_CLR_ATN:
+ÂÂÂÂÂÂÂ kcs_clear_atn(kcs_bmc);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ case KCS_BMC_IOCTL_FORCE_ABORT:
+ÂÂÂÂÂÂÂ kcs_force_abort(kcs_bmc);
+ÂÂÂÂÂÂÂ break;
+
+ÂÂÂ default:
+ÂÂÂÂÂÂÂ ret = -EINVAL;
+ÂÂÂÂÂÂÂ break;
+ÂÂÂ }
+
+ÂÂÂ return ret;
+}
+
+static int kcs_bmc_release(struct inode *inode, struct file *filp)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ spin_lock_irqsave(&kcs_bmc->lock, flags);
+ÂÂÂ kcs_bmc->running = 0;
+ÂÂÂ spin_unlock_irqrestore(&kcs_bmc->lock, flags);
+
+ÂÂÂ return 0;
+}
+
+static const struct file_operations kcs_bmc_fops = {
+ÂÂÂ .ownerÂÂÂÂÂÂÂÂÂ = THIS_MODULE,
+ÂÂÂ .openÂÂÂÂÂÂÂÂÂÂ = kcs_bmc_open,
+ÂÂÂ .readÂÂÂÂÂÂÂÂÂÂ = kcs_bmc_read,
+ÂÂÂ .writeÂÂÂÂÂÂÂÂÂ = kcs_bmc_write,
+ÂÂÂ .releaseÂÂÂÂÂÂÂ = kcs_bmc_release,
+ÂÂÂ .pollÂÂÂÂÂÂÂÂÂÂ = kcs_bmc_poll,
+ÂÂÂ .unlocked_ioctl = kcs_bmc_ioctl,
+};
+
+static int kcs_bmc_probe(struct platform_device *pdev)
+{
+ÂÂÂ struct device *dev = &pdev->dev;
+ÂÂÂ const struct kcs_ioreg *ioreg;
+ÂÂÂ struct kcs_bmc *kcs_bmc;
+ÂÂÂ u32 chan, addr;
+ÂÂÂ int rc;
+
+ÂÂÂ kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
+ÂÂÂ if (!kcs_bmc)
+ÂÂÂÂÂÂÂ return -ENOMEM;

Every error after this point will leak kcs_bmc.Â I'd recommend a "goto out_err"
to handle this.

It is 'devm_xxx' API, it will be released automatically by the dev framework. It also can auto-release
the irq, so that probe function can be designed clearly. This is the first time I know about this. ;-)

+
+ÂÂÂ rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
+ÂÂÂ if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {
+ÂÂÂÂÂÂÂ dev_err(dev, "no valid 'kcs_chan' configured\n");
+ÂÂÂÂÂÂÂ return -ENODEV;
+ÂÂÂ }
+
+ÂÂÂ rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
+ÂÂÂ if (rc) {
+ÂÂÂÂÂÂÂ dev_err(dev, "no valid 'kcs_addr' configured\n");
+ÂÂÂÂÂÂÂ return -ENODEV;
+ÂÂÂ }
+
+ÂÂÂ kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
+ÂÂÂ if (IS_ERR(kcs_bmc->map)) {
+ÂÂÂÂÂÂÂ dev_err(dev, "Couldn't get regmap\n");
+ÂÂÂÂÂÂÂ return -ENODEV;
+ÂÂÂ }
+
+ÂÂÂ dev_set_name(dev, "ipmi-kcs%u", chan);
+
+ÂÂÂ spin_lock_init(&kcs_bmc->lock);
+ÂÂÂ kcs_bmc->chan = chan;

You need error checking on chan.

Has been checked above : if ((rc != 0) || (chan == 0 || chan > KCS_CHANNEL_MAX)) {

+
+ÂÂÂ ioreg = &kcs_ioreg_map[chan - 1];
+ÂÂÂ kcs_bmc->idrÂ = ioreg->idr;
+ÂÂÂ kcs_bmc->odrÂ = ioreg->odr;
+ÂÂÂ kcs_bmc->strÂ = ioreg->str;
+
+ÂÂÂ init_waitqueue_head(&kcs_bmc->queue);
+ÂÂÂ kcs_bmc->data_inÂ = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+ÂÂÂ kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+ÂÂÂ if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
+ÂÂÂÂÂÂÂ dev_err(dev, "Failed to allocate data buffers\n");

You will leak memory if you fail to allocate data_out but do allocate data_in.

It is 'devm_xxx' API, it will be released automatically by the dev framework.;-)

+ÂÂÂÂÂÂÂ return -ENOMEM;
+ÂÂÂ }
+
+ÂÂÂ kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
+ÂÂÂ kcs_bmc->miscdev.name = dev_name(dev);
+ÂÂÂ kcs_bmc->miscdev.fops = &kcs_bmc_fops;
+ÂÂÂ rc = misc_register(&kcs_bmc->miscdev);
+ÂÂÂ if (rc) {
+ÂÂÂÂÂÂÂ dev_err(dev, "Unable to register device\n");
+ÂÂÂÂÂÂÂ return rc;
+ÂÂÂ }

After you call misc_register something can open the device and use it.
You need to do that after everything is enabled.

Got it, will change the code order.

+
+ÂÂÂ kcs_set_addr(kcs_bmc, addr);
+ÂÂÂ kcs_enable_channel(kcs_bmc, 1);
+
+ÂÂÂ rc = kcs_bmc_config_irq(kcs_bmc, pdev);
+ÂÂÂ if (rc) {
+ÂÂÂÂÂÂÂ misc_deregister(&kcs_bmc->miscdev);
+ÂÂÂÂÂÂÂ return rc;
+ÂÂÂ }
+
+ÂÂÂ dev_set_drvdata(&pdev->dev, kcs_bmc);

ThisÂ should definitely be before you enable or register.Â The drvdata
needs to be available in case an irq comes in, for instance.

Got it, will change the code order.

+
+ÂÂÂ dev_info(dev, "addr=0x%x, idr=0x%x, odr=0x%x, str=0x%x\n",
+ÂÂÂÂÂÂÂ addr, kcs_bmc->idr, kcs_bmc->odr, kcs_bmc->str);
+
+ÂÂÂ return 0;
+}
+
+static int kcs_bmc_remove(struct platform_device *pdev)
+{
+ÂÂÂ struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
+
+ÂÂÂ misc_deregister(&kcs_bmc->miscdev);
+
+ÂÂÂ return 0;
+}
+
+static const struct of_device_id kcs_bmc_match[] = {
+ÂÂÂ { .compatible = "aspeed,ast2400-kcs-bmc" },
+ÂÂÂ { .compatible = "aspeed,ast2500-kcs-bmc" },
+ÂÂÂ { }
+};
+
+static struct platform_driver kcs_bmc_driver = {
+ÂÂÂ .driver = {
+ÂÂÂÂÂÂÂ .nameÂÂÂÂÂÂÂÂÂÂ = DEVICE_NAME,
+ÂÂÂÂÂÂÂ .of_match_table = kcs_bmc_match,
+ÂÂÂ },
+ÂÂÂ .probe = kcs_bmc_probe,
+ÂÂÂ .remove = kcs_bmc_remove,
+};
+
+module_platform_driver(kcs_bmc_driver);
+
+MODULE_DEVICE_TABLE(of, kcs_bmc_match);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("Linux device interface to the IPMI KCS interface");
diff --git a/include/uapi/linux/kcs-bmc.h b/include/uapi/linux/kcs-bmc.h
new file mode 100644
index 0000000..d1550d3
--- /dev/null
+++ b/include/uapi/linux/kcs-bmc.h
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2015-2018, Intel Corporation.
+
+#ifndef _UAPI_LINUX_KCS_BMC_H
+#define _UAPI_LINUX_KCS_BMC_H
+
+#include <linux/ioctl.h>
+
+#define __KCS_BMC_IOCTL_MAGICÂÂÂÂÂÂÂ 'K'
+#define KCS_BMC_IOCTL_SET_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 1)
+#define KCS_BMC_IOCTL_CLR_ATN _IO(__KCS_BMC_IOCTL_MAGIC, 2)
+#define KCS_BMC_IOCTL_FORCE_ABORT _IO(__KCS_BMC_IOCTL_MAGIC, 3)
+
+#endif /* _UAPI_LINUX_KCS_BMC_H */

Next message: Wang, Haiyue: "Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver"
Previous message: Josh Poimboeuf: "Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()"
In reply to: Corey Minyard: "Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver"
Next in thread: Corey Minyard: "Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]