From: Zev Weiss <zweiss@equinix.com>
To: Andrew Jeffery <andrew@aj.id.au>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"Chia-Wei, Wang" <chiawei_wang@aspeedtech.com>,
Ryan Chen <ryan_chen@aspeedtech.com>,
Tomer Maimon <tmaimon77@gmail.com>,
Corey Minyard <minyard@acm.org>,
Avi Fishman <avifishman70@gmail.com>,
Patrick Venture <venture@google.com>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Tali Perry <tali.perry1@gmail.com>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"openipmi-developer@lists.sourceforge.net"
<openipmi-developer@lists.sourceforge.net>,
Lee Jones <lee.jones@linaro.org>,
Linus Walleij <linus.walleij@linaro.org>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
Benjamin Fair <benjaminfair@google.com>
Subject: Re: [PATCH v2 09/21] ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi
Date: Fri, 9 Apr 2021 19:21:34 +0000 [thread overview]
Message-ID: <YHCpPa3EtVy9XcLn@packtop> (raw)
In-Reply-To: <6aa7c000-da09-4058-96b4-f330193c7fc6@www.fastmail.com>
On Fri, Apr 09, 2021 at 12:48:21AM CDT, Andrew Jeffery wrote:
>
>
>On Fri, 9 Apr 2021, at 13:26, Zev Weiss wrote:
>> On Fri, Mar 19, 2021 at 01:27:40AM CDT, Andrew Jeffery wrote:
>> >Take steps towards defining a coherent API to separate the KCS device
>> >drivers from the userspace interface. Decreasing the coupling will
>> >improve the separation of concerns and enable the introduction of
>> >alternative userspace interfaces.
>> >
>> >For now, simply split the chardev logic out to a separate file. The code
>> >continues to build into the same module.
>> >
>> >Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> >---
>> > drivers/char/ipmi/Makefile | 2 +-
>> > drivers/char/ipmi/kcs_bmc.c | 423 +------------------------
>> > drivers/char/ipmi/kcs_bmc.h | 10 +-
>> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 428 ++++++++++++++++++++++++++
>> > 4 files changed, 451 insertions(+), 412 deletions(-)
>> > create mode 100644 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
>> >
>> >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
>> >index 0822adc2ec41..a302bc865370 100644
>> >--- a/drivers/char/ipmi/Makefile
>> >+++ b/drivers/char/ipmi/Makefile
>> >@@ -22,7 +22,7 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
>> > obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
>> > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
>> > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
>> >-obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
>> >+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o
>> > 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
>> >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>> >index c4336c1f2d6d..ef5c48ffe74a 100644
>> >--- a/drivers/char/ipmi/kcs_bmc.c
>> >+++ b/drivers/char/ipmi/kcs_bmc.c
>> >@@ -3,446 +3,51 @@
>> > * Copyright (c) 2015-2018, Intel Corporation.
>> > */
>> >
>> >-#define pr_fmt(fmt) "kcs-bmc: " fmt
>> >-
>> >-#include <linux/errno.h>
>> >-#include <linux/io.h>
>> >-#include <linux/ipmi_bmc.h>
>> > #include <linux/module.h>
>> >-#include <linux/platform_device.h>
>> >-#include <linux/poll.h>
>> >-#include <linux/sched.h>
>> >-#include <linux/slab.h>
>> >
>> > #include "kcs_bmc.h"
>> >
>> >-#define DEVICE_NAME "ipmi-kcs"
>> >-
>> >-#define KCS_MSG_BUFSIZ 1000
>> >-
>> >-#define KCS_ZERO_DATA 0
>> >-
>> >-
>> >-/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
>> >-#define KCS_STATUS_STATE(state) (state << 6)
>> >-#define KCS_STATUS_STATE_MASK GENMASK(7, 6)
>> >-#define KCS_STATUS_CMD_DAT BIT(3)
>> >-#define KCS_STATUS_SMS_ATN BIT(2)
>> >-#define KCS_STATUS_IBF BIT(1)
>> >-#define KCS_STATUS_OBF BIT(0)
>> >-
>> >-/* IPMI 2.0 - Table 9-2, KCS Interface State Bits */
>> >-enum kcs_states {
>> >- IDLE_STATE = 0,
>> >- READ_STATE = 1,
>> >- WRITE_STATE = 2,
>> >- ERROR_STATE = 3,
>> >-};
>> >-
>> >-/* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */
>> >-#define KCS_CMD_GET_STATUS_ABORT 0x60
>> >-#define KCS_CMD_WRITE_START 0x61
>> >-#define KCS_CMD_WRITE_END 0x62
>> >-#define KCS_CMD_READ_BYTE 0x68
>> >-
>> >-static inline u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
>> >+u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
>> > {
>> > return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
>> > }
>> >+EXPORT_SYMBOL(kcs_bmc_read_data);
>> >
>> >-static inline void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
>> >+void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
>> > {
>> > kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
>> > }
>> >+EXPORT_SYMBOL(kcs_bmc_write_data);
>> >
>> >-static inline u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
>> >+u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
>> > {
>> > return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
>> > }
>> >+EXPORT_SYMBOL(kcs_bmc_read_status);
>> >
>> >-static inline void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
>> >+void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
>> > {
>> > kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
>> > }
>> >+EXPORT_SYMBOL(kcs_bmc_write_status);
>> >
>> >-static void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
>> >+void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val)
>> > {
>> > kcs_bmc->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val);
>> > }
>> >+EXPORT_SYMBOL(kcs_bmc_update_status);
>> >
>> >-static inline void set_state(struct kcs_bmc *kcs_bmc, u8 state)
>> >-{
>> >- kcs_bmc_update_status(kcs_bmc, KCS_STATUS_STATE_MASK,
>> >- KCS_STATUS_STATE(state));
>> >-}
>> >-
>> >-static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
>> >-{
>> >- set_state(kcs_bmc, ERROR_STATE);
>> >- kcs_bmc_read_data(kcs_bmc);
>> >- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>> >-
>> >- kcs_bmc->phase = KCS_PHASE_ERROR;
>> >- kcs_bmc->data_in_avail = false;
>> >- kcs_bmc->data_in_idx = 0;
>> >-}
>> >-
>> >-static void kcs_bmc_handle_data(struct kcs_bmc *kcs_bmc)
>> >-{
>> >- u8 data;
>> >-
>> >- switch (kcs_bmc->phase) {
>> >- case KCS_PHASE_WRITE_START:
>> >- kcs_bmc->phase = KCS_PHASE_WRITE_DATA;
>> >- fallthrough;
>> >-
>> >- case KCS_PHASE_WRITE_DATA:
>> >- if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
>> >- set_state(kcs_bmc, WRITE_STATE);
>> >- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>> >- kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
>> >- kcs_bmc_read_data(kcs_bmc);
>> >- } else {
>> >- kcs_force_abort(kcs_bmc);
>> >- kcs_bmc->error = KCS_LENGTH_ERROR;
>> >- }
>> >- break;
>> >-
>> >- case KCS_PHASE_WRITE_END_CMD:
>> >- if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ) {
>> >- set_state(kcs_bmc, READ_STATE);
>> >- kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
>> >- kcs_bmc_read_data(kcs_bmc);
>> >- kcs_bmc->phase = KCS_PHASE_WRITE_DONE;
>> >- kcs_bmc->data_in_avail = true;
>> >- wake_up_interruptible(&kcs_bmc->queue);
>> >- } else {
>> >- kcs_force_abort(kcs_bmc);
>> >- kcs_bmc->error = KCS_LENGTH_ERROR;
>> >- }
>> >- break;
>> >-
>> >- case KCS_PHASE_READ:
>> >- if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len)
>> >- set_state(kcs_bmc, IDLE_STATE);
>> >-
>> >- data = kcs_bmc_read_data(kcs_bmc);
>> >- if (data != KCS_CMD_READ_BYTE) {
>> >- set_state(kcs_bmc, ERROR_STATE);
>> >- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>> >- break;
>> >- }
>> >-
>> >- if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) {
>> >- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>> >- kcs_bmc->phase = KCS_PHASE_IDLE;
>> >- break;
>> >- }
>> >-
>> >- kcs_bmc_write_data(kcs_bmc,
>> >- kcs_bmc->data_out[kcs_bmc->data_out_idx++]);
>> >- break;
>> >-
>> >- case KCS_PHASE_ABORT_ERROR1:
>> >- set_state(kcs_bmc, READ_STATE);
>> >- kcs_bmc_read_data(kcs_bmc);
>> >- kcs_bmc_write_data(kcs_bmc, kcs_bmc->error);
>> >- kcs_bmc->phase = KCS_PHASE_ABORT_ERROR2;
>> >- break;
>> >-
>> >- case KCS_PHASE_ABORT_ERROR2:
>> >- set_state(kcs_bmc, IDLE_STATE);
>> >- kcs_bmc_read_data(kcs_bmc);
>> >- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>> >- kcs_bmc->phase = KCS_PHASE_IDLE;
>> >- break;
>> >-
>> >- default:
>> >- kcs_force_abort(kcs_bmc);
>> >- break;
>> >- }
>> >-}
>> >-
>> >-static void kcs_bmc_handle_cmd(struct kcs_bmc *kcs_bmc)
>> >-{
>> >- u8 cmd;
>> >-
>> >- set_state(kcs_bmc, WRITE_STATE);
>> >- kcs_bmc_write_data(kcs_bmc, KCS_ZERO_DATA);
>> >-
>> >- cmd = kcs_bmc_read_data(kcs_bmc);
>> >- switch (cmd) {
>> >- case KCS_CMD_WRITE_START:
>> >- kcs_bmc->phase = KCS_PHASE_WRITE_START;
>> >- kcs_bmc->error = KCS_NO_ERROR;
>> >- kcs_bmc->data_in_avail = false;
>> >- kcs_bmc->data_in_idx = 0;
>> >- break;
>> >-
>> >- case KCS_CMD_WRITE_END:
>> >- if (kcs_bmc->phase != KCS_PHASE_WRITE_DATA) {
>> >- kcs_force_abort(kcs_bmc);
>> >- break;
>> >- }
>> >-
>> >- kcs_bmc->phase = KCS_PHASE_WRITE_END_CMD;
>> >- break;
>> >-
>> >- case KCS_CMD_GET_STATUS_ABORT:
>> >- if (kcs_bmc->error == KCS_NO_ERROR)
>> >- kcs_bmc->error = KCS_ABORTED_BY_COMMAND;
>> >-
>> >- kcs_bmc->phase = KCS_PHASE_ABORT_ERROR1;
>> >- kcs_bmc->data_in_avail = false;
>> >- kcs_bmc->data_in_idx = 0;
>> >- break;
>> >-
>> >- default:
>> >- kcs_force_abort(kcs_bmc);
>> >- kcs_bmc->error = KCS_ILLEGAL_CONTROL_CODE;
>> >- break;
>> >- }
>> >-}
>> >-
>> >+int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc);
>>
>> This declaration looks a bit out of place here; should it be in
>> kcs_bmc.h instead?
>
>These are only temporary and get removed later on in the series after
>some shuffling of the code.
>
Okay -- there were a couple others further down in the patch that I'm
pretty sure were strictly redundant and could perhaps be cleaned up
(kcs_bmc_ipmi_event and kcs_bmc_ipmi_alloc in kcs_bmc_cdev_ipmi.c), but
aside from that:
Reviewed-by: Zev Weiss <zweiss@equinix.com>
next prev parent reply other threads:[~2021-04-09 19:22 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 6:27 [PATCH v2 01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 02/21] ARM: dts: Remove LPC BMC and Host partitions Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 03/21] ipmi: kcs: aspeed: Adapt to new LPC DTS layout Andrew Jeffery
2021-04-09 3:35 ` Joel Stanley
2021-03-19 6:27 ` [PATCH v2 04/21] pinctrl: aspeed-g5: Adapt to new LPC device tree layout Andrew Jeffery
2021-04-09 3:36 ` Joel Stanley
2021-03-19 6:27 ` [PATCH v2 05/21] soc: aspeed: " Andrew Jeffery
2021-04-09 3:38 ` Joel Stanley
2021-03-19 6:27 ` [PATCH v2 06/21] ipmi: kcs_bmc_aspeed: Use of match data to extract KCS properties Andrew Jeffery
2021-04-06 6:07 ` ChiaWei Wang
2021-04-09 3:24 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 07/21] ipmi: kcs_bmc: Make status update atomic Andrew Jeffery
2021-04-09 5:32 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 08/21] ipmi: kcs_bmc: Rename {read, write}_{status, data}() functions Andrew Jeffery
2021-04-09 5:33 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 09/21] ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi Andrew Jeffery
2021-04-09 3:56 ` Zev Weiss
2021-04-09 5:48 ` Andrew Jeffery
2021-04-09 19:21 ` Zev Weiss [this message]
2021-03-19 6:27 ` [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out Andrew Jeffery
2021-04-09 3:57 ` Zev Weiss
2021-04-09 5:59 ` Andrew Jeffery
2021-04-09 6:25 ` Zev Weiss
2021-04-09 19:26 ` Zev Weiss
2021-04-11 23:00 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 11/21] ipmi: kcs_bmc: Split headers into device and client Andrew Jeffery
2021-04-09 4:01 ` Zev Weiss
2021-04-09 6:06 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 12/21] ipmi: kcs_bmc: Strip private client data from struct kcs_bmc Andrew Jeffery
2021-04-09 4:07 ` Zev Weiss
2021-04-09 6:15 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core Andrew Jeffery
2021-04-06 6:07 ` ChiaWei Wang
2021-04-09 4:35 ` Zev Weiss
2021-04-09 6:24 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 14/21] ipmi: kcs_bmc: Allow clients to control KCS IRQ state Andrew Jeffery
2021-04-09 4:37 ` Zev Weiss
2021-04-09 6:39 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel Andrew Jeffery
2021-04-09 5:07 ` Zev Weiss
2021-04-09 6:42 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 16/21] ipmi: kcs_bmc: Add a "raw" character device interface Andrew Jeffery
2021-04-09 5:17 ` Zev Weiss
2021-04-09 6:46 ` Andrew Jeffery
2021-04-09 7:55 ` Arnd Bergmann
2021-04-12 1:33 ` Andrew Jeffery
2021-04-12 8:48 ` Arnd Bergmann
2021-04-12 23:45 ` Andrew Jeffery
2021-04-13 8:22 ` Arnd Bergmann
2021-04-14 0:30 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 17/21] dt-bindings: ipmi: Convert ASPEED KCS binding to schema Andrew Jeffery
2021-03-26 1:48 ` Rob Herring
2021-04-09 5:15 ` Zev Weiss
2021-04-09 5:33 ` Andrew Jeffery
2021-04-09 5:44 ` Zev Weiss
2021-04-09 8:46 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 18/21] dt-bindings: ipmi: Add optional SerIRQ property to ASPEED KCS devices Andrew Jeffery
2021-03-26 1:49 ` Rob Herring
2021-03-19 6:27 ` [PATCH v2 19/21] ipmi: kcs_bmc_aspeed: Implement KCS SerIRQ configuration Andrew Jeffery
2021-04-01 9:30 ` [EXTERNAL] " Zev Weiss
2021-03-19 6:27 ` [PATCH v2 20/21] ipmi: kcs_bmc_aspeed: Fix IBFIE typo from datasheet Andrew Jeffery
2021-04-09 5:40 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 21/21] ipmi: kcs_bmc_aspeed: Optionally apply status address Andrew Jeffery
2021-04-01 18:18 ` Re " Zev Weiss
2021-04-06 6:09 ` ChiaWei Wang
2021-04-09 3:18 ` [PATCH v2 01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning Joel Stanley
2021-04-09 5:24 ` Andrew Jeffery
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YHCpPa3EtVy9XcLn@packtop \
--to=zweiss@equinix.com \
--cc=andrew@aj.id.au \
--cc=avifishman70@gmail.com \
--cc=benjaminfair@google.com \
--cc=chiawei_wang@aspeedtech.com \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=minyard@acm.org \
--cc=openbmc@lists.ozlabs.org \
--cc=openipmi-developer@lists.sourceforge.net \
--cc=robh+dt@kernel.org \
--cc=ryan_chen@aspeedtech.com \
--cc=tali.perry1@gmail.com \
--cc=tmaimon77@gmail.com \
--cc=venture@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).