From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8C167C47095 for ; Tue, 8 Jun 2021 00:39:17 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 52CCF61108 for ; Tue, 8 Jun 2021 00:39:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 52CCF61108 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=aj.id.au Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Subject:Cc:To:From:Date:References: In-Reply-To:Message-Id:Mime-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=HjNLm60MPtwwHthGYD+QCr3/I07Htk7r0CZ15LuiOa4=; b=bG253kCRlEyqs4 2od0BZ2rdeAKA8dHKkRlsa9Mj0eMeeqR02sR0aroStGyzV/smDK9RjmqwvkwXTbqclzyVObdc31+p aOAHVPyq7wkJxQ+Lcg8IbKABMv+VVQ8X5M4AT2nFz6yfESyNNhOxb0X+pq4dGrwSdwIvxTwIhOqdW /eNboyF37EUn6/bkiaPEfG9pOAcAnsKXdO+bDnW/G1W+1UUDSMVSrUtDvUEd0eY8lm0iRnvpZbFf8 iXXGiQXQ7tnU0ZA5Nf9c0d7bIqPYN604ScldGN0xp/oxuFhrbiKJ/UfUXLUzMji4VmvC8TMv+sKtX FPp46QEhiRiFqefuuw+A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lqPkO-005ppn-6T; Tue, 08 Jun 2021 00:37:36 +0000 Received: from new2-smtp.messagingengine.com ([66.111.4.224]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lqPkH-005poT-QC for linux-arm-kernel@lists.infradead.org; Tue, 08 Jun 2021 00:37:34 +0000 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210607_173729_948630_DDE900A0 X-CRM114-Status: GOOD ( 25.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel