lkml.org 
[lkml]   [2021]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 11/21] ipmi: kcs_bmc: Split headers into device and client
    Date
    On Fri, Mar 19, 2021 at 01:27:42AM CDT, Andrew Jeffery wrote:
    >Strengthen the distinction between code that abstracts the
    >implementation of the KCS behaviours (device drivers) and code that
    >exploits KCS behaviours (clients). Neither needs to know about the APIs
    >required by the other, so provide separate headers.
    >
    >Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
    >---
    > drivers/char/ipmi/kcs_bmc.c | 21 ++++++++++-----
    > drivers/char/ipmi/kcs_bmc.h | 30 ++++++++++-----------
    > drivers/char/ipmi/kcs_bmc_aspeed.c | 20 +++++++++-----
    > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 39 ++++++++++++++++++---------
    > drivers/char/ipmi/kcs_bmc_client.h | 29 ++++++++++++++++++++
    > drivers/char/ipmi/kcs_bmc_device.h | 19 +++++++++++++
    > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 20 +++++++++-----
    > 7 files changed, 129 insertions(+), 49 deletions(-)
    > create mode 100644 drivers/char/ipmi/kcs_bmc_client.h
    > create mode 100644 drivers/char/ipmi/kcs_bmc_device.h
    >
    >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
    >index 709b6bdec165..1046ce2bbefc 100644
    >--- a/drivers/char/ipmi/kcs_bmc.c
    >+++ b/drivers/char/ipmi/kcs_bmc.c
    >@@ -1,46 +1,52 @@
    > // SPDX-License-Identifier: GPL-2.0
    > /*
    > * Copyright (c) 2015-2018, Intel Corporation.
    >+ * Copyright (c) 2021, IBM Corp.
    > */
    >
    > #include <linux/module.h>
    >
    > #include "kcs_bmc.h"
    >
    >+/* Implement both the device and client interfaces here */
    >+#include "kcs_bmc_device.h"
    >+#include "kcs_bmc_client.h"
    >+
    >+/* Consumer data access */
    >+
    > u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc)
    > {
    >- return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
    >+ return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
    > }
    > EXPORT_SYMBOL(kcs_bmc_read_data);
    >
    > void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data)
    > {
    >- kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
    >+ kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
    > }
    > EXPORT_SYMBOL(kcs_bmc_write_data);
    >
    > u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc)
    > {
    >- return kcs_bmc->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
    >+ return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
    > }
    > EXPORT_SYMBOL(kcs_bmc_read_status);
    >
    > void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data)
    > {
    >- kcs_bmc->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
    >+ kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
    > }
    > EXPORT_SYMBOL(kcs_bmc_write_status);
    >
    > 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);
    >+ kcs_bmc->ops->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val);
    > }
    > EXPORT_SYMBOL(kcs_bmc_update_status);
    >
    >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc);
    > int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
    > {
    >- return kcs_bmc_ipmi_event(kcs_bmc);
    >+ return kcs_bmc->client.ops->event(&kcs_bmc->client);
    > }
    > EXPORT_SYMBOL(kcs_bmc_handle_event);
    >
    >@@ -60,4 +66,5 @@ EXPORT_SYMBOL(kcs_bmc_remove_device);
    >
    > MODULE_LICENSE("GPL v2");
    > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
    >+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
    > MODULE_DESCRIPTION("KCS BMC to handle the IPMI request from system software");
    >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
    >index bf0ae327997f..a1350e567723 100644
    >--- a/drivers/char/ipmi/kcs_bmc.h
    >+++ b/drivers/char/ipmi/kcs_bmc.h
    >@@ -8,6 +8,15 @@
    >
    > #include <linux/miscdevice.h>
    >
    >+#include "kcs_bmc_client.h"
    >+
    >+#define KCS_BMC_EVENT_NONE 0
    >+#define KCS_BMC_EVENT_HANDLED 1

    Is there a particular reason we're introducing these macros and using an
    int return type for kcs_bmc_client_ops.event instead of just having it
    be irqreturn_t? Other event types or outcomes we're anticipating needing
    to handle maybe?

    >+
    >+#define KCS_BMC_STR_OBF BIT(0)
    >+#define KCS_BMC_STR_IBF BIT(1)
    >+#define KCS_BMC_STR_CMD_DAT BIT(3)

    The first two of these macros are used later in the series, but the third
    doesn't end up used at all I think?

    >+
    > /* Different phases of the KCS BMC module.
    > * KCS_PHASE_IDLE:
    > * BMC should not be expecting nor sending any data.
    >@@ -66,19 +75,21 @@ struct kcs_ioreg {
    > u32 str;
    > };
    >
    >+struct kcs_bmc_device_ops;
    >+
    > struct kcs_bmc {
    > struct device *dev;
    >
    >+ const struct kcs_bmc_device_ops *ops;
    >+
    >+ struct kcs_bmc_client client;
    >+
    > spinlock_t lock;
    >
    > u32 channel;
    > int running;
    >
    >- /* Setup by BMC KCS controller driver */
    > struct kcs_ioreg ioreg;
    >- u8 (*io_inputb)(struct kcs_bmc *kcs_bmc, u32 reg);
    >- void (*io_outputb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 b);
    >- void (*io_updateb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 val);
    >
    > enum kcs_phases phase;
    > enum kcs_errors error;
    >@@ -97,15 +108,4 @@ struct kcs_bmc {
    >
    > struct miscdevice miscdev;
    > };
    >-
    >-int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
    >-int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
    >-int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
    >-
    >-u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
    >-void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
    >-u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc);
    >-void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data);
    >-void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val);
    >-
    > #endif /* __KCS_BMC_H__ */
    >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
    >index 0416ac78ce68..1b313355b1c8 100644
    >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
    >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
    >@@ -21,7 +21,7 @@
    > #include <linux/slab.h>
    > #include <linux/timer.h>
    >
    >-#include "kcs_bmc.h"
    >+#include "kcs_bmc_device.h"
    >
    >
    > #define DEVICE_NAME "ast-kcs-bmc"
    >@@ -220,14 +220,22 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
    > }
    > }
    >
    >+static const struct kcs_bmc_device_ops aspeed_kcs_ops = {
    >+ .io_inputb = aspeed_kcs_inb,
    >+ .io_outputb = aspeed_kcs_outb,
    >+ .io_updateb = aspeed_kcs_updateb,
    >+};
    >+
    > static irqreturn_t aspeed_kcs_irq(int irq, void *arg)
    > {
    > struct kcs_bmc *kcs_bmc = arg;
    >+ int rc;
    >
    >- if (!kcs_bmc_handle_event(kcs_bmc))
    >- return IRQ_HANDLED;
    >+ rc = kcs_bmc_handle_event(kcs_bmc);
    >+ if (rc < 0)
    >+ dev_warn(kcs_bmc->dev, "Failed to service irq: %d\n", rc);
    >
    >- return IRQ_NONE;
    >+ return rc == KCS_BMC_EVENT_HANDLED ? IRQ_HANDLED : IRQ_NONE;
    > }
    >
    > static int aspeed_kcs_config_irq(struct kcs_bmc *kcs_bmc,
    >@@ -362,9 +370,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
    > kcs_bmc->dev = &pdev->dev;
    > kcs_bmc->channel = channel;
    > kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1];
    >- kcs_bmc->io_inputb = aspeed_kcs_inb;
    >- kcs_bmc->io_outputb = aspeed_kcs_outb;
    >- kcs_bmc->io_updateb = aspeed_kcs_updateb;
    >+ kcs_bmc->ops = &aspeed_kcs_ops;
    >
    > priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
    > if (IS_ERR(priv->map)) {
    >diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
    >index 0ca71c135a1a..fd852d8abe48 100644
    >--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
    >+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
    >@@ -22,7 +22,6 @@
    >
    > #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)
    >@@ -179,12 +178,19 @@ static void kcs_bmc_ipmi_handle_cmd(struct kcs_bmc *kcs_bmc)
    > }
    > }
    >
    >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc);
    >-int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc)
    >+static inline struct kcs_bmc *client_to_kcs_bmc(struct kcs_bmc_client *client)
    > {
    >+ return container_of(client, struct kcs_bmc, client);
    >+}
    >+
    >+static int kcs_bmc_ipmi_event(struct kcs_bmc_client *client)
    >+{
    >+ struct kcs_bmc *kcs_bmc;
    > unsigned long flags;
    >- int ret = -ENODATA;
    > u8 status;
    >+ int ret;
    >+
    >+ kcs_bmc = client_to_kcs_bmc(client);
    >
    > spin_lock_irqsave(&kcs_bmc->lock, flags);
    >
    >@@ -197,23 +203,28 @@ int kcs_bmc_ipmi_event(struct kcs_bmc *kcs_bmc)
    > else
    > kcs_bmc_ipmi_handle_data(kcs_bmc);
    >
    >- ret = 0;
    >+ ret = KCS_BMC_EVENT_HANDLED;
    >+ } else {
    >+ ret = KCS_BMC_EVENT_NONE;
    > }
    >
    > spin_unlock_irqrestore(&kcs_bmc->lock, flags);
    >
    > return ret;
    > }
    >-EXPORT_SYMBOL(kcs_bmc_ipmi_event);
    >
    >-static inline struct kcs_bmc *to_kcs_bmc(struct file *filp)
    >+static const struct kcs_bmc_client_ops kcs_bmc_ipmi_client_ops = {
    >+ .event = kcs_bmc_ipmi_event,
    >+};
    >+
    >+static inline struct kcs_bmc *file_to_kcs_bmc(struct file *filp)
    > {
    > return container_of(filp->private_data, struct kcs_bmc, miscdev);
    > }
    >
    > static int kcs_bmc_ipmi_open(struct inode *inode, struct file *filp)
    > {
    >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
    >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
    > int ret = 0;
    >
    > spin_lock_irq(&kcs_bmc->lock);
    >@@ -228,7 +239,7 @@ static int kcs_bmc_ipmi_open(struct inode *inode, struct file *filp)
    >
    > static __poll_t kcs_bmc_ipmi_poll(struct file *filp, poll_table *wait)
    > {
    >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
    >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
    > __poll_t mask = 0;
    >
    > poll_wait(filp, &kcs_bmc->queue, wait);
    >@@ -244,7 +255,7 @@ static __poll_t kcs_bmc_ipmi_poll(struct file *filp, poll_table *wait)
    > static ssize_t kcs_bmc_ipmi_read(struct file *filp, char __user *buf,
    > size_t count, loff_t *ppos)
    > {
    >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
    >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
    > bool data_avail;
    > size_t data_len;
    > ssize_t ret;
    >@@ -306,7 +317,7 @@ static ssize_t kcs_bmc_ipmi_read(struct file *filp, char __user *buf,
    > static ssize_t kcs_bmc_ipmi_write(struct file *filp, const char __user *buf,
    > size_t count, loff_t *ppos)
    > {
    >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
    >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
    > ssize_t ret;
    >
    > /* a minimum response size '3' : netfn + cmd + ccode */
    >@@ -342,7 +353,7 @@ static ssize_t kcs_bmc_ipmi_write(struct file *filp, const char __user *buf,
    > static long kcs_bmc_ipmi_ioctl(struct file *filp, unsigned int cmd,
    > unsigned long arg)
    > {
    >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
    >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
    > long ret = 0;
    >
    > spin_lock_irq(&kcs_bmc->lock);
    >@@ -372,7 +383,7 @@ static long kcs_bmc_ipmi_ioctl(struct file *filp, unsigned int cmd,
    >
    > static int kcs_bmc_ipmi_release(struct inode *inode, struct file *filp)
    > {
    >- struct kcs_bmc *kcs_bmc = to_kcs_bmc(filp);
    >+ struct kcs_bmc *kcs_bmc = file_to_kcs_bmc(filp);
    >
    > spin_lock_irq(&kcs_bmc->lock);
    > kcs_bmc->running = 0;
    >@@ -401,6 +412,8 @@ int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc *kcs_bmc)
    > mutex_init(&kcs_bmc->mutex);
    > init_waitqueue_head(&kcs_bmc->queue);
    >
    >+ kcs_bmc->client.dev = kcs_bmc;
    >+ kcs_bmc->client.ops = &kcs_bmc_ipmi_client_ops;
    > kcs_bmc->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
    > kcs_bmc->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
    > kcs_bmc->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
    >diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
    >new file mode 100644
    >index 000000000000..140631d157d8
    >--- /dev/null
    >+++ b/drivers/char/ipmi/kcs_bmc_client.h
    >@@ -0,0 +1,29 @@
    >+/* SPDX-License-Identifier: GPL-2.0 */
    >+/* Copyright (c) 2021, IBM Corp. */
    >+
    >+#ifndef __KCS_BMC_CONSUMER_H__
    >+#define __KCS_BMC_CONSUMER_H__
    >+
    >+#include <linux/list.h>
    >+#include <linux/notifier.h>
    >+#include <stdbool.h>
    >+
    >+struct kcs_bmc;
    >+struct kcs_bmc_client_ops;
    >+
    >+struct kcs_bmc_client {
    >+ const struct kcs_bmc_client_ops *ops;
    >+
    >+ struct kcs_bmc *dev;
    >+};
    >+
    >+struct kcs_bmc_client_ops {
    >+ int (*event)(struct kcs_bmc_client *client);
    >+};
    >+
    >+u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
    >+void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
    >+u8 kcs_bmc_read_status(struct kcs_bmc *kcs_bmc);
    >+void kcs_bmc_write_status(struct kcs_bmc *kcs_bmc, u8 data);
    >+void kcs_bmc_update_status(struct kcs_bmc *kcs_bmc, u8 mask, u8 val);
    >+#endif
    >diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h
    >new file mode 100644
    >index 000000000000..33462174516d
    >--- /dev/null
    >+++ b/drivers/char/ipmi/kcs_bmc_device.h
    >@@ -0,0 +1,19 @@
    >+/* SPDX-License-Identifier: GPL-2.0 */
    >+/* Copyright (c) 2021, IBM Corp. */
    >+
    >+#ifndef __KCS_BMC_DEVICE_H__
    >+#define __KCS_BMC_DEVICE_H__
    >+
    >+#include "kcs_bmc.h"
    >+
    >+struct kcs_bmc_device_ops {
    >+ u8 (*io_inputb)(struct kcs_bmc *kcs_bmc, u32 reg);
    >+ void (*io_outputb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 b);
    >+ void (*io_updateb)(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 b);
    >+};
    >+
    >+int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
    >+int kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
    >+int kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
    >+
    >+#endif
    >diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
    >index 5d017498dc69..1d21697fc585 100644
    >--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
    >+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
    >@@ -17,7 +17,7 @@
    > #include <linux/regmap.h>
    > #include <linux/slab.h>
    >
    >-#include "kcs_bmc.h"
    >+#include "kcs_bmc_device.h"
    >
    > #define DEVICE_NAME "npcm-kcs-bmc"
    > #define KCS_CHANNEL_MAX 3
    >@@ -127,11 +127,13 @@ static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
    > static irqreturn_t npcm7xx_kcs_irq(int irq, void *arg)
    > {
    > struct kcs_bmc *kcs_bmc = arg;
    >+ int rc;
    >
    >- if (!kcs_bmc_handle_event(kcs_bmc))
    >- return IRQ_HANDLED;
    >+ rc = kcs_bmc_handle_event(kcs_bmc);
    >+ if (rc < 0)
    >+ dev_warn(kcs_bmc->dev, "Failed to service irq: %d\n", rc);
    >
    >- return IRQ_NONE;
    >+ return rc == KCS_BMC_EVENT_HANDLED ? IRQ_HANDLED : IRQ_NONE;
    > }
    >
    > static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
    >@@ -148,6 +150,12 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc *kcs_bmc,
    > dev_name(dev), kcs_bmc);
    > }
    >
    >+static const struct kcs_bmc_device_ops npcm7xx_kcs_ops = {
    >+ .io_inputb = npcm7xx_kcs_inb,
    >+ .io_outputb = npcm7xx_kcs_outb,
    >+ .io_updateb = npcm7xx_kcs_updateb,
    >+};
    >+
    > static int npcm7xx_kcs_probe(struct platform_device *pdev)
    > {
    > struct device *dev = &pdev->dev;
    >@@ -179,9 +187,7 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
    > kcs_bmc->ioreg.idr = priv->reg->dib;
    > kcs_bmc->ioreg.odr = priv->reg->dob;
    > kcs_bmc->ioreg.str = priv->reg->sts;
    >- kcs_bmc->io_inputb = npcm7xx_kcs_inb;
    >- kcs_bmc->io_outputb = npcm7xx_kcs_outb;
    >- kcs_bmc->io_updateb = npcm7xx_kcs_updateb;
    >+ kcs_bmc->ops = &npcm7xx_kcs_ops;
    >
    > platform_set_drvdata(pdev, priv);
    >
    >--
    >2.27.0
    >
    \
     
     \ /
      Last update: 2021-04-09 06:05    [W:3.478 / U:1.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site