From: "Andrew Jeffery" <andrew@aj.id.au>
To: openbmc@lists.ozlabs.org
Subject: Re: [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel
Date: Fri, 09 Apr 2021 16:12:39 +0930 [thread overview]
Message-ID: <f2f05ab5-5403-462c-807b-369251cc6b17@www.fastmail.com> (raw)
In-Reply-To: <YG/g/poZLwO34QH7@packtop>
On Fri, 9 Apr 2021, at 14:37, Zev Weiss wrote:
> On Fri, Mar 19, 2021 at 01:27:46AM CDT, Andrew Jeffery wrote:
> >Soon it will be possible for one KCS device to have multiple associated
> >chardevs exposed to userspace (for IPMI and raw-style access). However,
> >don't prevent userspace from:
> >
> >1. Opening more than one chardev at a time, or
> >2. Opening the same chardev more than once.
> >
> >System behaviour is undefined for both classes of multiple access, so
> >userspace must manage itself accordingly.
> >
> >The implementation delivers IBF and OBF events to the first chardev
> >client to associate with the KCS device. An open on a related chardev
> >cannot associate its client with the KCS device and so will not
> >receive notification of events. However, any fd on any chardev may race
> >their accesses to the data and status registers.
> >
> >Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> >---
> > drivers/char/ipmi/kcs_bmc.c | 34 ++++++++++-------------------
> > drivers/char/ipmi/kcs_bmc_aspeed.c | 3 +--
> > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 3 +--
> > 3 files changed, 14 insertions(+), 26 deletions(-)
> >
> >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> >index 05bbb72418b2..2fafa9541934 100644
> >--- a/drivers/char/ipmi/kcs_bmc.c
> >+++ b/drivers/char/ipmi/kcs_bmc.c
> >@@ -55,24 +55,12 @@ EXPORT_SYMBOL(kcs_bmc_update_status);
> > int kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
> > {
> > struct kcs_bmc_client *client;
> >- int rc;
> >+ int rc = KCS_BMC_EVENT_NONE;
> >
> > spin_lock(&kcs_bmc->lock);
> > client = kcs_bmc->client;
> >- if (client) {
> >+ if (!WARN_ON_ONCE(!client))
> > rc = client->ops->event(client);
>
> The double-negation split by a macro seems a bit confusing to me
> readability-wise;
I did a poll internally about that and I didn't get any complaints :D
> could we simplify to something like
>
> if (client)
> rc = client->ops->event(client);
> else
> WARN_ONCE();
>
> ?
I guess.
>
> >- } else {
> >- u8 status;
> >-
> >- status = kcs_bmc_read_status(kcs_bmc);
> >- if (status & KCS_BMC_STR_IBF) {
> >- /* Ack the event by reading the data */
> >- kcs_bmc_read_data(kcs_bmc);
> >- rc = KCS_BMC_EVENT_HANDLED;
> >- } else {
> >- rc = KCS_BMC_EVENT_NONE;
> >- }
> >- }
> > spin_unlock(&kcs_bmc->lock);
> >
> > return rc;
> >@@ -81,26 +69,28 @@ EXPORT_SYMBOL(kcs_bmc_handle_event);
> >
> > int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
> > {
> >- int rc;
> >-
> > spin_lock_irq(&kcs_bmc->lock);
> >- if (kcs_bmc->client) {
> >- rc = -EBUSY;
> >- } else {
> >+ if (!kcs_bmc->client) {
> >+ u8 mask = KCS_BMC_EVENT_TYPE_IBF;
> >+
> > kcs_bmc->client = client;
> >- rc = 0;
> >+ kcs_bmc_update_event_mask(kcs_bmc, mask, mask);
> > }
> > spin_unlock_irq(&kcs_bmc->lock);
> >
> >- return rc;
> >+ return 0;
>
> Since this function appears to be infallible now, should it just return
> void? (Might be more churn than it's worth...shrug.)
Yeah, I think I was being a little lazy here.
Cheers,
Andrew
next prev parent reply other threads:[~2021-04-09 6:43 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 6:27 [PATCH v2 01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 02/21] ARM: dts: Remove LPC BMC and Host partitions Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 03/21] ipmi: kcs: aspeed: Adapt to new LPC DTS layout Andrew Jeffery
2021-04-09 3:35 ` Joel Stanley
2021-03-19 6:27 ` [PATCH v2 04/21] pinctrl: aspeed-g5: Adapt to new LPC device tree layout Andrew Jeffery
2021-04-09 3:36 ` Joel Stanley
2021-03-19 6:27 ` [PATCH v2 05/21] soc: aspeed: " Andrew Jeffery
2021-04-09 3:38 ` Joel Stanley
2021-03-19 6:27 ` [PATCH v2 06/21] ipmi: kcs_bmc_aspeed: Use of match data to extract KCS properties Andrew Jeffery
2021-04-06 6:07 ` ChiaWei Wang
2021-04-09 3:24 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 07/21] ipmi: kcs_bmc: Make status update atomic Andrew Jeffery
2021-04-09 5:32 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 08/21] ipmi: kcs_bmc: Rename {read, write}_{status, data}() functions Andrew Jeffery
2021-04-09 5:33 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 09/21] ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi Andrew Jeffery
2021-04-09 3:56 ` Zev Weiss
2021-04-09 5:48 ` Andrew Jeffery
2021-04-09 19:21 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 10/21] ipmi: kcs_bmc: Turn the driver data-structures inside-out Andrew Jeffery
2021-04-09 3:57 ` Zev Weiss
2021-04-09 5:59 ` Andrew Jeffery
2021-04-09 6:25 ` Zev Weiss
2021-04-09 19:26 ` Zev Weiss
2021-04-11 23:00 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 11/21] ipmi: kcs_bmc: Split headers into device and client Andrew Jeffery
2021-04-09 4:01 ` Zev Weiss
2021-04-09 6:06 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 12/21] ipmi: kcs_bmc: Strip private client data from struct kcs_bmc Andrew Jeffery
2021-04-09 4:07 ` Zev Weiss
2021-04-09 6:15 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core Andrew Jeffery
2021-04-06 6:07 ` ChiaWei Wang
2021-04-09 4:35 ` Zev Weiss
2021-04-09 6:24 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 14/21] ipmi: kcs_bmc: Allow clients to control KCS IRQ state Andrew Jeffery
2021-04-09 4:37 ` Zev Weiss
2021-04-09 6:39 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel Andrew Jeffery
2021-04-09 5:07 ` Zev Weiss
2021-04-09 6:42 ` Andrew Jeffery [this message]
2021-03-19 6:27 ` [PATCH v2 16/21] ipmi: kcs_bmc: Add a "raw" character device interface Andrew Jeffery
2021-04-09 5:17 ` Zev Weiss
2021-04-09 6:46 ` Andrew Jeffery
2021-04-09 7:55 ` Arnd Bergmann
2021-04-12 1:33 ` Andrew Jeffery
2021-04-12 8:48 ` Arnd Bergmann
2021-04-12 23:45 ` Andrew Jeffery
2021-04-13 8:22 ` Arnd Bergmann
2021-04-14 0:30 ` Andrew Jeffery
2021-03-19 6:27 ` [PATCH v2 17/21] dt-bindings: ipmi: Convert ASPEED KCS binding to schema Andrew Jeffery
2021-03-26 1:48 ` Rob Herring
2021-04-09 5:15 ` Zev Weiss
2021-04-09 5:33 ` Andrew Jeffery
2021-04-09 5:44 ` Zev Weiss
2021-04-09 8:46 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 18/21] dt-bindings: ipmi: Add optional SerIRQ property to ASPEED KCS devices Andrew Jeffery
2021-03-26 1:49 ` Rob Herring
2021-03-19 6:27 ` [PATCH v2 19/21] ipmi: kcs_bmc_aspeed: Implement KCS SerIRQ configuration Andrew Jeffery
2021-04-01 9:30 ` [EXTERNAL] " Zev Weiss
2021-03-19 6:27 ` [PATCH v2 20/21] ipmi: kcs_bmc_aspeed: Fix IBFIE typo from datasheet Andrew Jeffery
2021-04-09 5:40 ` Zev Weiss
2021-03-19 6:27 ` [PATCH v2 21/21] ipmi: kcs_bmc_aspeed: Optionally apply status address Andrew Jeffery
2021-04-01 18:18 ` Re " Zev Weiss
2021-04-06 6:09 ` ChiaWei Wang
2021-04-09 3:18 ` [PATCH v2 01/21] dt-bindings: aspeed-lpc: Remove LPC partitioning Joel Stanley
2021-04-09 5:24 ` Andrew Jeffery
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f2f05ab5-5403-462c-807b-369251cc6b17@www.fastmail.com \
--to=andrew@aj.id.au \
--cc=openbmc@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).