| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 09 Apr 2021 15:36:48 +0930
From: "Andrew Jeffery" <andrew@...id.au>
To: "Zev Weiss" <zweiss@...inix.com>
Cc: "openipmi-developer@...ts.sourceforge.net"
<openipmi-developer@...ts.sourceforge.net>,
"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
"Corey Minyard" <minyard@....org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"Ryan Chen" <ryan_chen@...eedtech.com>,
"Tomer Maimon" <tmaimon77@...il.com>,
"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
"Avi Fishman" <avifishman70@...il.com>,
"Patrick Venture" <venture@...gle.com>,
"Linus Walleij" <linus.walleij@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Tali Perry" <tali.perry1@...il.com>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"Rob Herring" <robh+dt@...nel.org>,
"Lee Jones" <lee.jones@...aro.org>,
"Chia-Wei, Wang" <chiawei_wang@...eedtech.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"Benjamin Fair" <benjaminfair@...gle.com>
Subject: Re: [PATCH v2 11/21] ipmi: kcs_bmc: Split headers into device and client
On Fri, 9 Apr 2021, at 13:31, Zev Weiss wrote:
> 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@...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@...ux.intel.com>");
> >+MODULE_AUTHOR("Andrew Jeffery <andrew@...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?
In earlier iterations of the patches I was doing some extra work in the
event handling path and felt it was useful at the time. However I've
refactored things a little and this may have outlived its usefulness.
I'll reasses!
>
> >+
> >+#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?
I think I just added it as documentation as the hardware-defined bits
aren't contiguous.
Andrew
Powered by blists - more mailing lists