Re: [PATCH v3 11/16] ipmi: kcs_bmc: Add serio adaptor

[Andrew Jeffery]

On Fri, 21 May 2021, at 16:50, Zev Weiss wrote:
> On Mon, May 10, 2021 at 12:42:08AM CDT, Andrew Jeffery wrote:
> >kcs_bmc_serio acts as a bridge between the KCS drivers in the IPMI
> >subsystem and the existing userspace interfaces available through the
> >serio subsystem. This is useful when userspace would like to make use of
> >the BMC KCS devices for purposes that aren't IPMI.
> >
> >Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> >---
> > drivers/char/ipmi/Kconfig         |  14 +++
> > drivers/char/ipmi/Makefile        |   1 +
> > drivers/char/ipmi/kcs_bmc_serio.c | 151 ++++++++++++++++++++++++++++++
> > 3 files changed, 166 insertions(+)
> > create mode 100644 drivers/char/ipmi/kcs_bmc_serio.c
> >
> >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> >index bc5f81899b62..249b31197eea 100644
> >--- a/drivers/char/ipmi/Kconfig
> >+++ b/drivers/char/ipmi/Kconfig
> >@@ -137,6 +137,20 @@ config IPMI_KCS_BMC_CDEV_IPMI
> > 	  This support is also available as a module. The module will be
> > 	  called kcs_bmc_cdev_ipmi.
> >
> >+config IPMI_KCS_BMC_SERIO
> >+	depends on IPMI_KCS_BMC && SERIO
> >+	tristate "SerIO adaptor for BMC KCS devices"
> >+	help
> >+	  Adapts the BMC KCS device for the SerIO subsystem. This allows users
> >+	  to take advantage of userspace interfaces provided by SerIO where
> >+	  appropriate.
> >+
> >+	  Say YES if you wish to expose KCS devices on the BMC via SerIO
> >+	  interfaces.
> >+
> >+	  This support is also available as a module. The module will be
> >+	  called kcs_bmc_serio.
> >+
> > config ASPEED_BT_IPMI_BMC
> > 	depends on ARCH_ASPEED || COMPILE_TEST
> > 	depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
> >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> >index fcfa676afddb..84f47d18007f 100644
> >--- a/drivers/char/ipmi/Makefile
> >+++ b/drivers/char/ipmi/Makefile
> >@@ -23,6 +23,7 @@ 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_SERIO) += kcs_bmc_serio.o
> > obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o
> > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
> > obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
> >diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> >new file mode 100644
> >index 000000000000..30a2b7ab464b
> >--- /dev/null
> >+++ b/drivers/char/ipmi/kcs_bmc_serio.c
> >@@ -0,0 +1,151 @@
> >+// SPDX-License-Identifier: GPL-2.0-or-later
> >+/* Copyright (c) 2021 IBM Corp. */
> >+
> >+#include <linux/delay.h>
> >+#include <linux/device.h>
> >+#include <linux/errno.h>
> >+#include <linux/list.h>
> >+#include <linux/module.h>
> >+#include <linux/sched/signal.h>
> >+#include <linux/serio.h>
> >+#include <linux/slab.h>
> >+
> >+#include "kcs_bmc_client.h"
> >+
> >+struct kcs_bmc_serio {
> >+	struct list_head entry;
> >+
> >+	struct kcs_bmc_client client;
> >+	struct serio *port;
> >+
> >+	spinlock_t lock;
> >+};
> >+
> >+static inline struct kcs_bmc_serio *client_to_kcs_bmc_serio(struct kcs_bmc_client *client)
> >+{
> >+	return container_of(client, struct kcs_bmc_serio, client);
> >+}
> >+
> >+static irqreturn_t kcs_bmc_serio_event(struct kcs_bmc_client *client)
> >+{
> >+	struct kcs_bmc_serio *priv;
> >+	u8 handled = IRQ_NONE;
> >+	u8 status;
> >+
> >+	priv = client_to_kcs_bmc_serio(client);
> >+
> >+	spin_lock(&priv->lock);
> >+
> >+	status = kcs_bmc_read_status(client->dev);
> >+
> >+	if (status & KCS_BMC_STR_IBF)
> >+		handled = serio_interrupt(priv->port, kcs_bmc_read_data(client->dev), 0);
> >+
> >+	spin_unlock(&priv->lock);
> >+
> >+	return handled;
> >+}
> >+
> >+static const struct kcs_bmc_client_ops kcs_bmc_serio_client_ops = {
> >+	.event = kcs_bmc_serio_event,
> >+};
> >+
> >+static int kcs_bmc_serio_open(struct serio *port)
> >+{
> >+	struct kcs_bmc_serio *priv = port->port_data;
> >+
> >+	return kcs_bmc_enable_device(priv->client.dev, &priv->client);
> >+}
> >+
> >+static void kcs_bmc_serio_close(struct serio *port)
> >+{
> >+	struct kcs_bmc_serio *priv = port->port_data;
> >+
> >+	kcs_bmc_disable_device(priv->client.dev, &priv->client);
> >+}
> >+
> >+static DEFINE_SPINLOCK(kcs_bmc_serio_instances_lock);
> >+static LIST_HEAD(kcs_bmc_serio_instances);
> >+
> >+static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
> >+{
> >+	struct kcs_bmc_serio *priv;
> >+	struct serio *port;
> >+
> >+	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> >+	port = kzalloc(sizeof(*port), GFP_KERNEL);
> 
> Is there a particular reason to allocate port with a raw kzalloc()
> instead of another devm_kzalloc()?

Yes, because it causes pointer confusion on remove. We get the following backtrace:

[   95.018845] Backtrace: 
[   95.019162] [<802e1fb0>] (___cache_free) from [<802e31cc>] (kfree+0xc0/0x1e8)
[   95.019658]  r10:00000081 r9:8667c000 r8:80100284 r7:81000b40 r6:a0000013 r5:80640bd4
[   95.020032]  r4:82a5ec40
[   95.020206] [<802e310c>] (kfree) from [<80640bd4>] (serio_release_port+0x1c/0x28)
[   95.020571]  r7:00000000 r6:819c1540 r5:86acf480 r4:82a5ed70
[   95.020818] [<80640bb8>] (serio_release_port) from [<80565e00>] (device_release+0x40/0xb4)
[   95.021387] [<80565dc0>] (device_release) from [<804a0bcc>] (kobject_put+0xc8/0x210)
[   95.021961]  r5:80c77c30 r4:82a5ed70
[   95.022141] [<804a0b04>] (kobject_put) from [<80566078>] (put_device+0x20/0x24)
[   95.022537]  r7:80c820cc r6:82a5ec40 r5:80c820e4 r4:82a5ed70
[   95.023017] [<80566058>] (put_device) from [<80640a58>] (serio_destroy_port+0x12c/0x140)
[   95.023719] [<8064092c>] (serio_destroy_port) from [<80640b3c>] (serio_unregister_port+0x34/0x44)
[   95.024326]  r7:7f0012b4 r6:7f002024 r5:80c820e4 r4:82a5ec40
[   95.024792] [<80640b08>] (serio_unregister_port) from [<7f0100b8>] (kcs_bmc_serio_remove_device+0x90/0xbc [kcs_bmc_serio])
[   95.026951]  r5:8668b7c0 r4:86acf0c0
[   95.027390] [<7f010028>] (kcs_bmc_serio_remove_device [kcs_bmc_serio]) from [<7f00048c>] (kcs_bmc_unregister_driver+0x60/0xbd4 [kcs_bmc])
[   95.028443]  r5:7f012018 r4:8668b7c0
[   95.028634] [<7f00042c>] (kcs_bmc_unregister_driver [kcs_bmc]) from [<7f0102c4>] (kcs_bmc_serio_exit+0x1c/0xd58 [kcs_bmc_serio])
[   95.029165]  r7:00000081 r6:80cd0dac r5:00000000 r4:7f012040
[   95.029397] [<7f0102a8>] (kcs_bmc_serio_exit [kcs_bmc_serio]) from [<801cbab0>] (sys_delete_module+0x140/0x280)
[   95.029875] [<801cb970>] (sys_delete_module) from [<80100080>] (ret_fast_syscall+0x0/0x58)

> 
> >+	if (!(priv && port))
> >+		return -ENOMEM;
> >+
> >+	port->id.type = SERIO_8042;
> >+	port->open = kcs_bmc_serio_open;
> >+	port->close = kcs_bmc_serio_close;
> >+	port->port_data = priv;
> >+	port->dev.parent = kcs_bmc->dev;
> >+
> >+	spin_lock_init(&priv->lock);
> >+	priv->port = port;
> >+	priv->client.dev = kcs_bmc;
> >+	priv->client.ops = &kcs_bmc_serio_client_ops;
> >+
> >+	spin_lock_irq(&kcs_bmc_serio_instances_lock);
> >+	list_add(&priv->entry, &kcs_bmc_serio_instances);
> >+	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
> >+
> >+	serio_register_port(port);
> >+
> >+	dev_info(kcs_bmc->dev, "Initialised serio client for channel %d", kcs_bmc->channel);
> >+
> >+	return 0;
> >+}
> >+
> >+static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
> >+{
> >+	struct kcs_bmc_serio *priv = NULL, *pos;
> >+
> >+	spin_lock_irq(&kcs_bmc_serio_instances_lock);
> >+	list_for_each_entry(pos, &kcs_bmc_serio_instances, entry) {
> >+		if (pos->client.dev == kcs_bmc) {
> >+			priv = pos;
> >+			list_del(&pos->entry);
> >+			break;
> >+		}
> >+	}
> >+	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
> >+
> >+	if (!priv)
> >+		return -ENODEV;
> >+
> >+	serio_unregister_port(priv->port);
> >+	kcs_bmc_disable_device(kcs_bmc, &priv->client);
> >+	devm_kfree(priv->client.dev->dev, priv);
> 
> Looks like the priv->port allocation would leak here I think?

No, because port's released through serio_unregister_port(). It's not super obvious though, so I'll add a comment next to the kzalloc().

> 
> Also, is the asymmetry of having kcs_bmc_disable_device() here but no
> corresponding kcs_bmc_enable_device() in kcs_bmc_serio_add_device()
> intentional?  If so, an explanatory comment of some sort might be nice
> to add.

It's intentional to make sure the device isn't left registered as enabled in the core. kcs_bmc_enable_device() is called in the open() path.

Andrew