Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp3677293pxj; Mon, 7 Jun 2021 17:38:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzuWqOm+X11tEKCZY1uj8RSuQlzu4xt1hKL5AmbIXGIpOyGWNWg2n4yeuqP1WFzUIG6uGJp X-Received: by 2002:a17:907:7b97:: with SMTP id ne23mr20329755ejc.499.1623112734730; Mon, 07 Jun 2021 17:38:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1623112734; cv=none; d=google.com; s=arc-20160816; b=KZ5M5vANYmUL0fGI4aliorv4Bhu8B6umkJiMORyv9Dmz7AtMycXb/jfWhlNgxNoIcr z6WgOh1bmFDRZMpHjXeaZRiuncu/g/YfPr3HN0Lj3aTgXXU3ElFOoJUDtrL+bGAWdTNU iAE4uRZBY3L6IYU5A7re2S/iEEfvxkLu9+vhtTctM3iQOYq+Aow2AcQMnCJ4TYE0EyeR IpzaWYWNm0V0Z9+F6FiWdNXcHyXzic3P5rzXvfqnLqzB+oowotb7OvCtJs56fuGu3+4J MS9tFx0HlVFDtZn9ZUFPEnHAyatST1EEunz+bmm52C3Z5KHyo5i+0l9uePJNtaZBCkrL JlQw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:subject:cc:to:from:date:references:in-reply-to :message-id:mime-version:user-agent:dkim-signature:dkim-signature; bh=yBCq6nwjdfe7dMAcZu21nIyfwfagBbYE5SfivjE0NBU=; b=PE/FiWLD6fSq32h0Mi0u5zQnBcZeS+coxxoO9Dll8n9LhfUoqsvLY+XSugPlqhN+3E 6WIJ5kLUfK1PcrmJkmczuJAD14WrKIZXRlumZZChMvUboLZwj24ABUHirZs+5IL019tS rjX+/6POsOAa4t6Krf6TUKMcM8gCavH15GHmsGbOFKZtrNEY5gnFAjwXx8tfdXW4Jlnm TimDjRSpmZ3N4mKl02DwQjUhCjzhnLw0E3Hmsp7s7IXwX/vRnuVu/k1c8Q7KlRgO6AKx XAcSuU72GbgTRIX0h+QtVqREA9lyZuf6N6uzoIa3AhE3cJ/bawT48kirmuF6cR+5sQTr /7lQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@aj.id.au header.s=fm3 header.b=QSG4Mf75; dkim=pass header.i=@messagingengine.com header.s=fm3 header.b=GKrgIZP3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id os28si6403127ejb.695.2021.06.07.17.38.31; Mon, 07 Jun 2021 17:38:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@aj.id.au header.s=fm3 header.b=QSG4Mf75; dkim=pass header.i=@messagingengine.com header.s=fm3 header.b=GKrgIZP3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230508AbhFHAjT (ORCPT + 99 others); Mon, 7 Jun 2021 20:39:19 -0400 Received: from new2-smtp.messagingengine.com ([66.111.4.224]:40637 "EHLO new2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230209AbhFHAjT (ORCPT ); Mon, 7 Jun 2021 20:39:19 -0400 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.nyi.internal (Postfix) with ESMTP id 7906B5804E0; Mon, 7 Jun 2021 20:37:26 -0400 (EDT) Received: from imap43 ([10.202.2.93]) by compute2.internal (MEProxy); Mon, 07 Jun 2021 20:37:26 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm3; bh=yBCq6nwjdfe7dMAcZu21nIyfwfagBbY E5SfivjE0NBU=; b=QSG4Mf75bxuMOTWUpZLd9hfuWWOhffwrvIJdl565GGjCbHp FsU/7cSnExk40ZcBxkU4a9XkPl0iKlEToNVoHVXqwocPPeb4J2z/V4mePE6K915l pMDyg1rdAf6f2Gp/cne8qGMnf11FKyvOngVN9xB/8MZsA/X+ONPJT2G7Om+EnClz jDLcHbY9z/eYW47SNeudU3KIPsrHWjHkmxJuAFl0vrwJHmVcgQbV2dMr4f08IpRz Mf9n/fbCsLGUC7K5Z9FaGDcAEpMOei6t3Mo/a4SrMefdV1QZnQ2IMGvhNp0MdA0H 9wdtpsOJ6VhgnsBYYluf/3ajcunepAc68MfqOJw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=yBCq6n wjdfe7dMAcZu21nIyfwfagBbYE5SfivjE0NBU=; b=GKrgIZP3/n9Xx79RGsKT1O VGQqsMJlMsaYa05QKQjaSamCw1Qcsq5VsqpAuqzoc9bbrVO3IyuUj3bQR8vxzriI 1CtRfjisuZ9kQN1eqcbwPWdm7sWd78RC9eSa6p86ZhHNZiOxzgUw+YAh7zBySiWp d2egvck1aaDulnz8IDLxXbSWae+Kp9KVtDSMH0pMnFzMUOafeSQ3Z9JqSWDUd3CI nUMc6IS1McTHctyxaOquHrq5yrlChOR1mibAZFLZeMNjaekwY2V/TrQ5Tkeb6Na4 bxS6geXmDrFeE7/m5+DW4tBKjazd6FeBXFHHGw7b6/V9dDv3w1SXGOvmNmowayZQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfedtkedgvdejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesthdtredtreertdenucfhrhhomhepfdetnhgu rhgvficulfgvfhhfvghrhidfuceorghnughrvgifsegrjhdrihgurdgruheqnecuggftrf grthhtvghrnhephefhfeekgfekudevheffheeihedujeefjeevjeefudfgfeeutdeuvdeh hfevueffnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomh eprghnughrvgifsegrjhdrihgurdgruh X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id EE4F0AC0062; Mon, 7 Jun 2021 20:37:24 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-519-g27a961944e-fm-20210531.001-g27a96194 Mime-Version: 1.0 Message-Id: <15bfe16d-a9a1-464b-bb23-c59ac91b41a8@www.fastmail.com> In-Reply-To: References: <20210510054213.1610760-1-andrew@aj.id.au> <20210510054213.1610760-12-andrew@aj.id.au> Date: Tue, 08 Jun 2021 10:07:03 +0930 From: "Andrew Jeffery" To: "Zev Weiss" Cc: "openipmi-developer@lists.sourceforge.net" , "openbmc@lists.ozlabs.org" , "Corey Minyard" , "devicetree@vger.kernel.org" , "Tomer Maimon" , "linux-aspeed@lists.ozlabs.org" , "Avi Fishman" , "Patrick Venture" , "linux-kernel@vger.kernel.org" , "Tali Perry" , "Rob Herring" , "Chia-Wei, Wang" , "linux-arm-kernel@lists.infradead.org" , "Benjamin Fair" , "Arnd Bergmann" Subject: Re: [PATCH v3 11/16] ipmi: kcs_bmc: Add serio adaptor Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >--- > > 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 > >+#include > >+#include > >+#include > >+#include > >+#include > >+#include > >+#include > >+ > >+#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