| Message ID | 20210319062752.145730-15-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | ipmi: Allow raw access to KCS devices | expand |
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 >
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
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",
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(-)