diff mbox series

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

Message ID 20210319062752.145730-15-andrew@aj.id.au (mailing list archive)
State New, archived
Headers show
Series ipmi: Allow raw access to KCS devices | expand

Commit Message

Andrew Jeffery March 19, 2021, 6:27 a.m. UTC 
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(-)

Comments

Zev Weiss April 9, 2021, 5:07 a.m. UTC | #1 
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; 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
>
diff mbox series

Patch

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);
-	} 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;
 }
 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",