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.8 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 6A863C433B4 for ; Fri, 9 Apr 2021 06:50:00 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 B1E7E610C8 for ; Fri, 9 Apr 2021 06:49:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B1E7E610C8 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=desiato.20200630; 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=CWPyImYV9TFoFIZVL5h5OwvtMgTTiq9ASf+cStQ2uIQ=; b=nWP7suekdoyn5yTrDLHrNMrnn 6k/jRQfO9jy7Fq0XMjmqCXK4sESAF0/0nPdevWxunYnGxoCqirga2/y2r7XTFBW+osV8Oe5PP+eS3 /06XqMoh82F9et+zplEIVyIhmHKfhSlBNyrCm7STAzCLn7YE4CpLGgF+tks+qNkhWn7IjP1DW/pm1 PNHRvnHvz9Gqt9ShnGQmfNwu5efM22QSS3zaT+tmI8o4I2zfOpcyeOSL22sa173Y8YfKDaIZYeOXw nCus4JHg2QrFcOW98WxeSye63ggeziyb0D/kGyPXbzT4he4aidxBMjrAl232hBRMs5YGEJUqLlAxt T0bKVG4GQ==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lUkhG-00AJ7t-EJ; Fri, 09 Apr 2021 06:32:59 +0000 Received: from new4-smtp.messagingengine.com ([66.111.4.230]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lUkZe-00AIr2-3j for linux-arm-kernel@lists.infradead.org; Fri, 09 Apr 2021 06:25:28 +0000 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailnew.nyi.internal (Postfix) with ESMTP id 17DDA5807E5; Fri, 9 Apr 2021 02:24:55 -0400 (EDT) Received: from imap2 ([10.202.2.52]) by compute3.internal (MEProxy); Fri, 09 Apr 2021 02:24:55 -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=fm2; bh=ide85Y6I3nLlU21oZu0+39r8t0oRTPX dyMtRCjs0F6c=; b=r5/e97xxS+m0PjTFSn115oBSwosQ53R/yPSVvM97dosczzI jiDWqbectoRQ4Ux+yda6QSH4TnKWSXmPoUZuQARGrXIqBgjvz37jWtgvLzTCQTN9 ePgTMgcGe4B7T5C2TRRcpCWsVYEI9+v8bh3BgR3xtTv6Wy+4kycoR12m2zoP5rWv T8QIFaPZtC6L44YNUAFKte2NxdMep0jGoa/Q6rZV8Yu3kzlq8HTYg+0xBuyzekss NYLPM5IWm/Fg2e0k4Sy8o3DfTzbXYpWSVdkBxfNiYc+x3XgUD6jnTiAp8AUfs/jZ XowBCZ7aR4rYu7e4oD8LLs5XS1KW90HR8pIN1Ow== 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=fm2; bh=ide85Y 6I3nLlU21oZu0+39r8t0oRTPXdyMtRCjs0F6c=; b=iRPBBTcL6jJtM7O+0Z7lVn L2D2YiB3+RQ9NOyzgD5cnir4RxRdNNY7tafnxndKUrxRVTRfWc/Q7zUm5fiSwFEf kTyilUmrP5+GXkcZNqoFuUQpaTG/B7tR/JU7OIEwvwTQQSFF+xBrASQyl5/j8n7v y9WVHtjTgELCV+YDFzfPp0LEmSHCGm+NFW/kajwjJ3bWZHo2gytU8HUGuwzm8SBc Gz5ppWWY2hy+KUG/pffmaH4/guJcIVrr7sgfdps/rJDOlGCtaBjckHg4Zq/VgZnK kC36QMw1Gsrz+hGoJjyGI12iUOhBjn7CYaK17NusMFhnaPdbXbsYW34XMvpcc9RQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrudektddguddthecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefofgggkfgjfhffhffvufgtsehttdertderreejnecuhfhrohhmpedftehn ughrvgifucflvghffhgvrhihfdcuoegrnhgurhgvfiesrghjrdhiugdrrghuqeenucggtf frrghtthgvrhhnpedutddtkeeugeegvddttdeukeeiuddtgfeuuddtfeeiueetfeeileet tedvtdfhieenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhroh hmpegrnhgurhgvfiesrghjrdhiugdrrghu X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 14E9BA0007F; Fri, 9 Apr 2021 02:24:54 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-273-g8500d2492d-fm-20210323.002-g8500d249 Mime-Version: 1.0 Message-Id: <386cc03a-f6d0-45b6-a27b-fc204dedbcac@www.fastmail.com> In-Reply-To: References: <20210319062752.145730-1-andrew@aj.id.au> <20210319062752.145730-13-andrew@aj.id.au> Date: Fri, 09 Apr 2021 15:54:33 +0930 From: "Andrew Jeffery" To: "Zev Weiss" Cc: "openipmi-developer@lists.sourceforge.net" , "openbmc@lists.ozlabs.org" , "Corey Minyard" , "devicetree@vger.kernel.org" , "Ryan Chen" , "Tomer Maimon" , "linux-aspeed@lists.ozlabs.org" , "Avi Fishman" , "Patrick Venture" , "Linus Walleij" , "linux-kernel@vger.kernel.org" , "Tali Perry" , "linux-gpio@vger.kernel.org" , "Rob Herring" , "Lee Jones" , "Chia-Wei, Wang" , "linux-arm-kernel@lists.infradead.org" , "Benjamin Fair" Subject: =?UTF-8?Q?Re:_[PATCH_v2_13/21]_ipmi:_kcs=5Fbmc:_Decouple_the_IPMI_charde?= =?UTF-8?Q?v_from_the_core?= X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210409_072526_453079_1CA7AFA9 X-CRM114-Status: GOOD ( 23.47 ) 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, 9 Apr 2021, at 14:05, Zev Weiss wrote: > On Fri, Mar 19, 2021 at 01:27:44AM CDT, Andrew Jeffery wrote: > >Now that we have untangled the data-structures, split the userspace > >interface out into its own module. Userspace interfaces and drivers are > >registered to the KCS BMC core to support arbitrary binding of either. > > > >Signed-off-by: Andrew Jeffery > >--- > > drivers/char/ipmi/Kconfig | 13 +++++ > > drivers/char/ipmi/Makefile | 3 +- > > drivers/char/ipmi/kcs_bmc.c | 78 ++++++++++++++++++++++++++- > > drivers/char/ipmi/kcs_bmc.h | 4 -- > > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +++++++++--- > > drivers/char/ipmi/kcs_bmc_client.h | 14 +++++ > > 6 files changed, 132 insertions(+), 13 deletions(-) > > > >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig > >index 07847d9a459a..bc5f81899b62 100644 > >--- a/drivers/char/ipmi/Kconfig > >+++ b/drivers/char/ipmi/Kconfig > >@@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC > > This support is also available as a module. If so, the module > > will be called kcs_bmc_npcm7xx. > > > >+config IPMI_KCS_BMC_CDEV_IPMI > >+ depends on IPMI_KCS_BMC > >+ tristate "IPMI character device interface for BMC KCS devices" > >+ help > >+ Provides a BMC-side character device implementing IPMI > >+ semantics for KCS IPMI devices. > >+ > >+ Say YES if you wish to expose KCS devices on the BMC for IPMI > >+ purposes. > >+ > >+ This support is also available as a module. The module will be > >+ called kcs_bmc_cdev_ipmi. > >+ > > 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 a302bc865370..fcfa676afddb 100644 > >--- a/drivers/char/ipmi/Makefile > >+++ b/drivers/char/ipmi/Makefile > >@@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o > > 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 kcs_bmc_cdev_ipmi.o > >+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.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 > > obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o > >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c > >index 266ebec71d6f..694db6ee2a92 100644 > >--- a/drivers/char/ipmi/kcs_bmc.c > >+++ b/drivers/char/ipmi/kcs_bmc.c > >@@ -5,7 +5,9 @@ > > */ > > > > #include > >+#include > > #include > >+#include > > > > #include "kcs_bmc.h" > > > >@@ -13,6 +15,11 @@ > > #include "kcs_bmc_device.h" > > #include "kcs_bmc_client.h" > > > >+/* Record probed devices and cdevs */ > >+static DEFINE_MUTEX(kcs_bmc_lock); > >+static LIST_HEAD(kcs_bmc_devices); > >+static LIST_HEAD(kcs_bmc_cdevs); > >+ > > /* Consumer data access */ > > > > u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc) > >@@ -100,16 +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device); > > > > int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc) > > { > >- return kcs_bmc_ipmi_attach_cdev(kcs_bmc); > >+ struct kcs_bmc_cdev *cdev; > >+ int rc; > >+ > >+ spin_lock_init(&kcs_bmc->lock); > >+ kcs_bmc->client = NULL; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_add(&kcs_bmc->entry, &kcs_bmc_devices); > >+ list_for_each_entry(cdev, &kcs_bmc_cdevs, entry) { > >+ rc = cdev->ops->add_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return 0; > > We're ignoring failed ->add_device() calls here? Yep. If one chardev module is failing to accept new devices we don't want to not add them to the remaining chardev modules. What would the caller do with a error return value? Maybe it should just be void. > > > } > > EXPORT_SYMBOL(kcs_bmc_add_device); > > > > int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc) > > { > >- return kcs_bmc_ipmi_detach_cdev(kcs_bmc); > >+ struct kcs_bmc_cdev *cdev; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_del(&kcs_bmc->entry); > >+ list_for_each_entry(cdev, &kcs_bmc_cdevs, entry) { > >+ rc = cdev->ops->remove_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to remove chardev for KCS channel %d: %d", > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return 0; > > Similarly with the return value here... As above. > > > } > > EXPORT_SYMBOL(kcs_bmc_remove_device); > > > >+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev) > >+{ > >+ struct kcs_bmc_device *kcs_bmc; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_add(&cdev->entry, &kcs_bmc_cdevs); > >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { > >+ rc = cdev->ops->add_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return 0; > > ...return value again here... As above. > > >+} > >+EXPORT_SYMBOL(kcs_bmc_register_cdev); > >+ > >+int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev) > >+{ > >+ struct kcs_bmc_device *kcs_bmc; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_del(&cdev->entry); > >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { > >+ rc = cdev->ops->remove_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > > s/add/remove/ Thanks. > > Might also want to differentiate the *_device() error messages from the > *_cdev() ones a bit more? I'll look into it. > > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return rc; > > ...but this one is a bit incongruous, propagating the return value of > only the last ->remove_device() call. Hah. good catch. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel