Re: [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core

2021-04-09 Thread Andrew Jeffery



On Fri, 9 Apr 2021, at 14:05, Zev Weiss wrote:
> On Fri, Mar 19, 2021 at 01:27:44AM CDT, Andrew Jeffery wrote:
> >Now that we have untangled the data-structures, split the userspace
> >interface out into its own module. Userspace interfaces and drivers are
> >registered to the KCS BMC core to support arbitrary binding of either.
> >
> >Signed-off-by: Andrew Jeffery 
> >---
> > drivers/char/ipmi/Kconfig | 13 +
> > drivers/char/ipmi/Makefile|  3 +-
> > drivers/char/ipmi/kcs_bmc.c   | 78 ++-
> > drivers/char/ipmi/kcs_bmc.h   |  4 --
> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +---
> > drivers/char/ipmi/kcs_bmc_client.h| 14 +
> > 6 files changed, 132 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> >index 07847d9a459a..bc5f81899b62 100644
> >--- a/drivers/char/ipmi/Kconfig
> >+++ b/drivers/char/ipmi/Kconfig
> >@@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC
> >   This support is also available as a module.  If so, the module
> >   will be called kcs_bmc_npcm7xx.
> >
> >+config IPMI_KCS_BMC_CDEV_IPMI
> >+depends on IPMI_KCS_BMC
> >+tristate "IPMI character device interface for BMC KCS devices"
> >+help
> >+  Provides a BMC-side character device implementing IPMI
> >+  semantics for KCS IPMI devices.
> >+
> >+  Say YES if you wish to expose KCS devices on the BMC for IPMI
> >+  purposes.
> >+
> >+  This support is also available as a module. The module will be
> >+  called kcs_bmc_cdev_ipmi.
> >+
> > config ASPEED_BT_IPMI_BMC
> > depends on ARCH_ASPEED || COMPILE_TEST
> > depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
> >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> >index a302bc865370..fcfa676afddb 100644
> >--- a/drivers/char/ipmi/Makefile
> >+++ b/drivers/char/ipmi/Makefile
> >@@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
> > obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
> > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
> > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
> >-obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o
> >+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
> >+obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o
> > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
> > obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
> > obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
> >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> >index 266ebec71d6f..694db6ee2a92 100644
> >--- a/drivers/char/ipmi/kcs_bmc.c
> >+++ b/drivers/char/ipmi/kcs_bmc.c
> >@@ -5,7 +5,9 @@
> >  */
> >
> > #include 
> >+#include 
> > #include 
> >+#include 
> >
> > #include "kcs_bmc.h"
> >
> >@@ -13,6 +15,11 @@
> > #include "kcs_bmc_device.h"
> > #include "kcs_bmc_client.h"
> >
> >+/* Record probed devices and cdevs */
> >+static DEFINE_MUTEX(kcs_bmc_lock);
> >+static LIST_HEAD(kcs_bmc_devices);
> >+static LIST_HEAD(kcs_bmc_cdevs);
> >+
> > /* Consumer data access */
> >
> > u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
> >@@ -100,16 +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device);
> >
> > int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
> > {
> >-return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> >+struct kcs_bmc_cdev *cdev;
> >+int rc;
> >+
> >+spin_lock_init(_bmc->lock);
> >+kcs_bmc->client = NULL;
> >+
> >+mutex_lock(_bmc_lock);
> >+list_add(_bmc->entry, _bmc_devices);
> >+list_for_each_entry(cdev, _bmc_cdevs, entry) {
> >+rc = cdev->ops->add_device(kcs_bmc);
> >+if (rc)
> >+dev_err(kcs_bmc->dev, "Failed to add chardev for KCS 
> >channel %d: %d",
> >+kcs_bmc->channel, rc);
> >+}
> >+mutex_unlock(_bmc_lock);
> >+
> >+return 0;
> 
> We're ignoring failed ->add_device() calls here?

Yep. If one chardev module is failing to accept new devices we don't 
want to not add them to the remaining chardev modules.

What would the caller do with a error return value? Maybe it should 
just be void.

> 
> > }
> > EXPORT_SYMBOL(kcs_bmc_add_device);
> >
> > int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
> > {
> >-return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
> >+struct kcs_bmc_cdev *cdev;
> >+int rc;
> >+
> >+mutex_lock(_bmc_lock);
> >+list_del(_bmc->entry);
> >+list_for_each_entry(cdev, _bmc_cdevs, entry) {
> >+rc = cdev->ops->remove_device(kcs_bmc);
> >+if (rc)
> >+dev_err(kcs_bmc->dev, "Failed to remove chardev for KCS 
> >channel %d: %d",
> >+kcs_bmc->channel, rc);
> >+}
> >+mutex_unlock(_bmc_lock);
> >+
> >+return 0;
> 
> Similarly with the return value here...

As above.

> 
> > }
> > EXPORT_SYMBOL(kcs_bmc_remove_device);
> >
> >+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev)
> >+{
> >+struct kcs_bmc_device *kcs_bmc;
> 

Re: [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core

2021-04-08 Thread Zev Weiss
On Fri, Mar 19, 2021 at 01:27:44AM CDT, Andrew Jeffery wrote:
>Now that we have untangled the data-structures, split the userspace
>interface out into its own module. Userspace interfaces and drivers are
>registered to the KCS BMC core to support arbitrary binding of either.
>
>Signed-off-by: Andrew Jeffery 
>---
> drivers/char/ipmi/Kconfig | 13 +
> drivers/char/ipmi/Makefile|  3 +-
> drivers/char/ipmi/kcs_bmc.c   | 78 ++-
> drivers/char/ipmi/kcs_bmc.h   |  4 --
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +---
> drivers/char/ipmi/kcs_bmc_client.h| 14 +
> 6 files changed, 132 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
>index 07847d9a459a..bc5f81899b62 100644
>--- a/drivers/char/ipmi/Kconfig
>+++ b/drivers/char/ipmi/Kconfig
>@@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC
> This support is also available as a module.  If so, the module
> will be called kcs_bmc_npcm7xx.
>
>+config IPMI_KCS_BMC_CDEV_IPMI
>+  depends on IPMI_KCS_BMC
>+  tristate "IPMI character device interface for BMC KCS devices"
>+  help
>+Provides a BMC-side character device implementing IPMI
>+semantics for KCS IPMI devices.
>+
>+Say YES if you wish to expose KCS devices on the BMC for IPMI
>+purposes.
>+
>+This support is also available as a module. The module will be
>+called kcs_bmc_cdev_ipmi.
>+
> config ASPEED_BT_IPMI_BMC
>   depends on ARCH_ASPEED || COMPILE_TEST
>   depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
>diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
>index a302bc865370..fcfa676afddb 100644
>--- a/drivers/char/ipmi/Makefile
>+++ b/drivers/char/ipmi/Makefile
>@@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
> obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
> obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
> obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
>-obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o
>+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
>+obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o
> obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
> obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
> obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
>diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
>index 266ebec71d6f..694db6ee2a92 100644
>--- a/drivers/char/ipmi/kcs_bmc.c
>+++ b/drivers/char/ipmi/kcs_bmc.c
>@@ -5,7 +5,9 @@
>  */
>
> #include 
>+#include 
> #include 
>+#include 
>
> #include "kcs_bmc.h"
>
>@@ -13,6 +15,11 @@
> #include "kcs_bmc_device.h"
> #include "kcs_bmc_client.h"
>
>+/* Record probed devices and cdevs */
>+static DEFINE_MUTEX(kcs_bmc_lock);
>+static LIST_HEAD(kcs_bmc_devices);
>+static LIST_HEAD(kcs_bmc_cdevs);
>+
> /* Consumer data access */
>
> u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
>@@ -100,16 +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device);
>
> int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
> {
>-  return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
>+  struct kcs_bmc_cdev *cdev;
>+  int rc;
>+
>+  spin_lock_init(_bmc->lock);
>+  kcs_bmc->client = NULL;
>+
>+  mutex_lock(_bmc_lock);
>+  list_add(_bmc->entry, _bmc_devices);
>+  list_for_each_entry(cdev, _bmc_cdevs, entry) {
>+  rc = cdev->ops->add_device(kcs_bmc);
>+  if (rc)
>+  dev_err(kcs_bmc->dev, "Failed to add chardev for KCS 
>channel %d: %d",
>+  kcs_bmc->channel, rc);
>+  }
>+  mutex_unlock(_bmc_lock);
>+
>+  return 0;

We're ignoring failed ->add_device() calls here?

> }
> EXPORT_SYMBOL(kcs_bmc_add_device);
>
> int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
> {
>-  return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
>+  struct kcs_bmc_cdev *cdev;
>+  int rc;
>+
>+  mutex_lock(_bmc_lock);
>+  list_del(_bmc->entry);
>+  list_for_each_entry(cdev, _bmc_cdevs, entry) {
>+  rc = cdev->ops->remove_device(kcs_bmc);
>+  if (rc)
>+  dev_err(kcs_bmc->dev, "Failed to remove chardev for KCS 
>channel %d: %d",
>+  kcs_bmc->channel, rc);
>+  }
>+  mutex_unlock(_bmc_lock);
>+
>+  return 0;

Similarly with the return value here...

> }
> EXPORT_SYMBOL(kcs_bmc_remove_device);
>
>+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev)
>+{
>+  struct kcs_bmc_device *kcs_bmc;
>+  int rc;
>+
>+  mutex_lock(_bmc_lock);
>+  list_add(>entry, _bmc_cdevs);
>+  list_for_each_entry(kcs_bmc, _bmc_devices, entry) {
>+  rc = cdev->ops->add_device(kcs_bmc);
>+  if (rc)
>+  dev_err(kcs_bmc->dev, "Failed to add chardev for KCS 
>channel %d: %d",
>+  kcs_bmc->channel, rc);
>+  }
>+  mutex_unlock(_bmc_lock);
>+
>+  return 0;

...return value again 

RE: [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core

2021-04-06 Thread ChiaWei Wang
I have tried this patch on Intel EGS CRB with AST2600 A1 as the BMC.
Chiawei

Tested-by: Chia-Wei Wang 

> -Original Message-
> From: Andrew Jeffery 
> Sent: Friday, March 19, 2021 2:28 PM
> To: openipmi-develo...@lists.sourceforge.net; open...@lists.ozlabs.org;
> miny...@acm.org
> Subject: [PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the
> core
> 
> Now that we have untangled the data-structures, split the userspace interface
> out into its own module. Userspace interfaces and drivers are registered to 
> the
> KCS BMC core to support arbitrary binding of either.
> 
> Signed-off-by: Andrew Jeffery 
> ---
>  drivers/char/ipmi/Kconfig | 13 +
>  drivers/char/ipmi/Makefile|  3 +-
>  drivers/char/ipmi/kcs_bmc.c   | 78
> ++-
>  drivers/char/ipmi/kcs_bmc.h   |  4 --
>  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +---
>  drivers/char/ipmi/kcs_bmc_client.h| 14 +
>  6 files changed, 132 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig index
> 07847d9a459a..bc5f81899b62 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC
> This support is also available as a module.  If so, the module
> will be called kcs_bmc_npcm7xx.
> 
> +config IPMI_KCS_BMC_CDEV_IPMI
> + depends on IPMI_KCS_BMC
> + tristate "IPMI character device interface for BMC KCS devices"
> + help
> +   Provides a BMC-side character device implementing IPMI
> +   semantics for KCS IPMI devices.
> +
> +   Say YES if you wish to expose KCS devices on the BMC for IPMI
> +   purposes.
> +
> +   This support is also available as a module. The module will be
> +   called kcs_bmc_cdev_ipmi.
> +
>  config ASPEED_BT_IPMI_BMC
>   depends on ARCH_ASPEED || COMPILE_TEST
>   depends on REGMAP && REGMAP_MMIO && MFD_SYSCON diff --git
> a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile index
> a302bc865370..fcfa676afddb 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
>  obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
>  obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
>  obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
> -obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o
> +obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
> +obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o
>  obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
>  obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
>  obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o diff --git
> a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c index
> 266ebec71d6f..694db6ee2a92 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -5,7 +5,9 @@
>   */
> 
>  #include 
> +#include 
>  #include 
> +#include 
> 
>  #include "kcs_bmc.h"
> 
> @@ -13,6 +15,11 @@
>  #include "kcs_bmc_device.h"
>  #include "kcs_bmc_client.h"
> 
> +/* Record probed devices and cdevs */
> +static DEFINE_MUTEX(kcs_bmc_lock);
> +static LIST_HEAD(kcs_bmc_devices);
> +static LIST_HEAD(kcs_bmc_cdevs);
> +
>  /* Consumer data access */
> 
>  u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc) @@ -100,16
> +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device);
> 
>  int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)  {
> - return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
> + struct kcs_bmc_cdev *cdev;
> + int rc;
> +
> + spin_lock_init(_bmc->lock);
> + kcs_bmc->client = NULL;
> +
> + mutex_lock(_bmc_lock);
> + list_add(_bmc->entry, _bmc_devices);
> + list_for_each_entry(cdev, _bmc_cdevs, entry) {
> + rc = cdev->ops->add_device(kcs_bmc);
> + if (rc)
> + dev_err(kcs_bmc->dev, "Failed to add chardev for KCS 
> channel
> %d: %d",
> + kcs_bmc->channel, rc);
> + }
> + mutex_unlock(_bmc_lock);
> +
> + return 0;
>  }
>  EXPORT_SYMBOL(kcs_bmc_add_device);
> 
>  int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)  {
> - return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
> + struct kcs_bmc_cdev *cdev;
> + int rc;
> +
> + mutex_lock(_bmc_lock);
> + list_del(_bmc->entry);
> + list_for_each_entry(cdev, _bmc_cdevs, entry) {
> + rc = cdev->ops->remove_device(kcs_bmc);
> + if (rc)
> + dev_err(kcs_bmc-

[PATCH v2 13/21] ipmi: kcs_bmc: Decouple the IPMI chardev from the core

2021-03-19 Thread Andrew Jeffery
Now that we have untangled the data-structures, split the userspace
interface out into its own module. Userspace interfaces and drivers are
registered to the KCS BMC core to support arbitrary binding of either.

Signed-off-by: Andrew Jeffery 
---
 drivers/char/ipmi/Kconfig | 13 +
 drivers/char/ipmi/Makefile|  3 +-
 drivers/char/ipmi/kcs_bmc.c   | 78 ++-
 drivers/char/ipmi/kcs_bmc.h   |  4 --
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 33 +---
 drivers/char/ipmi/kcs_bmc_client.h| 14 +
 6 files changed, 132 insertions(+), 13 deletions(-)

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index 07847d9a459a..bc5f81899b62 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -124,6 +124,19 @@ config NPCM7XX_KCS_IPMI_BMC
  This support is also available as a module.  If so, the module
  will be called kcs_bmc_npcm7xx.
 
+config IPMI_KCS_BMC_CDEV_IPMI
+   depends on IPMI_KCS_BMC
+   tristate "IPMI character device interface for BMC KCS devices"
+   help
+ Provides a BMC-side character device implementing IPMI
+ semantics for KCS IPMI devices.
+
+ Say YES if you wish to expose KCS devices on the BMC for IPMI
+ purposes.
+
+ This support is also available as a module. The module will be
+ called kcs_bmc_cdev_ipmi.
+
 config ASPEED_BT_IPMI_BMC
depends on ARCH_ASPEED || COMPILE_TEST
depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index a302bc865370..fcfa676afddb 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -22,7 +22,8 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
 obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
 obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
 obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
-obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o kcs_bmc_cdev_ipmi.o
+obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
+obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
 obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
 obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 266ebec71d6f..694db6ee2a92 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -5,7 +5,9 @@
  */
 
 #include 
+#include 
 #include 
+#include 
 
 #include "kcs_bmc.h"
 
@@ -13,6 +15,11 @@
 #include "kcs_bmc_device.h"
 #include "kcs_bmc_client.h"
 
+/* Record probed devices and cdevs */
+static DEFINE_MUTEX(kcs_bmc_lock);
+static LIST_HEAD(kcs_bmc_devices);
+static LIST_HEAD(kcs_bmc_cdevs);
+
 /* Consumer data access */
 
 u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
@@ -100,16 +107,83 @@ EXPORT_SYMBOL(kcs_bmc_disable_device);
 
 int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
 {
-   return kcs_bmc_ipmi_attach_cdev(kcs_bmc);
+   struct kcs_bmc_cdev *cdev;
+   int rc;
+
+   spin_lock_init(_bmc->lock);
+   kcs_bmc->client = NULL;
+
+   mutex_lock(_bmc_lock);
+   list_add(_bmc->entry, _bmc_devices);
+   list_for_each_entry(cdev, _bmc_cdevs, entry) {
+   rc = cdev->ops->add_device(kcs_bmc);
+   if (rc)
+   dev_err(kcs_bmc->dev, "Failed to add chardev for KCS 
channel %d: %d",
+   kcs_bmc->channel, rc);
+   }
+   mutex_unlock(_bmc_lock);
+
+   return 0;
 }
 EXPORT_SYMBOL(kcs_bmc_add_device);
 
 int kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
 {
-   return kcs_bmc_ipmi_detach_cdev(kcs_bmc);
+   struct kcs_bmc_cdev *cdev;
+   int rc;
+
+   mutex_lock(_bmc_lock);
+   list_del(_bmc->entry);
+   list_for_each_entry(cdev, _bmc_cdevs, entry) {
+   rc = cdev->ops->remove_device(kcs_bmc);
+   if (rc)
+   dev_err(kcs_bmc->dev, "Failed to remove chardev for KCS 
channel %d: %d",
+   kcs_bmc->channel, rc);
+   }
+   mutex_unlock(_bmc_lock);
+
+   return 0;
 }
 EXPORT_SYMBOL(kcs_bmc_remove_device);
 
+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev)
+{
+   struct kcs_bmc_device *kcs_bmc;
+   int rc;
+
+   mutex_lock(_bmc_lock);
+   list_add(>entry, _bmc_cdevs);
+   list_for_each_entry(kcs_bmc, _bmc_devices, entry) {
+   rc = cdev->ops->add_device(kcs_bmc);
+   if (rc)
+   dev_err(kcs_bmc->dev, "Failed to add chardev for KCS 
channel %d: %d",
+   kcs_bmc->channel, rc);
+   }
+   mutex_unlock(_bmc_lock);
+
+   return 0;
+}
+EXPORT_SYMBOL(kcs_bmc_register_cdev);
+
+int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev)
+{
+   struct kcs_bmc_device *kcs_bmc;
+   int rc;
+
+   mutex_lock(_bmc_lock);
+   list_del(>entry);
+   list_for_each_entry(kcs_bmc,