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

From: Corey Minyard
Date: Wed Jan 17 2018 - 11:31:37 EST

On 01/17/2018 08:31 AM, Wang, Haiyue wrote:



Snip...

+/* 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


Hmm, the code that you had there seemed ok to me (though you didn't check that you got three
registers).

I'm ok either way. The idea of the device tree is that if someone has this same device,
they just add a device tree and it works without modifying the driver. If Joel doesn't
think it's worth it, then I guess it's ok.


+
+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 ?


Sort of. In the case below, you need to use spin_lock_irqsave(), you don't necessarily get
here with interrupts disabled.

In the ones called from user context, you should really use spin_lock_irq(). Interrupts
should always be on at that point, so it's better.

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;
ÂÂÂ }


That's fine.

+ÂÂÂÂÂÂÂ 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 ?

Just checking one place is fine :).

+ÂÂÂÂÂÂÂ 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 ?

Not exactly. Two threads can call this, and if it hasn't transitions from the read phase,
the data out will be overwritten.

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. ;-)

Oh, you are right, yes. Sorry about that.

+
+ÂÂÂ 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)) {

Yeah, sorry I missed that.

-corey

+
+ÂÂÂ 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 */