| Message ID | 20210319062752.145730-13-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | ipmi: Allow raw access to KCS devices | expand |
I have tried this patch on Intel EGS CRB with AST2600 A1 as the BMC. Chiawei Tested-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > Sent: Friday, March 19, 2021 2:28 PM > To: openipmi-developer@lists.sourceforge.net; openbmc@lists.ozlabs.org; > minyard@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 <andrew@aj.id.au> > --- > 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 <linux/device.h> > +#include <linux/list.h> > #include <linux/module.h> > +#include <linux/mutex.h> > > #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(&kcs_bmc->lock); > + kcs_bmc->client = NULL; > + > + mutex_lock(&kcs_bmc_lock); > + list_add(&kcs_bmc->entry, &kcs_bmc_devices); > + list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); > + list_del(&kcs_bmc->entry); > + list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); > + list_add(&cdev->entry, &kcs_bmc_cdevs); > + list_for_each_entry(kcs_bmc, &kcs_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(&kcs_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(&kcs_bmc_lock); > + list_del(&cdev->entry); > + list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { > + rc = cdev->ops->remove_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(&kcs_bmc_lock); > + > + return rc; > +} > +EXPORT_SYMBOL(kcs_bmc_unregister_cdev); > + > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); > MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); diff --git > a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h index > 3f266740c759..5deb9a0b8e60 100644 > --- a/drivers/char/ipmi/kcs_bmc.h > +++ b/drivers/char/ipmi/kcs_bmc.h > @@ -42,8 +42,4 @@ struct kcs_bmc_device { > spinlock_t lock; > struct kcs_bmc_client *client; > }; > - > -/* Temporary exports while refactoring */ -int > kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); -int > kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); #endif /* > __KCS_BMC_H__ */ diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > index 58c42e76483d..df83d67851ac 100644 > --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > @@ -469,8 +469,7 @@ static const struct file_operations kcs_bmc_ipmi_fops > = { static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock); > static LIST_HEAD(kcs_bmc_ipmi_instances); > > -int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); -int > kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) > +static int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_ipmi *priv; > int rc; > @@ -512,10 +511,8 @@ int kcs_bmc_ipmi_attach_cdev(struct > kcs_bmc_device *kcs_bmc) > > return 0; > } > -EXPORT_SYMBOL(kcs_bmc_ipmi_attach_cdev); > > -int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); -int > kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) > +static int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_ipmi *priv = NULL, *pos; > > @@ -541,7 +538,31 @@ int kcs_bmc_ipmi_detach_cdev(struct > kcs_bmc_device *kcs_bmc) > > return 0; > } > -EXPORT_SYMBOL(kcs_bmc_ipmi_detach_cdev); > + > +static const struct kcs_bmc_cdev_ops kcs_bmc_ipmi_cdev_ops = { > + .add_device = kcs_bmc_ipmi_attach_cdev, > + .remove_device = kcs_bmc_ipmi_detach_cdev, }; > + > +static struct kcs_bmc_cdev kcs_bmc_ipmi_cdev = { > + .ops = &kcs_bmc_ipmi_cdev_ops, > +}; > + > +static int kcs_bmc_ipmi_init(void) > +{ > + return kcs_bmc_register_cdev(&kcs_bmc_ipmi_cdev); > +} > +module_init(kcs_bmc_ipmi_init); > + > +static void kcs_bmc_ipmi_exit(void) > +{ > + int rc; > + > + rc = kcs_bmc_unregister_cdev(&kcs_bmc_ipmi_cdev); > + if (rc) > + pr_warn("Failed to remove KCS BMC client: %d", rc); } > +module_exit(kcs_bmc_ipmi_exit); > > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); diff > --git a/drivers/char/ipmi/kcs_bmc_client.h > b/drivers/char/ipmi/kcs_bmc_client.h > index 2dd710f4b4aa..d0a7404ff584 100644 > --- a/drivers/char/ipmi/kcs_bmc_client.h > +++ b/drivers/char/ipmi/kcs_bmc_client.h > @@ -10,6 +10,17 @@ > > #include "kcs_bmc.h" > > +struct kcs_bmc_cdev_ops { > + int (*add_device)(struct kcs_bmc_device *kcs_bmc); > + int (*remove_device)(struct kcs_bmc_device *kcs_bmc); }; > + > +struct kcs_bmc_cdev { > + struct list_head entry; > + > + const struct kcs_bmc_cdev_ops *ops; > +}; > + > struct kcs_bmc_client_ops { > int (*event)(struct kcs_bmc_client *client); }; @@ -20,6 +31,9 @@ > struct kcs_bmc_client { > struct kcs_bmc_device *dev; > }; > > +int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev); int > +kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev); > + > int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct > kcs_bmc_client *client); void kcs_bmc_disable_device(struct > kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > > -- > 2.27.0
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 <andrew@aj.id.au> >--- > 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 <linux/device.h> >+#include <linux/list.h> > #include <linux/module.h> >+#include <linux/mutex.h> > > #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(&kcs_bmc->lock); >+ kcs_bmc->client = NULL; >+ >+ mutex_lock(&kcs_bmc_lock); >+ list_add(&kcs_bmc->entry, &kcs_bmc_devices); >+ list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); >+ list_del(&kcs_bmc->entry); >+ list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); >+ list_add(&cdev->entry, &kcs_bmc_cdevs); >+ list_for_each_entry(kcs_bmc, &kcs_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(&kcs_bmc_lock); >+ >+ return 0; ...return value again here... >+} >+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(&kcs_bmc_lock); >+ list_del(&cdev->entry); >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { >+ rc = cdev->ops->remove_device(kcs_bmc); >+ if (rc) >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", s/add/remove/ Might also want to differentiate the *_device() error messages from the *_cdev() ones a bit more? >+ kcs_bmc->channel, rc); >+ } >+ mutex_unlock(&kcs_bmc_lock); >+ >+ return rc; ...but this one is a bit incongruous, propagating the return value of only the last ->remove_device() call. (I'd have expected this to trigger a warning about returning a potentially uninitialized 'rc', but in some manual testing it doesn't seem to do so for me...not certain why.) >+} >+EXPORT_SYMBOL(kcs_bmc_unregister_cdev); >+ > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); > MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h >index 3f266740c759..5deb9a0b8e60 100644 >--- a/drivers/char/ipmi/kcs_bmc.h >+++ b/drivers/char/ipmi/kcs_bmc.h >@@ -42,8 +42,4 @@ struct kcs_bmc_device { > spinlock_t lock; > struct kcs_bmc_client *client; > }; >- >-/* Temporary exports while refactoring */ >-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); >-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); > #endif /* __KCS_BMC_H__ */ >diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >index 58c42e76483d..df83d67851ac 100644 >--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c >@@ -469,8 +469,7 @@ static const struct file_operations kcs_bmc_ipmi_fops = { > static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock); > static LIST_HEAD(kcs_bmc_ipmi_instances); > >-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); >-int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) >+static int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_ipmi *priv; > int rc; >@@ -512,10 +511,8 @@ int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) > > return 0; > } >-EXPORT_SYMBOL(kcs_bmc_ipmi_attach_cdev); > >-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); >-int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) >+static int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) > { > struct kcs_bmc_ipmi *priv = NULL, *pos; > >@@ -541,7 +538,31 @@ int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) > > return 0; > } >-EXPORT_SYMBOL(kcs_bmc_ipmi_detach_cdev); >+ >+static const struct kcs_bmc_cdev_ops kcs_bmc_ipmi_cdev_ops = { >+ .add_device = kcs_bmc_ipmi_attach_cdev, >+ .remove_device = kcs_bmc_ipmi_detach_cdev, >+}; >+ >+static struct kcs_bmc_cdev kcs_bmc_ipmi_cdev = { >+ .ops = &kcs_bmc_ipmi_cdev_ops, >+}; >+ >+static int kcs_bmc_ipmi_init(void) >+{ >+ return kcs_bmc_register_cdev(&kcs_bmc_ipmi_cdev); >+} >+module_init(kcs_bmc_ipmi_init); >+ >+static void kcs_bmc_ipmi_exit(void) >+{ >+ int rc; >+ >+ rc = kcs_bmc_unregister_cdev(&kcs_bmc_ipmi_cdev); >+ if (rc) >+ pr_warn("Failed to remove KCS BMC client: %d", rc); >+} >+module_exit(kcs_bmc_ipmi_exit); > > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); >diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h >index 2dd710f4b4aa..d0a7404ff584 100644 >--- a/drivers/char/ipmi/kcs_bmc_client.h >+++ b/drivers/char/ipmi/kcs_bmc_client.h >@@ -10,6 +10,17 @@ > > #include "kcs_bmc.h" > >+struct kcs_bmc_cdev_ops { >+ int (*add_device)(struct kcs_bmc_device *kcs_bmc); >+ int (*remove_device)(struct kcs_bmc_device *kcs_bmc); >+}; >+ >+struct kcs_bmc_cdev { >+ struct list_head entry; >+ >+ const struct kcs_bmc_cdev_ops *ops; >+}; >+ > struct kcs_bmc_client_ops { > int (*event)(struct kcs_bmc_client *client); > }; >@@ -20,6 +31,9 @@ struct kcs_bmc_client { > struct kcs_bmc_device *dev; > }; > >+int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev); >+int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev); >+ > int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > >-- >2.27.0 >
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 <andrew@aj.id.au> > >--- > > 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 <linux/device.h> > >+#include <linux/list.h> > > #include <linux/module.h> > >+#include <linux/mutex.h> > > > > #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(&kcs_bmc->lock); > >+ kcs_bmc->client = NULL; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_add(&kcs_bmc->entry, &kcs_bmc_devices); > >+ list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); > >+ list_del(&kcs_bmc->entry); > >+ list_for_each_entry(cdev, &kcs_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(&kcs_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; > >+ int rc; > >+ > >+ mutex_lock(&kcs_bmc_lock); > >+ list_add(&cdev->entry, &kcs_bmc_cdevs); > >+ list_for_each_entry(kcs_bmc, &kcs_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(&kcs_bmc_lock); > >+ > >+ return 0; > > ...return value again here... As above. > > >+} > >+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(&kcs_bmc_lock); > >+ list_del(&cdev->entry); > >+ list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { > >+ rc = cdev->ops->remove_device(kcs_bmc); > >+ if (rc) > >+ dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d", > > s/add/remove/ Thanks. > > Might also want to differentiate the *_device() error messages from the > *_cdev() ones a bit more? I'll look into it. > > >+ kcs_bmc->channel, rc); > >+ } > >+ mutex_unlock(&kcs_bmc_lock); > >+ > >+ return rc; > > ...but this one is a bit incongruous, propagating the return value of > only the last ->remove_device() call. Hah. good catch. Andrew
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 <linux/device.h> +#include <linux/list.h> #include <linux/module.h> +#include <linux/mutex.h> #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(&kcs_bmc->lock); + kcs_bmc->client = NULL; + + mutex_lock(&kcs_bmc_lock); + list_add(&kcs_bmc->entry, &kcs_bmc_devices); + list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); + list_del(&kcs_bmc->entry); + list_for_each_entry(cdev, &kcs_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(&kcs_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(&kcs_bmc_lock); + list_add(&cdev->entry, &kcs_bmc_cdevs); + list_for_each_entry(kcs_bmc, &kcs_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(&kcs_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(&kcs_bmc_lock); + list_del(&cdev->entry); + list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) { + rc = cdev->ops->remove_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(&kcs_bmc_lock); + + return rc; +} +EXPORT_SYMBOL(kcs_bmc_unregister_cdev); + MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h index 3f266740c759..5deb9a0b8e60 100644 --- a/drivers/char/ipmi/kcs_bmc.h +++ b/drivers/char/ipmi/kcs_bmc.h @@ -42,8 +42,4 @@ struct kcs_bmc_device { spinlock_t lock; struct kcs_bmc_client *client; }; - -/* Temporary exports while refactoring */ -int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); -int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); #endif /* __KCS_BMC_H__ */ diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c index 58c42e76483d..df83d67851ac 100644 --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c @@ -469,8 +469,7 @@ static const struct file_operations kcs_bmc_ipmi_fops = { static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock); static LIST_HEAD(kcs_bmc_ipmi_instances); -int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc); -int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) +static int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) { struct kcs_bmc_ipmi *priv; int rc; @@ -512,10 +511,8 @@ int kcs_bmc_ipmi_attach_cdev(struct kcs_bmc_device *kcs_bmc) return 0; } -EXPORT_SYMBOL(kcs_bmc_ipmi_attach_cdev); -int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc); -int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) +static int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) { struct kcs_bmc_ipmi *priv = NULL, *pos; @@ -541,7 +538,31 @@ int kcs_bmc_ipmi_detach_cdev(struct kcs_bmc_device *kcs_bmc) return 0; } -EXPORT_SYMBOL(kcs_bmc_ipmi_detach_cdev); + +static const struct kcs_bmc_cdev_ops kcs_bmc_ipmi_cdev_ops = { + .add_device = kcs_bmc_ipmi_attach_cdev, + .remove_device = kcs_bmc_ipmi_detach_cdev, +}; + +static struct kcs_bmc_cdev kcs_bmc_ipmi_cdev = { + .ops = &kcs_bmc_ipmi_cdev_ops, +}; + +static int kcs_bmc_ipmi_init(void) +{ + return kcs_bmc_register_cdev(&kcs_bmc_ipmi_cdev); +} +module_init(kcs_bmc_ipmi_init); + +static void kcs_bmc_ipmi_exit(void) +{ + int rc; + + rc = kcs_bmc_unregister_cdev(&kcs_bmc_ipmi_cdev); + if (rc) + pr_warn("Failed to remove KCS BMC client: %d", rc); +} +module_exit(kcs_bmc_ipmi_exit); MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h index 2dd710f4b4aa..d0a7404ff584 100644 --- a/drivers/char/ipmi/kcs_bmc_client.h +++ b/drivers/char/ipmi/kcs_bmc_client.h @@ -10,6 +10,17 @@ #include "kcs_bmc.h" +struct kcs_bmc_cdev_ops { + int (*add_device)(struct kcs_bmc_device *kcs_bmc); + int (*remove_device)(struct kcs_bmc_device *kcs_bmc); +}; + +struct kcs_bmc_cdev { + struct list_head entry; + + const struct kcs_bmc_cdev_ops *ops; +}; + struct kcs_bmc_client_ops { int (*event)(struct kcs_bmc_client *client); }; @@ -20,6 +31,9 @@ struct kcs_bmc_client { struct kcs_bmc_device *dev; }; +int kcs_bmc_register_cdev(struct kcs_bmc_cdev *cdev); +int kcs_bmc_unregister_cdev(struct kcs_bmc_cdev *cdev); + int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client);
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 <andrew@aj.id.au> --- 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(-)