diff mbox series

[v3,10/16] ipmi: kcs_bmc: Don't enforce single-open policy in the kernel

Message ID 20210510054213.1610760-11-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 May 10, 2021, 5:42 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

Corey Minyard May 21, 2021, 5:30 p.m. UTC | #1 
On Mon, May 10, 2021 at 03:12:07PM +0930, 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.

I don't understand why you want to allow this.  If the second open won't
work right, then why allow it?  Why remove code that causes the second
open to error?

-corey

> 
> 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 7081541bb6ce..ad9ff13ba831 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);
>  irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
>  {
>  	struct kcs_bmc_client *client;
> -	irqreturn_t rc;
> +	irqreturn_t rc = IRQ_NONE;
>  
>  	spin_lock(&kcs_bmc->lock);
>  	client = kcs_bmc->client;
> -	if (client) {
> +	if (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 = IRQ_HANDLED;
> -		} else {
> -			rc = IRQ_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 8b223e58d900..8a0b1e18e945 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> @@ -414,8 +414,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);
>  
>  	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 f8b7162fb830..ab4a8caf1270 100644
> --- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
> +++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
> @@ -202,8 +202,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
>
Andrew Jeffery May 24, 2021, 12:39 a.m. UTC | #2 
On Sat, 22 May 2021, at 03:00, Corey Minyard wrote:
> On Mon, May 10, 2021 at 03:12:07PM +0930, 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.
> 
> I don't understand why you want to allow this.  If the second open won't
> work right, then why allow it?  Why remove code that causes the second
> open to error?

Really I was just shifting the problem to userspace so it wasn't 
something I needed to address in the kernel. It seems I'm alone in 
thinking this is a good idea, as yourself, Zev, William and Joel 
(privately) have pushed back against it. Initially the idea was tied up 
in how I was doing some interrupt handling, but in revising the code 
that problem has gone away.

I'll just drop this patch and save everyone the heartburn of arguing 
about it :)

Andrew
diff mbox series

Patch

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 7081541bb6ce..ad9ff13ba831 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);
 irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
 {
 	struct kcs_bmc_client *client;
-	irqreturn_t rc;
+	irqreturn_t rc = IRQ_NONE;
 
 	spin_lock(&kcs_bmc->lock);
 	client = kcs_bmc->client;
-	if (client) {
+	if (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 = IRQ_HANDLED;
-		} else {
-			rc = IRQ_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 8b223e58d900..8a0b1e18e945 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -414,8 +414,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);
 
 	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 f8b7162fb830..ab4a8caf1270 100644
--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
@@ -202,8 +202,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",