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

[Zev Weiss]

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@xxxxxxxx>
>---
> 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; could we simplify to something like

	if (client)
		rc = client->ops->event(client);
	else
		WARN_ONCE();

?

>-	} 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.)

> }
> EXPORT_SYMBOL(kcs_bmc_enable_device);
>
> void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
> {
> 	spin_lock_irq(&kcs_bmc->lock);
>-	if (client == kcs_bmc->client)
>+	if (client == kcs_bmc->client) {
>+		u8 mask = KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE;
>+
>+		kcs_bmc_update_event_mask(kcs_bmc, mask, 0);
> 		kcs_bmc->client = NULL;
>+	}
> 	spin_unlock_irq(&kcs_bmc->lock);
> }
> EXPORT_SYMBOL(kcs_bmc_disable_device);
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
>index 5f26471c038c..271845eb2e26 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -419,8 +419,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>
> 	platform_set_drvdata(pdev, priv);
>
>-	aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE),
>-				   KCS_BMC_EVENT_TYPE_IBF);
>+	aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0);
> 	aspeed_kcs_enable_channel(kcs_bmc, true);
>
> 	rc = kcs_bmc_add_device(&priv->kcs_bmc);
>diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>index c2032728a03d..fdf35cad2eba 100644
>--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
>@@ -207,8 +207,7 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
> 	if (rc)
> 		return rc;
>
>-	npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE),
>-				    KCS_BMC_EVENT_TYPE_IBF);
>+	npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0);
> 	npcm7xx_kcs_enable_channel(kcs_bmc, true);
>
> 	pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n",
>-- 
>2.27.0
>