Re: [PATCH v2 15/21] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel

[Andrew Jeffery]

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 at 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