[PATCH 00/10] ipmi: kcs_bmc: Miscellaneous cleanups

[PATCH 00/10] ipmi: kcs_bmc: Miscellaneous cleanups

[Andrew Jeffery]

Hello,

A cleanup of the KCS subsystem was prompted after some concerns raised
by Jonathan on Konstantin's series implementing DSP0254[1] (the MCTP KCS
Transport Binding Specification):

https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/

[1]: https://www.dmtf.org/sites/default/files/standards/documents/DSP0254_1.0.0.pdf

The MCTP KCS patches are currently at v5:

https://lore.kernel.org/all/20231010122321.823-1-aladyshev22@gmail.com/

A v6 will be necessary to rework them in terms of the cleanup done here.
I've pushed a preview of that work here:

https://github.com/amboar/linux/compare/d2cc82b50335c8fcf83e1d8f396c8f8cf4333ac4...mctp-kcs

In addition to addressing some of the resource lifetime concerns I've
added kerneldoc for the subsystem in anticipation of Konstantin's series
moving the headers to include/linux/.

To get Konstantin's work merged I expect we'll have to either take these
KCS patches through netdev or the MCTP patches through the IPMI tree. We
should figure out which way we want to go, but netdev's not open right
now and so that's not a pressing concern.

Please review!

Thanks,

Andrew

Andrew Jeffery (10):
  ipmi: kcs_bmc: Update module description
  ipmi: kcs_bmc: Include spinlock.h
  ipmi: kcs_bmc: Make kcs_bmc_update_event_mask() static
  ipmi: kcs_bmc: Make remove_device() callback return void
  ipmi: kcs_bmc: Define client actions in terms of kcs_bmc_client
  ipmi: kcs_bmc: Integrate buffers into driver struct
  ipmi: kcs_bmc: Disassociate client from device lifetimes
  ipmi: kcs_bmc: Track clients in core
  ipmi: kcs_bmc: Add module_kcs_bmc_driver()
  ipmi: kcs_bmc: Add subsystem kerneldoc

 drivers/char/ipmi/kcs_bmc.c           | 160 +++++++++++---------
 drivers/char/ipmi/kcs_bmc.h           |  41 +++++
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 152 +++++++------------
 drivers/char/ipmi/kcs_bmc_client.h    | 206 +++++++++++++++++++++++---
 drivers/char/ipmi/kcs_bmc_device.h    |  44 +++++-
 drivers/char/ipmi/kcs_bmc_serio.c     |  84 ++++-------
 6 files changed, 448 insertions(+), 239 deletions(-)

-- 
2.39.2

[PATCH 01/10] ipmi: kcs_bmc: Update module description

[Andrew Jeffery]

KCS devices are often used for IPMI, but they're not constrained to it.
Update the subsystem module description to reflect its more general
capabilities.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 8b1161d5194a..a429d9f8a7bf 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -187,4 +187,4 @@ EXPORT_SYMBOL(kcs_bmc_update_event_mask);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
 MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
-MODULE_DESCRIPTION("KCS BMC to handle the IPMI request from system software");
+MODULE_DESCRIPTION("Subsystem for BMCs to communicate via KCS devices");
-- 
2.39.2

[PATCH 02/10] ipmi: kcs_bmc: Include spinlock.h

[Andrew Jeffery]

struct kcs_bmc_device defines a spinlock member but the header in which
it is defined failed to include the spinlock header. In the spirit of
include-what-you-use, do what's necessary.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index fa408b802c79..880d835fb90c 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -7,6 +7,7 @@
 #define __KCS_BMC_H__
 
 #include <linux/list.h>
+#include <linux/spinlock.h>
 
 #define KCS_BMC_EVENT_TYPE_OBE	BIT(0)
 #define KCS_BMC_EVENT_TYPE_IBF	BIT(1)
-- 
2.39.2

[PATCH 03/10] ipmi: kcs_bmc: Make kcs_bmc_update_event_mask() static

[Andrew Jeffery]

There were no users outside the subsystem core, so let's not expose it.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.c        | 11 +++++------
 drivers/char/ipmi/kcs_bmc_client.h |  2 --
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index a429d9f8a7bf..1a827db8a465 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -68,6 +68,11 @@ irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
 }
 EXPORT_SYMBOL(kcs_bmc_handle_event);
 
+static void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events)
+{
+	kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events);
+}
+
 int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
 {
 	int rc;
@@ -178,12 +183,6 @@ void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
 }
 EXPORT_SYMBOL(kcs_bmc_unregister_driver);
 
-void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events)
-{
-	kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events);
-}
-EXPORT_SYMBOL(kcs_bmc_update_event_mask);
-
 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_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index 6fdcde0a7169..814ad8e052ef 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -35,8 +35,6 @@ void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv);
 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);
 
-void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events);
-
 u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc);
 void kcs_bmc_write_data(struct kcs_bmc_device *kcs_bmc, u8 data);
 u8 kcs_bmc_read_status(struct kcs_bmc_device *kcs_bmc);
-- 
2.39.2

[PATCH 04/10] ipmi: kcs_bmc: Make remove_device() callback return void

[Andrew Jeffery]

Don't pretend there's a valid failure path when there's not.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.c           | 12 ++----------
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c |  6 ++----
 drivers/char/ipmi/kcs_bmc_client.h    |  2 +-
 drivers/char/ipmi/kcs_bmc_serio.c     |  6 ++----
 4 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 1a827db8a465..5a3f199241d2 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -135,15 +135,11 @@ EXPORT_SYMBOL(kcs_bmc_add_device);
 void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
 {
 	struct kcs_bmc_driver *drv;
-	int rc;
 
 	mutex_lock(&kcs_bmc_lock);
 	list_del(&kcs_bmc->entry);
 	list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
-		rc = drv->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);
+		drv->ops->remove_device(kcs_bmc);
 	}
 	mutex_unlock(&kcs_bmc_lock);
 }
@@ -169,15 +165,11 @@ EXPORT_SYMBOL(kcs_bmc_register_driver);
 void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
 {
 	struct kcs_bmc_device *kcs_bmc;
-	int rc;
 
 	mutex_lock(&kcs_bmc_lock);
 	list_del(&drv->entry);
 	list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
-		rc = drv->ops->remove_device(kcs_bmc);
-		if (rc)
-			dev_err(kcs_bmc->dev, "Failed to remove driver for KCS channel %d: %d",
-				kcs_bmc->channel, rc);
+		drv->ops->remove_device(kcs_bmc);
 	}
 	mutex_unlock(&kcs_bmc_lock);
 }
diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index cf670e891966..0552a07d6775 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -512,7 +512,7 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 	return 0;
 }
 
-static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 {
 	struct kcs_bmc_ipmi *priv = NULL, *pos;
 
@@ -527,7 +527,7 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
 
 	if (!priv)
-		return -ENODEV;
+		return;
 
 	misc_deregister(&priv->miscdev);
 	kcs_bmc_disable_device(priv->client.dev, &priv->client);
@@ -535,8 +535,6 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 	devm_kfree(kcs_bmc->dev, priv->data_out);
 	devm_kfree(kcs_bmc->dev, priv->data_in);
 	devm_kfree(kcs_bmc->dev, priv);
-
-	return 0;
 }
 
 static const struct kcs_bmc_driver_ops kcs_bmc_ipmi_driver_ops = {
diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index 814ad8e052ef..1c0df184860d 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -10,7 +10,7 @@
 
 struct kcs_bmc_driver_ops {
 	int (*add_device)(struct kcs_bmc_device *kcs_bmc);
-	int (*remove_device)(struct kcs_bmc_device *kcs_bmc);
+	void (*remove_device)(struct kcs_bmc_device *kcs_bmc);
 };
 
 struct kcs_bmc_driver {
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 1793358be782..0320ea974e03 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -103,7 +103,7 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 	return 0;
 }
 
-static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
 {
 	struct kcs_bmc_serio *priv = NULL, *pos;
 
@@ -118,7 +118,7 @@ static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
 	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
 
 	if (!priv)
-		return -ENODEV;
+		return;
 
 	/* kfree()s priv->port via put_device() */
 	serio_unregister_port(priv->port);
@@ -127,8 +127,6 @@ static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
 	kcs_bmc_disable_device(kcs_bmc, &priv->client);
 
 	devm_kfree(priv->client.dev->dev, priv);
-
-	return 0;
 }
 
 static const struct kcs_bmc_driver_ops kcs_bmc_serio_driver_ops = {
-- 
2.39.2

[PATCH 05/10] ipmi: kcs_bmc: Define client actions in terms of kcs_bmc_client

[Andrew Jeffery]

Operations such as reading and writing from hardware and updating the
events of interest are operations in which the client is interested, but
are applied to the device. Strengthen the concept of the client in the
subsystem and clean up some call-sites by translating between the client
and device types in the core of the KCS subsystem.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.c           | 67 ++++++++++++++++++---------
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 50 ++++++++++----------
 drivers/char/ipmi/kcs_bmc_client.h    | 15 +++---
 drivers/char/ipmi/kcs_bmc_serio.c     | 10 ++--
 4 files changed, 81 insertions(+), 61 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 5a3f199241d2..d70e503041bd 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -22,33 +22,53 @@ static LIST_HEAD(kcs_bmc_drivers);
 
 /* Consumer data access */
 
-u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_client_validate(struct kcs_bmc_client *client)
 {
-	return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
+	WARN_ONCE(client != READ_ONCE(client->dev->client), "KCS client confusion detected");
+}
+
+u8 kcs_bmc_read_data(struct kcs_bmc_client *client)
+{
+	struct kcs_bmc_device *dev = client->dev;
+
+	kcs_bmc_client_validate(client);
+	return dev->ops->io_inputb(dev, dev->ioreg.idr);
 }
 EXPORT_SYMBOL(kcs_bmc_read_data);
 
-void kcs_bmc_write_data(struct kcs_bmc_device *kcs_bmc, u8 data)
+void kcs_bmc_write_data(struct kcs_bmc_client *client, u8 data)
 {
-	kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.odr, data);
+	struct kcs_bmc_device *dev = client->dev;
+
+	kcs_bmc_client_validate(client);
+	dev->ops->io_outputb(dev, dev->ioreg.odr, data);
 }
 EXPORT_SYMBOL(kcs_bmc_write_data);
 
-u8 kcs_bmc_read_status(struct kcs_bmc_device *kcs_bmc)
+u8 kcs_bmc_read_status(struct kcs_bmc_client *client)
 {
-	return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.str);
+	struct kcs_bmc_device *dev = client->dev;
+
+	kcs_bmc_client_validate(client);
+	return dev->ops->io_inputb(dev, dev->ioreg.str);
 }
 EXPORT_SYMBOL(kcs_bmc_read_status);
 
-void kcs_bmc_write_status(struct kcs_bmc_device *kcs_bmc, u8 data)
+void kcs_bmc_write_status(struct kcs_bmc_client *client, u8 data)
 {
-	kcs_bmc->ops->io_outputb(kcs_bmc, kcs_bmc->ioreg.str, data);
+	struct kcs_bmc_device *dev = client->dev;
+
+	kcs_bmc_client_validate(client);
+	dev->ops->io_outputb(dev, dev->ioreg.str, data);
 }
 EXPORT_SYMBOL(kcs_bmc_write_status);
 
-void kcs_bmc_update_status(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 val)
+void kcs_bmc_update_status(struct kcs_bmc_client *client, u8 mask, u8 val)
 {
-	kcs_bmc->ops->io_updateb(kcs_bmc, kcs_bmc->ioreg.str, mask, val);
+	struct kcs_bmc_device *dev = client->dev;
+
+	kcs_bmc_client_validate(client);
+	dev->ops->io_updateb(dev, dev->ioreg.str, mask, val);
 }
 EXPORT_SYMBOL(kcs_bmc_update_status);
 
@@ -73,36 +93,39 @@ static void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u
 	kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events);
 }
 
-int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
+int kcs_bmc_enable_device(struct kcs_bmc_client *client)
 {
+	struct kcs_bmc_device *dev = client->dev;
 	int rc;
 
-	spin_lock_irq(&kcs_bmc->lock);
-	if (kcs_bmc->client) {
+	spin_lock_irq(&dev->lock);
+	if (dev->client) {
 		rc = -EBUSY;
 	} else {
 		u8 mask = KCS_BMC_EVENT_TYPE_IBF;
 
-		kcs_bmc->client = client;
-		kcs_bmc_update_event_mask(kcs_bmc, mask, mask);
+		dev->client = client;
+		kcs_bmc_update_event_mask(dev, mask, mask);
 		rc = 0;
 	}
-	spin_unlock_irq(&kcs_bmc->lock);
+	spin_unlock_irq(&dev->lock);
 
 	return rc;
 }
 EXPORT_SYMBOL(kcs_bmc_enable_device);
 
-void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
+void kcs_bmc_disable_device(struct kcs_bmc_client *client)
 {
-	spin_lock_irq(&kcs_bmc->lock);
-	if (client == kcs_bmc->client) {
+	struct kcs_bmc_device *dev = client->dev;
+
+	spin_lock_irq(&dev->lock);
+	if (client == dev->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;
+		kcs_bmc_update_event_mask(dev, mask, 0);
+		dev->client = NULL;
 	}
-	spin_unlock_irq(&kcs_bmc->lock);
+	spin_unlock_irq(&dev->lock);
 }
 EXPORT_SYMBOL(kcs_bmc_disable_device);
 
diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 0552a07d6775..712a80c27060 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -121,14 +121,14 @@ enum kcs_states {
 
 static inline void set_state(struct kcs_bmc_ipmi *priv, u8 state)
 {
-	kcs_bmc_update_status(priv->client.dev, KCS_STATUS_STATE_MASK, KCS_STATUS_STATE(state));
+	kcs_bmc_update_status(&priv->client, KCS_STATUS_STATE_MASK, KCS_STATUS_STATE(state));
 }
 
 static void kcs_bmc_ipmi_force_abort(struct kcs_bmc_ipmi *priv)
 {
 	set_state(priv, ERROR_STATE);
-	kcs_bmc_read_data(priv->client.dev);
-	kcs_bmc_write_data(priv->client.dev, KCS_ZERO_DATA);
+	kcs_bmc_read_data(&priv->client);
+	kcs_bmc_write_data(&priv->client, KCS_ZERO_DATA);
 
 	priv->phase = KCS_PHASE_ERROR;
 	priv->data_in_avail = false;
@@ -137,11 +137,9 @@ static void kcs_bmc_ipmi_force_abort(struct kcs_bmc_ipmi *priv)
 
 static void kcs_bmc_ipmi_handle_data(struct kcs_bmc_ipmi *priv)
 {
-	struct kcs_bmc_device *dev;
+	struct kcs_bmc_client *client = &priv->client;
 	u8 data;
 
-	dev = priv->client.dev;
-
 	switch (priv->phase) {
 	case KCS_PHASE_WRITE_START:
 		priv->phase = KCS_PHASE_WRITE_DATA;
@@ -150,8 +148,8 @@ static void kcs_bmc_ipmi_handle_data(struct kcs_bmc_ipmi *priv)
 	case KCS_PHASE_WRITE_DATA:
 		if (priv->data_in_idx < KCS_MSG_BUFSIZ) {
 			set_state(priv, WRITE_STATE);
-			kcs_bmc_write_data(dev, KCS_ZERO_DATA);
-			priv->data_in[priv->data_in_idx++] = kcs_bmc_read_data(dev);
+			kcs_bmc_write_data(client, KCS_ZERO_DATA);
+			priv->data_in[priv->data_in_idx++] = kcs_bmc_read_data(client);
 		} else {
 			kcs_bmc_ipmi_force_abort(priv);
 			priv->error = KCS_LENGTH_ERROR;
@@ -161,7 +159,7 @@ static void kcs_bmc_ipmi_handle_data(struct kcs_bmc_ipmi *priv)
 	case KCS_PHASE_WRITE_END_CMD:
 		if (priv->data_in_idx < KCS_MSG_BUFSIZ) {
 			set_state(priv, READ_STATE);
-			priv->data_in[priv->data_in_idx++] = kcs_bmc_read_data(dev);
+			priv->data_in[priv->data_in_idx++] = kcs_bmc_read_data(client);
 			priv->phase = KCS_PHASE_WRITE_DONE;
 			priv->data_in_avail = true;
 			wake_up_interruptible(&priv->queue);
@@ -175,33 +173,33 @@ static void kcs_bmc_ipmi_handle_data(struct kcs_bmc_ipmi *priv)
 		if (priv->data_out_idx == priv->data_out_len)
 			set_state(priv, IDLE_STATE);
 
-		data = kcs_bmc_read_data(dev);
+		data = kcs_bmc_read_data(client);
 		if (data != KCS_CMD_READ_BYTE) {
 			set_state(priv, ERROR_STATE);
-			kcs_bmc_write_data(dev, KCS_ZERO_DATA);
+			kcs_bmc_write_data(client, KCS_ZERO_DATA);
 			break;
 		}
 
 		if (priv->data_out_idx == priv->data_out_len) {
-			kcs_bmc_write_data(dev, KCS_ZERO_DATA);
+			kcs_bmc_write_data(client, KCS_ZERO_DATA);
 			priv->phase = KCS_PHASE_IDLE;
 			break;
 		}
 
-		kcs_bmc_write_data(dev, priv->data_out[priv->data_out_idx++]);
+		kcs_bmc_write_data(client, priv->data_out[priv->data_out_idx++]);
 		break;
 
 	case KCS_PHASE_ABORT_ERROR1:
 		set_state(priv, READ_STATE);
-		kcs_bmc_read_data(dev);
-		kcs_bmc_write_data(dev, priv->error);
+		kcs_bmc_read_data(client);
+		kcs_bmc_write_data(client, priv->error);
 		priv->phase = KCS_PHASE_ABORT_ERROR2;
 		break;
 
 	case KCS_PHASE_ABORT_ERROR2:
 		set_state(priv, IDLE_STATE);
-		kcs_bmc_read_data(dev);
-		kcs_bmc_write_data(dev, KCS_ZERO_DATA);
+		kcs_bmc_read_data(client);
+		kcs_bmc_write_data(client, KCS_ZERO_DATA);
 		priv->phase = KCS_PHASE_IDLE;
 		break;
 
@@ -216,9 +214,9 @@ static void kcs_bmc_ipmi_handle_cmd(struct kcs_bmc_ipmi *priv)
 	u8 cmd;
 
 	set_state(priv, WRITE_STATE);
-	kcs_bmc_write_data(priv->client.dev, KCS_ZERO_DATA);
+	kcs_bmc_write_data(&priv->client, KCS_ZERO_DATA);
 
-	cmd = kcs_bmc_read_data(priv->client.dev);
+	cmd = kcs_bmc_read_data(&priv->client);
 	switch (cmd) {
 	case KCS_CMD_WRITE_START:
 		priv->phase = KCS_PHASE_WRITE_START;
@@ -269,7 +267,7 @@ static irqreturn_t kcs_bmc_ipmi_event(struct kcs_bmc_client *client)
 
 	spin_lock(&priv->lock);
 
-	status = kcs_bmc_read_status(client->dev);
+	status = kcs_bmc_read_status(client);
 	if (status & KCS_STATUS_IBF) {
 		if (status & KCS_STATUS_CMD_DAT)
 			kcs_bmc_ipmi_handle_cmd(priv);
@@ -299,7 +297,7 @@ static int kcs_bmc_ipmi_open(struct inode *inode, struct file *filp)
 {
 	struct kcs_bmc_ipmi *priv = to_kcs_bmc(filp);
 
-	return kcs_bmc_enable_device(priv->client.dev, &priv->client);
+	return kcs_bmc_enable_device(&priv->client);
 }
 
 static __poll_t kcs_bmc_ipmi_poll(struct file *filp, poll_table *wait)
@@ -402,7 +400,7 @@ static ssize_t kcs_bmc_ipmi_write(struct file *filp, const char __user *buf,
 		priv->data_out_idx = 1;
 		priv->data_out_len = count;
 		memcpy(priv->data_out, priv->kbuffer, count);
-		kcs_bmc_write_data(priv->client.dev, priv->data_out[0]);
+		kcs_bmc_write_data(&priv->client, priv->data_out[0]);
 		ret = count;
 	} else {
 		ret = -EINVAL;
@@ -425,11 +423,11 @@ static long kcs_bmc_ipmi_ioctl(struct file *filp, unsigned int cmd,
 
 	switch (cmd) {
 	case IPMI_BMC_IOCTL_SET_SMS_ATN:
-		kcs_bmc_update_status(priv->client.dev, KCS_STATUS_SMS_ATN, KCS_STATUS_SMS_ATN);
+		kcs_bmc_update_status(&priv->client, KCS_STATUS_SMS_ATN, KCS_STATUS_SMS_ATN);
 		break;
 
 	case IPMI_BMC_IOCTL_CLEAR_SMS_ATN:
-		kcs_bmc_update_status(priv->client.dev, KCS_STATUS_SMS_ATN, 0);
+		kcs_bmc_update_status(&priv->client, KCS_STATUS_SMS_ATN, 0);
 		break;
 
 	case IPMI_BMC_IOCTL_FORCE_ABORT:
@@ -451,7 +449,7 @@ static int kcs_bmc_ipmi_release(struct inode *inode, struct file *filp)
 	struct kcs_bmc_ipmi *priv = to_kcs_bmc(filp);
 
 	kcs_bmc_ipmi_force_abort(priv);
-	kcs_bmc_disable_device(priv->client.dev, &priv->client);
+	kcs_bmc_disable_device(&priv->client);
 
 	return 0;
 }
@@ -530,7 +528,7 @@ static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 		return;
 
 	misc_deregister(&priv->miscdev);
-	kcs_bmc_disable_device(priv->client.dev, &priv->client);
+	kcs_bmc_disable_device(&priv->client);
 	devm_kfree(kcs_bmc->dev, priv->kbuffer);
 	devm_kfree(kcs_bmc->dev, priv->data_out);
 	devm_kfree(kcs_bmc->dev, priv->data_in);
diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index 1c0df184860d..1251e9669bcd 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -32,12 +32,11 @@ struct kcs_bmc_client {
 void kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
 void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv);
 
-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);
-
-u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc);
-void kcs_bmc_write_data(struct kcs_bmc_device *kcs_bmc, u8 data);
-u8 kcs_bmc_read_status(struct kcs_bmc_device *kcs_bmc);
-void kcs_bmc_write_status(struct kcs_bmc_device *kcs_bmc, u8 data);
-void kcs_bmc_update_status(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 val);
+int kcs_bmc_enable_device(struct kcs_bmc_client *client);
+void kcs_bmc_disable_device(struct kcs_bmc_client *client);
+u8 kcs_bmc_read_data(struct kcs_bmc_client *client);
+void kcs_bmc_write_data(struct kcs_bmc_client *client, u8 data);
+u8 kcs_bmc_read_status(struct kcs_bmc_client *client);
+void kcs_bmc_write_status(struct kcs_bmc_client *client, u8 data);
+void kcs_bmc_update_status(struct kcs_bmc_client *client, u8 mask, u8 val);
 #endif
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 0320ea974e03..713e847bbc4e 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -36,10 +36,10 @@ static irqreturn_t kcs_bmc_serio_event(struct kcs_bmc_client *client)
 
 	spin_lock(&priv->lock);
 
-	status = kcs_bmc_read_status(client->dev);
+	status = kcs_bmc_read_status(client);
 
 	if (status & KCS_BMC_STR_IBF)
-		handled = serio_interrupt(priv->port, kcs_bmc_read_data(client->dev), 0);
+		handled = serio_interrupt(priv->port, kcs_bmc_read_data(client), 0);
 
 	spin_unlock(&priv->lock);
 
@@ -54,14 +54,14 @@ static int kcs_bmc_serio_open(struct serio *port)
 {
 	struct kcs_bmc_serio *priv = port->port_data;
 
-	return kcs_bmc_enable_device(priv->client.dev, &priv->client);
+	return kcs_bmc_enable_device(&priv->client);
 }
 
 static void kcs_bmc_serio_close(struct serio *port)
 {
 	struct kcs_bmc_serio *priv = port->port_data;
 
-	kcs_bmc_disable_device(priv->client.dev, &priv->client);
+	kcs_bmc_disable_device(&priv->client);
 }
 
 static DEFINE_SPINLOCK(kcs_bmc_serio_instances_lock);
@@ -124,7 +124,7 @@ static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
 	serio_unregister_port(priv->port);
 
 	/* Ensure the IBF IRQ is disabled if we were the active client */
-	kcs_bmc_disable_device(kcs_bmc, &priv->client);
+	kcs_bmc_disable_device(&priv->client);
 
 	devm_kfree(priv->client.dev->dev, priv);
 }
-- 
2.39.2

[PATCH 06/10] ipmi: kcs_bmc: Integrate buffers into driver struct

[Andrew Jeffery]

Consolidate several necessary allocations into one to reduce the number
of possible error paths.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 712a80c27060..45ac930172ec 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -66,6 +66,10 @@ enum kcs_ipmi_errors {
 	KCS_UNSPECIFIED_ERROR       = 0xFF
 };
 
+#define DEVICE_NAME "ipmi-kcs"
+#define KCS_MSG_BUFSIZ    1000
+#define KCS_ZERO_DATA     0
+
 struct kcs_bmc_ipmi {
 	struct list_head entry;
 
@@ -79,24 +83,18 @@ struct kcs_bmc_ipmi {
 	wait_queue_head_t queue;
 	bool data_in_avail;
 	int  data_in_idx;
-	u8  *data_in;
+	u8   data_in[KCS_MSG_BUFSIZ];
 
 	int  data_out_idx;
 	int  data_out_len;
-	u8  *data_out;
+	u8   data_out[KCS_MSG_BUFSIZ];
 
 	struct mutex mutex;
-	u8 *kbuffer;
+	u8 kbuffer[KCS_MSG_BUFSIZ];
 
 	struct miscdevice miscdev;
 };
 
-#define DEVICE_NAME "ipmi-kcs"
-
-#define KCS_MSG_BUFSIZ    1000
-
-#define KCS_ZERO_DATA     0
-
 /* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
 #define KCS_STATUS_STATE(state) (state << 6)
 #define KCS_STATUS_STATE_MASK   GENMASK(7, 6)
@@ -478,19 +476,15 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 
 	spin_lock_init(&priv->lock);
 	mutex_init(&priv->mutex);
-
 	init_waitqueue_head(&priv->queue);
 
 	priv->client.dev = kcs_bmc;
 	priv->client.ops = &kcs_bmc_ipmi_client_ops;
-	priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	priv->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	priv->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
 
 	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
 	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
 					   kcs_bmc->channel);
-	if (!priv->data_in || !priv->data_out || !priv->kbuffer || !priv->miscdev.name)
+	if (!priv->miscdev.name)
 		return -EINVAL;
 
 	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
@@ -529,9 +523,6 @@ static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 
 	misc_deregister(&priv->miscdev);
 	kcs_bmc_disable_device(&priv->client);
-	devm_kfree(kcs_bmc->dev, priv->kbuffer);
-	devm_kfree(kcs_bmc->dev, priv->data_out);
-	devm_kfree(kcs_bmc->dev, priv->data_in);
 	devm_kfree(kcs_bmc->dev, priv);
 }
 
-- 
2.39.2

[PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device lifetimes

[Andrew Jeffery]

KCS client modules may be removed by actions unrelated to KCS devices.
As usual, removing a KCS client module requires unbinding all client
instances from the underlying devices to prevent further use of the code.

Previously, KCS client resource lifetimes were tied to the underlying
KCS device instance with the use of `devm_k[mz]alloc()` APIs. This
requires the use of `devm_free()` as a consequence. It's necessary to
scrutinise use of explicit `devm_free()`s because they generally
indicate there's a concerning corner-case in play, but that's not really
the case here. Switch to explicit kmalloc()/kfree() to align
expectations with the intent of the code.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 28 ++++++++++++++++++---------
 drivers/char/ipmi/kcs_bmc_serio.c     | 20 ++++++++++++-------
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 45ac930172ec..98f231f24c26 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -470,7 +470,7 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 	struct kcs_bmc_ipmi *priv;
 	int rc;
 
-	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -482,26 +482,35 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 	priv->client.ops = &kcs_bmc_ipmi_client_ops;
 
 	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
-	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
-					   kcs_bmc->channel);
-	if (!priv->miscdev.name)
-		return -EINVAL;
+	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
+	if (!priv->miscdev.name) {
+		rc = -ENOMEM;
+		goto cleanup_priv;
+	}
 
 	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
 
 	rc = misc_register(&priv->miscdev);
 	if (rc) {
-		dev_err(kcs_bmc->dev, "Unable to register device: %d\n", rc);
-		return rc;
+		pr_err("Unable to register device: %d\n", rc);
+		goto cleanup_miscdev_name;
 	}
 
 	spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
 	list_add(&priv->entry, &kcs_bmc_ipmi_instances);
 	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
 
-	dev_info(kcs_bmc->dev, "Initialised IPMI client for channel %d", kcs_bmc->channel);
+	pr_info("Initialised IPMI client for channel %d\n", kcs_bmc->channel);
 
 	return 0;
+
+cleanup_miscdev_name:
+	kfree(priv->miscdev.name);
+
+cleanup_priv:
+	kfree(priv);
+
+	return rc;
 }
 
 static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
@@ -523,7 +532,8 @@ static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 
 	misc_deregister(&priv->miscdev);
 	kcs_bmc_disable_device(&priv->client);
-	devm_kfree(kcs_bmc->dev, priv);
+	kfree(priv->miscdev.name);
+	kfree(priv);
 }
 
 static const struct kcs_bmc_driver_ops kcs_bmc_ipmi_driver_ops = {
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 713e847bbc4e..0a68c76da955 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -71,15 +71,18 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 {
 	struct kcs_bmc_serio *priv;
 	struct serio *port;
+	int rc;
 
-	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
 	port = kzalloc(sizeof(*port), GFP_KERNEL);
-	if (!port)
-		return -ENOMEM;
+	if (!port) {
+		rc = -ENOMEM;
+		goto cleanup_priv;
+	}
 
 	port->id.type = SERIO_8042;
 	port->open = kcs_bmc_serio_open;
@@ -98,9 +101,14 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 
 	serio_register_port(port);
 
-	dev_info(kcs_bmc->dev, "Initialised serio client for channel %d", kcs_bmc->channel);
+	pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
 
 	return 0;
+
+cleanup_priv:
+	kfree(priv);
+
+	return rc;
 }
 
 static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
@@ -122,11 +130,9 @@ static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
 
 	/* kfree()s priv->port via put_device() */
 	serio_unregister_port(priv->port);
-
 	/* Ensure the IBF IRQ is disabled if we were the active client */
 	kcs_bmc_disable_device(&priv->client);
-
-	devm_kfree(priv->client.dev->dev, priv);
+	kfree(priv);
 }
 
 static const struct kcs_bmc_driver_ops kcs_bmc_serio_driver_ops = {
-- 
2.39.2

[PATCH 08/10] ipmi: kcs_bmc: Track clients in core

[Andrew Jeffery]

I ran out of spoons before I could come up with a better client tracking
scheme back in the original refactoring series:

https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/

Jonathan prodded Konstantin about the issue in a review of Konstantin's
MCTP patches[1], prompting an attempt to clean it up.

[1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/

Prevent client modules from having to track their own instances by
requiring they return a pointer to a client object from their
add_device() implementation. We can then track this in the core, and
provide it as the argument to the remove_device() implementation to save
the client module from further work. The usual container_of() pattern
gets the client module access to its private data.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.c           | 68 ++++++++++++++++-----------
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 42 ++++-------------
 drivers/char/ipmi/kcs_bmc_client.h    | 35 ++++++++++----
 drivers/char/ipmi/kcs_bmc_device.h    |  4 +-
 drivers/char/ipmi/kcs_bmc_serio.c     | 43 +++++------------
 5 files changed, 88 insertions(+), 104 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index d70e503041bd..203cb73faa91 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -19,6 +19,7 @@
 static DEFINE_MUTEX(kcs_bmc_lock);
 static LIST_HEAD(kcs_bmc_devices);
 static LIST_HEAD(kcs_bmc_drivers);
+static LIST_HEAD(kcs_bmc_clients);
 
 /* Consumer data access */
 
@@ -129,25 +130,27 @@ void kcs_bmc_disable_device(struct kcs_bmc_client *client)
 }
 EXPORT_SYMBOL(kcs_bmc_disable_device);
 
-int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
+int kcs_bmc_add_device(struct kcs_bmc_device *dev)
 {
+	struct kcs_bmc_client *client;
 	struct kcs_bmc_driver *drv;
 	int error = 0;
-	int rc;
 
-	spin_lock_init(&kcs_bmc->lock);
-	kcs_bmc->client = NULL;
+	spin_lock_init(&dev->lock);
+	dev->client = NULL;
 
 	mutex_lock(&kcs_bmc_lock);
-	list_add(&kcs_bmc->entry, &kcs_bmc_devices);
+	list_add(&dev->entry, &kcs_bmc_devices);
 	list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
-		rc = drv->ops->add_device(kcs_bmc);
-		if (!rc)
-			continue;
-
-		dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d",
-			kcs_bmc->channel, rc);
-		error = rc;
+		client = drv->ops->add_device(drv, dev);
+		if (IS_ERR(client)) {
+			error = PTR_ERR(client);
+			dev_err(dev->dev,
+				"Failed to add chardev for KCS channel %d: %d",
+				dev->channel, error);
+			break;
+		}
+		list_add(&client->entry, &kcs_bmc_clients);
 	}
 	mutex_unlock(&kcs_bmc_lock);
 
@@ -155,31 +158,37 @@ int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
 }
 EXPORT_SYMBOL(kcs_bmc_add_device);
 
-void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
+void kcs_bmc_remove_device(struct kcs_bmc_device *dev)
 {
-	struct kcs_bmc_driver *drv;
+	struct kcs_bmc_client *curr, *tmp;
 
 	mutex_lock(&kcs_bmc_lock);
-	list_del(&kcs_bmc->entry);
-	list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
-		drv->ops->remove_device(kcs_bmc);
+	list_for_each_entry_safe(curr, tmp, &kcs_bmc_clients, entry) {
+		if (curr->dev == dev) {
+			list_del(&curr->entry);
+			curr->drv->ops->remove_device(curr);
+		}
 	}
+	list_del(&dev->entry);
 	mutex_unlock(&kcs_bmc_lock);
 }
 EXPORT_SYMBOL(kcs_bmc_remove_device);
 
 void kcs_bmc_register_driver(struct kcs_bmc_driver *drv)
 {
-	struct kcs_bmc_device *kcs_bmc;
-	int rc;
+	struct kcs_bmc_client *client;
+	struct kcs_bmc_device *dev;
 
 	mutex_lock(&kcs_bmc_lock);
 	list_add(&drv->entry, &kcs_bmc_drivers);
-	list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
-		rc = drv->ops->add_device(kcs_bmc);
-		if (rc)
-			dev_err(kcs_bmc->dev, "Failed to add driver for KCS channel %d: %d",
-				kcs_bmc->channel, rc);
+	list_for_each_entry(dev, &kcs_bmc_devices, entry) {
+		client = drv->ops->add_device(drv, dev);
+		if (IS_ERR(client)) {
+			dev_err(dev->dev, "Failed to add driver for KCS channel %d: %ld",
+				dev->channel, PTR_ERR(client));
+			continue;
+		}
+		list_add(&client->entry, &kcs_bmc_clients);
 	}
 	mutex_unlock(&kcs_bmc_lock);
 }
@@ -187,13 +196,16 @@ EXPORT_SYMBOL(kcs_bmc_register_driver);
 
 void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
 {
-	struct kcs_bmc_device *kcs_bmc;
+	struct kcs_bmc_client *curr, *tmp;
 
 	mutex_lock(&kcs_bmc_lock);
-	list_del(&drv->entry);
-	list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
-		drv->ops->remove_device(kcs_bmc);
+	list_for_each_entry_safe(curr, tmp, &kcs_bmc_clients, entry) {
+		if (curr->drv == drv) {
+			list_del(&curr->entry);
+			drv->ops->remove_device(curr);
+		}
 	}
+	list_del(&drv->entry);
 	mutex_unlock(&kcs_bmc_lock);
 }
 EXPORT_SYMBOL(kcs_bmc_unregister_driver);
diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 98f231f24c26..9fca31f8c7c2 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -71,8 +71,6 @@ enum kcs_ipmi_errors {
 #define KCS_ZERO_DATA     0
 
 struct kcs_bmc_ipmi {
-	struct list_head entry;
-
 	struct kcs_bmc_client client;
 
 	spinlock_t lock;
@@ -462,27 +460,24 @@ static const struct file_operations kcs_bmc_ipmi_fops = {
 	.unlocked_ioctl = kcs_bmc_ipmi_ioctl,
 };
 
-static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock);
-static LIST_HEAD(kcs_bmc_ipmi_instances);
-
-static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
+static struct kcs_bmc_client *
+kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
 {
 	struct kcs_bmc_ipmi *priv;
 	int rc;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-		return -ENOMEM;
+		return ERR_PTR(ENOMEM);
 
 	spin_lock_init(&priv->lock);
 	mutex_init(&priv->mutex);
 	init_waitqueue_head(&priv->queue);
 
-	priv->client.dev = kcs_bmc;
-	priv->client.ops = &kcs_bmc_ipmi_client_ops;
+	kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev);
 
 	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
-	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
+	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, dev->channel);
 	if (!priv->miscdev.name) {
 		rc = -ENOMEM;
 		goto cleanup_priv;
@@ -496,13 +491,9 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 		goto cleanup_miscdev_name;
 	}
 
-	spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
-	list_add(&priv->entry, &kcs_bmc_ipmi_instances);
-	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
+	pr_info("Initialised IPMI client for channel %d\n", dev->channel);
 
-	pr_info("Initialised IPMI client for channel %d\n", kcs_bmc->channel);
-
-	return 0;
+	return &priv->client;
 
 cleanup_miscdev_name:
 	kfree(priv->miscdev.name);
@@ -510,25 +501,12 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 cleanup_priv:
 	kfree(priv);
 
-	return rc;
+	return ERR_PTR(rc);
 }
 
-static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_client *client)
 {
-	struct kcs_bmc_ipmi *priv = NULL, *pos;
-
-	spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
-	list_for_each_entry(pos, &kcs_bmc_ipmi_instances, entry) {
-		if (pos->client.dev == kcs_bmc) {
-			priv = pos;
-			list_del(&pos->entry);
-			break;
-		}
-	}
-	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
-
-	if (!priv)
-		return;
+	struct kcs_bmc_ipmi *priv = client_to_kcs_bmc_ipmi(client);
 
 	misc_deregister(&priv->miscdev);
 	kcs_bmc_disable_device(&priv->client);
diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index 1251e9669bcd..1c6c812d6edc 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -8,16 +8,7 @@
 
 #include "kcs_bmc.h"
 
-struct kcs_bmc_driver_ops {
-	int (*add_device)(struct kcs_bmc_device *kcs_bmc);
-	void (*remove_device)(struct kcs_bmc_device *kcs_bmc);
-};
-
-struct kcs_bmc_driver {
-	struct list_head entry;
-
-	const struct kcs_bmc_driver_ops *ops;
-};
+struct kcs_bmc_driver;
 
 struct kcs_bmc_client_ops {
 	irqreturn_t (*event)(struct kcs_bmc_client *client);
@@ -26,7 +17,31 @@ struct kcs_bmc_client_ops {
 struct kcs_bmc_client {
 	const struct kcs_bmc_client_ops *ops;
 
+	struct kcs_bmc_driver *drv;
 	struct kcs_bmc_device *dev;
+	struct list_head entry;
+};
+
+struct kcs_bmc_driver_ops {
+	struct kcs_bmc_client *(*add_device)(struct kcs_bmc_driver *drv,
+					     struct kcs_bmc_device *dev);
+	void (*remove_device)(struct kcs_bmc_client *client);
+};
+
+static inline void kcs_bmc_client_init(struct kcs_bmc_client *client,
+				       const struct kcs_bmc_client_ops *ops,
+				       struct kcs_bmc_driver *drv,
+				       struct kcs_bmc_device *dev)
+{
+	client->ops = ops;
+	client->drv = drv;
+	client->dev = dev;
+}
+
+struct kcs_bmc_driver {
+	struct list_head entry;
+
+	const struct kcs_bmc_driver_ops *ops;
 };
 
 void kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h
index 17c572f25c54..ca2b5dc2031d 100644
--- a/drivers/char/ipmi/kcs_bmc_device.h
+++ b/drivers/char/ipmi/kcs_bmc_device.h
@@ -16,7 +16,7 @@ struct kcs_bmc_device_ops {
 };
 
 irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc);
-int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc);
-void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc);
+int kcs_bmc_add_device(struct kcs_bmc_device *dev);
+void kcs_bmc_remove_device(struct kcs_bmc_device *dev);
 
 #endif
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 0a68c76da955..3cfda39506f6 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -13,8 +13,6 @@
 #include "kcs_bmc_client.h"
 
 struct kcs_bmc_serio {
-	struct list_head entry;
-
 	struct kcs_bmc_client client;
 	struct serio *port;
 
@@ -64,10 +62,8 @@ static void kcs_bmc_serio_close(struct serio *port)
 	kcs_bmc_disable_device(&priv->client);
 }
 
-static DEFINE_SPINLOCK(kcs_bmc_serio_instances_lock);
-static LIST_HEAD(kcs_bmc_serio_instances);
-
-static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
+static struct kcs_bmc_client *
+kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
 {
 	struct kcs_bmc_serio *priv;
 	struct serio *port;
@@ -75,12 +71,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-		return -ENOMEM;
+		return ERR_PTR(ENOMEM);
 
 	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
 	port = kzalloc(sizeof(*port), GFP_KERNEL);
 	if (!port) {
-		rc = -ENOMEM;
+		rc = ENOMEM;
 		goto cleanup_priv;
 	}
 
@@ -88,45 +84,28 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
 	port->open = kcs_bmc_serio_open;
 	port->close = kcs_bmc_serio_close;
 	port->port_data = priv;
-	port->dev.parent = kcs_bmc->dev;
+	port->dev.parent = dev->dev;
 
 	spin_lock_init(&priv->lock);
 	priv->port = port;
-	priv->client.dev = kcs_bmc;
-	priv->client.ops = &kcs_bmc_serio_client_ops;
 
-	spin_lock_irq(&kcs_bmc_serio_instances_lock);
-	list_add(&priv->entry, &kcs_bmc_serio_instances);
-	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
+	kcs_bmc_client_init(&priv->client, &kcs_bmc_serio_client_ops, drv, dev);
 
 	serio_register_port(port);
 
-	pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
+	pr_info("Initialised serio client for channel %d\n", dev->channel);
 
-	return 0;
+	return &priv->client;
 
 cleanup_priv:
 	kfree(priv);
 
-	return rc;
+	return ERR_PTR(rc);
 }
 
-static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_serio_remove_device(struct kcs_bmc_client *client)
 {
-	struct kcs_bmc_serio *priv = NULL, *pos;
-
-	spin_lock_irq(&kcs_bmc_serio_instances_lock);
-	list_for_each_entry(pos, &kcs_bmc_serio_instances, entry) {
-		if (pos->client.dev == kcs_bmc) {
-			priv = pos;
-			list_del(&pos->entry);
-			break;
-		}
-	}
-	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
-
-	if (!priv)
-		return;
+	struct kcs_bmc_serio *priv = client_to_kcs_bmc_serio(client);
 
 	/* kfree()s priv->port via put_device() */
 	serio_unregister_port(priv->port);
-- 
2.39.2

[PATCH 09/10] ipmi: kcs_bmc: Add module_kcs_bmc_driver()

[Andrew Jeffery]

Remove some cruft in the client modules by adding the usual module macro
for the KCS subsystem.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.c           |  4 +++-
 drivers/char/ipmi/kcs_bmc.h           |  1 +
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 15 +--------------
 drivers/char/ipmi/kcs_bmc_client.h    |  7 ++++++-
 drivers/char/ipmi/kcs_bmc_serio.c     | 15 +--------------
 5 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index 203cb73faa91..c69eb671d9d0 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -174,7 +174,7 @@ void kcs_bmc_remove_device(struct kcs_bmc_device *dev)
 }
 EXPORT_SYMBOL(kcs_bmc_remove_device);
 
-void kcs_bmc_register_driver(struct kcs_bmc_driver *drv)
+int kcs_bmc_register_driver(struct kcs_bmc_driver *drv)
 {
 	struct kcs_bmc_client *client;
 	struct kcs_bmc_device *dev;
@@ -191,6 +191,8 @@ void kcs_bmc_register_driver(struct kcs_bmc_driver *drv)
 		list_add(&client->entry, &kcs_bmc_clients);
 	}
 	mutex_unlock(&kcs_bmc_lock);
+
+	return 0;
 }
 EXPORT_SYMBOL(kcs_bmc_register_driver);
 
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index 880d835fb90c..979d673f8f56 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -7,6 +7,7 @@
 #define __KCS_BMC_H__
 
 #include <linux/list.h>
+#include <linux/module.h>
 #include <linux/spinlock.h>
 
 #define KCS_BMC_EVENT_TYPE_OBE	BIT(0)
diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 9fca31f8c7c2..21d4c4c11e07 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -523,20 +523,7 @@ static struct kcs_bmc_driver kcs_bmc_ipmi_driver = {
 	.ops = &kcs_bmc_ipmi_driver_ops,
 };
 
-static int __init kcs_bmc_ipmi_init(void)
-{
-	kcs_bmc_register_driver(&kcs_bmc_ipmi_driver);
-
-	return 0;
-}
-module_init(kcs_bmc_ipmi_init);
-
-static void __exit kcs_bmc_ipmi_exit(void)
-{
-	kcs_bmc_unregister_driver(&kcs_bmc_ipmi_driver);
-}
-module_exit(kcs_bmc_ipmi_exit);
-
+module_kcs_bmc_driver(kcs_bmc_ipmi_driver);
 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_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index 1c6c812d6edc..afc9e71c9fc0 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -5,6 +5,7 @@
 #define __KCS_BMC_CONSUMER_H__
 
 #include <linux/irqreturn.h>
+#include <linux/module.h>
 
 #include "kcs_bmc.h"
 
@@ -44,9 +45,13 @@ struct kcs_bmc_driver {
 	const struct kcs_bmc_driver_ops *ops;
 };
 
-void kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
+int kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
 void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv);
 
+#define module_kcs_bmc_driver(__kcs_bmc_driver) \
+	module_driver(__kcs_bmc_driver, kcs_bmc_register_driver, \
+		kcs_bmc_unregister_driver)
+
 int kcs_bmc_enable_device(struct kcs_bmc_client *client);
 void kcs_bmc_disable_device(struct kcs_bmc_client *client);
 u8 kcs_bmc_read_data(struct kcs_bmc_client *client);
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 3cfda39506f6..8bb598c2aa38 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -123,20 +123,7 @@ static struct kcs_bmc_driver kcs_bmc_serio_driver = {
 	.ops = &kcs_bmc_serio_driver_ops,
 };
 
-static int __init kcs_bmc_serio_init(void)
-{
-	kcs_bmc_register_driver(&kcs_bmc_serio_driver);
-
-	return 0;
-}
-module_init(kcs_bmc_serio_init);
-
-static void __exit kcs_bmc_serio_exit(void)
-{
-	kcs_bmc_unregister_driver(&kcs_bmc_serio_driver);
-}
-module_exit(kcs_bmc_serio_exit);
-
+module_kcs_bmc_driver(kcs_bmc_serio_driver);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
 MODULE_DESCRIPTION("Adapter driver for serio access to BMC KCS devices");
-- 
2.39.2

[PATCH 10/10] ipmi: kcs_bmc: Add subsystem kerneldoc

[Andrew Jeffery]

Provide kerneldoc describing the relationships between and the
behaviours of the structures and functions of the KCS subsystem.

Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 drivers/char/ipmi/kcs_bmc.h        |  39 ++++++++
 drivers/char/ipmi/kcs_bmc_client.h | 151 +++++++++++++++++++++++++++++
 drivers/char/ipmi/kcs_bmc_device.h |  40 ++++++++
 3 files changed, 230 insertions(+)

diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index 979d673f8f56..69eee539ec50 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -10,6 +10,36 @@
 #include <linux/module.h>
 #include <linux/spinlock.h>
 
+/**
+ * DOC: KCS subsystem structure
+ *
+ * The KCS subsystem is split into three components:
+ *
+ * 1. &struct kcs_bmc_device
+ * 2. &struct kcs_bmc_driver
+ * 3. &struct kcs_bmc_client
+ *
+ * ``struct kcs_bmc_device`` (device) represents a driver instance for a
+ * particular KCS device. ``struct kcs_bmc_device``` abstracts away the device
+ * specifics allowing for device-independent implementation of protocols over
+ * KCS.
+ *
+ * ``struct kcs_bmc_driver`` (driver) represents an implementation of a KCS
+ * protocol. Implementations of a protocol either expose this behaviour out to
+ * userspace via a character device, or provide the glue into another kernel
+ * subsystem.
+ *
+ * ``struct kcs_bmc_client`` (client) associates a ``struct kcs_bmc_device``
+ * instance (``D``) with a &struct kcs_bmc_driver instance (``P``). An instance
+ * of each protocol implementation is associated with each device, yielding
+ * ``D*P`` client instances.
+ *
+ * A device may only have one active client at a time. A client becomes active
+ * on its associated device whenever userspace "opens" its interface in some
+ * fashion, for example, opening a character device. If the device associated
+ * with a client already has an active client then an error is propagated.
+ */
+
 #define KCS_BMC_EVENT_TYPE_OBE	BIT(0)
 #define KCS_BMC_EVENT_TYPE_IBF	BIT(1)
 
@@ -31,6 +61,15 @@ struct kcs_ioreg {
 struct kcs_bmc_device_ops;
 struct kcs_bmc_client;
 
+/**
+ * struct kcs_bmc_device - An abstract representation of a KCS device
+ * @entry: A list node for the KCS core to track KCS device instances
+ * @dev: The kernel device object for the KCS hardware
+ * @channel: The IPMI channel number for the KCS device
+ * @ops: A set of callbacks for providing abstract access to the KCS hardware
+ * @lock: Protects accesses to, and operations on &kcs_bmc_device.client
+ * @client: The client instance, if any, currently associated with the device
+ */
 struct kcs_bmc_device {
 	struct list_head entry;
 
diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index afc9e71c9fc0..8ccaaf10c10e 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -11,10 +11,24 @@
 
 struct kcs_bmc_driver;
 
+/**
+ * struct kcs_bmc_client_ops - Callbacks operating on a client instance
+ * @event: A notification to the client that the device has an active interrupt
+ */
 struct kcs_bmc_client_ops {
 	irqreturn_t (*event)(struct kcs_bmc_client *client);
 };
 
+/**
+ * struct kcs_bmc_client - Associates a KCS protocol implementation with a KCS device
+ * @ops: A set of callbacks for handling client events
+ * @drv: The KCS protocol implementation associated with the client instance
+ * @dev: The KCS device instance associated with the client instance
+ * @entry: A list node for the KCS core to track KCS client instances
+ *
+ * A ``struct kcs_bmc_client`` should be created for each device added via
+ * &kcs_bmc_driver_ops.add_device
+ */
 struct kcs_bmc_client {
 	const struct kcs_bmc_client_ops *ops;
 
@@ -23,12 +37,32 @@ struct kcs_bmc_client {
 	struct list_head entry;
 };
 
+/**
+ * struct kcs_bmc_driver_ops - KCS device lifecycle operations for a KCS protocol driver
+ * @add_device: A new device has appeared, a client instance is to be created
+ * @remove_device: A known device has been removed - a client instance should be destroyed
+ */
 struct kcs_bmc_driver_ops {
 	struct kcs_bmc_client *(*add_device)(struct kcs_bmc_driver *drv,
 					     struct kcs_bmc_device *dev);
 	void (*remove_device)(struct kcs_bmc_client *client);
 };
 
+/**
+ * kcs_bmc_client_init() - Initialise an instance of &struct kcs_bmc_client
+ * @client: The &struct kcs_bmc_client instance of interest, usually embedded in
+ *          an instance-private struct
+ * @ops: The &struct kcs_bmc_client_ops relevant for @client
+ * @drv: The &struct kcs_bmc_driver instance relevant for @client
+ * @dev: The &struct kcs_bmc_device instance relevant for @client
+ *
+ * It's intended that kcs_bmc_client_init() is invoked in the @add_device
+ * callback for the protocol driver where the protocol-private data is
+ * initialised for the new device instance. The function is provided to make
+ * sure that all required fields are initialised.
+ *
+ * Context: Any context
+ */
 static inline void kcs_bmc_client_init(struct kcs_bmc_client *client,
 				       const struct kcs_bmc_client_ops *ops,
 				       struct kcs_bmc_driver *drv,
@@ -39,24 +73,141 @@ static inline void kcs_bmc_client_init(struct kcs_bmc_client *client,
 	client->dev = dev;
 }
 
+/**
+ * struct kcs_bmc_driver - An implementation of a protocol run over a KCS channel
+ * @entry: A list node for the KCS core to track KCS protocol drivers
+ * @ops: A set of callbacks for handling device lifecycle events for the protocol driver
+ */
 struct kcs_bmc_driver {
 	struct list_head entry;
 
 	const struct kcs_bmc_driver_ops *ops;
 };
 
+/**
+ * kcs_bmc_register_driver() - Register a KCS protocol driver with the KCS subsystem
+ * @drv: The &struct kcs_bmc_driver instance to register
+ *
+ * Generally only invoked on module init.
+ *
+ * Context: Process context. Takes and releases the internal KCS subsystem mutex.
+ *
+ * Return: 0 on succes.
+ */
 int kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
+
+/**
+ * kcs_bmc_unregister_driver() - Unregister a KCS protocol driver with the KCS
+ *                               subsystem
+ * @drv: The &struct kcs_bmc_driver instance to unregister
+ *
+ * Generally only invoked on module exit.
+ *
+ * Context: Process context. Takes and releases the internal KCS subsystem mutex.
+ */
 void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv);
 
+/**
+ * module_kcs_bmc_driver() - Helper macro for registering a module KCS protocol driver
+ * @__kcs_bmc_driver: A ``struct kcs_bmc_driver``
+ *
+ * Helper macro for KCS protocol drivers which do not do anything special in
+ * module init/exit. This eliminates a lot of boilerplate. Each module may only
+ * use this macro once, and calling it replaces module_init() and module_exit()
+ */
 #define module_kcs_bmc_driver(__kcs_bmc_driver) \
 	module_driver(__kcs_bmc_driver, kcs_bmc_register_driver, \
 		kcs_bmc_unregister_driver)
 
+/**
+ * kcs_bmc_enable_device() - Prepare a KCS device for active use
+ * @client: The client whose KCS device should be enabled
+ *
+ * A client should enable its associated KCS device when the userspace
+ * interface for the client is "opened" in some fashion. Enabling the KCS device
+ * associates the client with the device and enables interrupts on the hardware.
+ *
+ * Context: Process context. Takes and releases ``client->dev->lock``
+ *
+ * Return: 0 on success, or -EBUSY if a client is already associated with the
+ *         device
+ */
 int kcs_bmc_enable_device(struct kcs_bmc_client *client);
+
+/**
+ * kcs_bmc_disable_device() - Remove a KCS device from active use
+ * @client: The client whose KCS device should be disabled
+ *
+ * A client should disable its associated KCS device when the userspace
+ * interface for the client is "closed" in some fashion. The client is
+ * disassociated from the device iff it was the active client. If the client is
+ * disassociated then interrupts are disabled on the hardware.
+ *
+ * Context: Process context. Takes and releases ``client->dev->lock``.
+ */
 void kcs_bmc_disable_device(struct kcs_bmc_client *client);
+
+/**
+ * kcs_bmc_read_data() - Read the Input Data Register (IDR) on a KCS device
+ * @client: The client whose device's IDR should be read
+ *
+ * Must only be called on a client that is currently active on its associated
+ * device.
+ *
+ * Context: Any context. Any spinlocks taken are also released.
+ *
+ * Return: The value of IDR
+ */
 u8 kcs_bmc_read_data(struct kcs_bmc_client *client);
+
+/**
+ * kcs_bmc_write_data() - Write the Output Data Register (ODR) on a KCS device
+ * @client: The client whose device's ODR should be written
+ * @data: The value to write to ODR
+ *
+ * Must only be called on a client that is currently active on its associated
+ * device.
+ *
+ * Context: Any context. Any spinlocks taken are also released.
+ */
 void kcs_bmc_write_data(struct kcs_bmc_client *client, u8 data);
+
+/**
+ * kcs_bmc_read_status() - Read the Status Register (STR) on a KCS device
+ * @client: The client whose device's STR should be read
+ *
+ * Must only be called on a client that is currently active on its associated
+ * device.
+ *
+ * Context: Any context. Any spinlocks taken are also released.
+ *
+ * Return: The value of STR
+ */
 u8 kcs_bmc_read_status(struct kcs_bmc_client *client);
+
+/**
+ * kcs_bmc_write_status() - Write the Status Register (STR) on a KCS device
+ * @client: The client whose device's STR should be written
+ * @data: The value to write to STR
+ *
+ * Must only be called on a client that is currently active on its associated
+ * device.
+ *
+ * Context: Any context. Any spinlocks taken are also released.
+ */
 void kcs_bmc_write_status(struct kcs_bmc_client *client, u8 data);
+
+/**
+ * kcs_bmc_update_status() - Update Status Register (STR) on a KCS device
+ * @client: The client whose device's STR should be updated
+ * @mask: A bit-mask defining the field in STR that should be updated
+ * @val: The new value of the field, specified in the position of the bit-mask
+ *       defined by @mask
+ *
+ * Must only be called on a client that is currently active on its associated
+ * device.
+ *
+ * Context: Any context. Any spinlocks taken are also released.
+ */
 void kcs_bmc_update_status(struct kcs_bmc_client *client, u8 mask, u8 val);
 #endif
diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h
index ca2b5dc2031d..b17171d1c981 100644
--- a/drivers/char/ipmi/kcs_bmc_device.h
+++ b/drivers/char/ipmi/kcs_bmc_device.h
@@ -8,6 +8,13 @@
 
 #include "kcs_bmc.h"
 
+/**
+ * struct kcs_bmc_device_ops - Callbacks operating on a KCS device
+ * @irq_mask_update: Update the set of events of interest
+ * @io_inputb: A callback to read the specified KCS register from hardware
+ * @io_outputb: A callback to write the specified KCS register to hardware
+ * @io_updateb: A callback to update a subfield of the specified KCS register
+ */
 struct kcs_bmc_device_ops {
 	void (*irq_mask_update)(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 enable);
 	u8 (*io_inputb)(struct kcs_bmc_device *kcs_bmc, u32 reg);
@@ -15,8 +22,41 @@ struct kcs_bmc_device_ops {
 	void (*io_updateb)(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 mask, u8 b);
 };
 
+/**
+ * kcs_bmc_handle_event() - Notify the active client of a hardware interrupt
+ * @kcs_bmc: The device instance whose interrupt was triggered
+ *
+ * Propagate a hardware interrupt as an event to the active client. The client's
+ * handler should take any necessary actions for the protocol it implements, but
+ * must read IDR to resolve the interrupt if the interrupt was generated by the
+ * KCS device.
+ *
+ * Context: Interrupt context. Takes and releases &kcs_bmc_device.lock.
+ *
+ * Return: An irqreturn_t value indicating whether the interrupt was handled.
+ */
 irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc);
+
+/**
+ * kcs_bmc_add_device() - Register a KCS device instance with the KCS subsystem
+ * @dev: The &struct kcs_bmc_device instance to register
+ *
+ * Should be called by the probe() implementation of the KCS hardware's driver.
+ *
+ * Context: Process context. Takes and releases the internal KCS subsystem mutex.
+ *
+ * Return: 0 on success, or a negative errno on failure.
+ */
 int kcs_bmc_add_device(struct kcs_bmc_device *dev);
+
+/**
+ * kcs_bmc_remove_device() - Unregister a KCS device instance with the KCS subsystem
+ * @dev: The &struct kcs_bmc_device instance to unregister
+ *
+ * Should be called by the remove() implementation of the KCS hardware's driver.
+ *
+ * Context: Process context. Takes and releases the internal KCS subsystem mutex.
+ */
 void kcs_bmc_remove_device(struct kcs_bmc_device *dev);
 
 #endif
-- 
2.39.2

Re: [PATCH 01/10] ipmi: kcs_bmc: Update module description

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:13 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> KCS devices are often used for IPMI, but they're not constrained to it.
> Update the subsystem module description to reflect its more general
> capabilities.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
LTGM, FWIW,

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/char/ipmi/kcs_bmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index 8b1161d5194a..a429d9f8a7bf 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -187,4 +187,4 @@ EXPORT_SYMBOL(kcs_bmc_update_event_mask);
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
>  MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
> -MODULE_DESCRIPTION("KCS BMC to handle the IPMI request from system software");
> +MODULE_DESCRIPTION("Subsystem for BMCs to communicate via KCS devices");

Re: [PATCH 02/10] ipmi: kcs_bmc: Include spinlock.h

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:14 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> struct kcs_bmc_device defines a spinlock member but the header in which
> it is defined failed to include the spinlock header. In the spirit of
> include-what-you-use, do what's necessary.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
This is fine, but whilst checking it I noticed there is no
forwards def of struct device or appropriate include.

Still that's unrelated
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/char/ipmi/kcs_bmc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
> index fa408b802c79..880d835fb90c 100644
> --- a/drivers/char/ipmi/kcs_bmc.h
> +++ b/drivers/char/ipmi/kcs_bmc.h
> @@ -7,6 +7,7 @@
>  #define __KCS_BMC_H__
>  
>  #include <linux/list.h>
> +#include <linux/spinlock.h>
>  
>  #define KCS_BMC_EVENT_TYPE_OBE	BIT(0)
>  #define KCS_BMC_EVENT_TYPE_IBF	BIT(1)

Re: [PATCH 03/10] ipmi: kcs_bmc: Make kcs_bmc_update_event_mask() static

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:15 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> There were no users outside the subsystem core, so let's not expose it.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Is it worth having the wrapper?

I guess all the other cases do have wrappers (even if that's because
they continue to be exported) so fair enough.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/char/ipmi/kcs_bmc.c        | 11 +++++------
>  drivers/char/ipmi/kcs_bmc_client.h |  2 --
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index a429d9f8a7bf..1a827db8a465 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -68,6 +68,11 @@ irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc)
>  }
>  EXPORT_SYMBOL(kcs_bmc_handle_event);
>  
> +static void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events)
> +{
> +	kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events);
> +}
> +
>  int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client)
>  {
>  	int rc;
> @@ -178,12 +183,6 @@ void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
>  }
>  EXPORT_SYMBOL(kcs_bmc_unregister_driver);
>  
> -void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events)
> -{
> -	kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events);
> -}
> -EXPORT_SYMBOL(kcs_bmc_update_event_mask);
> -
>  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_client.h b/drivers/char/ipmi/kcs_bmc_client.h
> index 6fdcde0a7169..814ad8e052ef 100644
> --- a/drivers/char/ipmi/kcs_bmc_client.h
> +++ b/drivers/char/ipmi/kcs_bmc_client.h
> @@ -35,8 +35,6 @@ void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv);
>  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);
>  
> -void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events);
> -
>  u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc);
>  void kcs_bmc_write_data(struct kcs_bmc_device *kcs_bmc, u8 data);
>  u8 kcs_bmc_read_status(struct kcs_bmc_device *kcs_bmc);

Re: [PATCH 04/10] ipmi: kcs_bmc: Make remove_device() callback return void

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:16 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> Don't pretend there's a valid failure path when there's not.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Whilst I agree returning an error code is pointless, it is perhaps
useful to make sure there is a dev_err() or similar in the paths
now that you've remove the one at the call site.

Minor point and up to you if you want to or not.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/char/ipmi/kcs_bmc.c           | 12 ++----------
>  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c |  6 ++----
>  drivers/char/ipmi/kcs_bmc_client.h    |  2 +-
>  drivers/char/ipmi/kcs_bmc_serio.c     |  6 ++----
>  4 files changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index 1a827db8a465..5a3f199241d2 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -135,15 +135,11 @@ EXPORT_SYMBOL(kcs_bmc_add_device);
>  void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
>  {
>  	struct kcs_bmc_driver *drv;
> -	int rc;
>  
>  	mutex_lock(&kcs_bmc_lock);
>  	list_del(&kcs_bmc->entry);
>  	list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
> -		rc = drv->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);
> +		drv->ops->remove_device(kcs_bmc);
>  	}
>  	mutex_unlock(&kcs_bmc_lock);
>  }
> @@ -169,15 +165,11 @@ EXPORT_SYMBOL(kcs_bmc_register_driver);
>  void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
>  {
>  	struct kcs_bmc_device *kcs_bmc;
> -	int rc;
>  
>  	mutex_lock(&kcs_bmc_lock);
>  	list_del(&drv->entry);
>  	list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
> -		rc = drv->ops->remove_device(kcs_bmc);
> -		if (rc)
> -			dev_err(kcs_bmc->dev, "Failed to remove driver for KCS channel %d: %d",
> -				kcs_bmc->channel, rc);
> +		drv->ops->remove_device(kcs_bmc);
>  	}
>  	mutex_unlock(&kcs_bmc_lock);
>  }
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index cf670e891966..0552a07d6775 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -512,7 +512,7 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
>  	return 0;
>  }
>  
> -static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
> +static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
>  {
>  	struct kcs_bmc_ipmi *priv = NULL, *pos;
>  
> @@ -527,7 +527,7 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
>  	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
>  
>  	if (!priv)
> -		return -ENODEV;
> +		return;
>  
>  	misc_deregister(&priv->miscdev);
>  	kcs_bmc_disable_device(priv->client.dev, &priv->client);
> @@ -535,8 +535,6 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
>  	devm_kfree(kcs_bmc->dev, priv->data_out);
>  	devm_kfree(kcs_bmc->dev, priv->data_in);
>  	devm_kfree(kcs_bmc->dev, priv);
> -
> -	return 0;
>  }
>  
>  static const struct kcs_bmc_driver_ops kcs_bmc_ipmi_driver_ops = {
> diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
> index 814ad8e052ef..1c0df184860d 100644
> --- a/drivers/char/ipmi/kcs_bmc_client.h
> +++ b/drivers/char/ipmi/kcs_bmc_client.h
> @@ -10,7 +10,7 @@
>  
>  struct kcs_bmc_driver_ops {
>  	int (*add_device)(struct kcs_bmc_device *kcs_bmc);
> -	int (*remove_device)(struct kcs_bmc_device *kcs_bmc);
> +	void (*remove_device)(struct kcs_bmc_device *kcs_bmc);
>  };
>  
>  struct kcs_bmc_driver {
> diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> index 1793358be782..0320ea974e03 100644
> --- a/drivers/char/ipmi/kcs_bmc_serio.c
> +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> @@ -103,7 +103,7 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
>  	return 0;
>  }
>  
> -static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
> +static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
>  {
>  	struct kcs_bmc_serio *priv = NULL, *pos;
>  
> @@ -118,7 +118,7 @@ static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
>  	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
>  
>  	if (!priv)
> -		return -ENODEV;
> +		return;
>  
>  	/* kfree()s priv->port via put_device() */
>  	serio_unregister_port(priv->port);
> @@ -127,8 +127,6 @@ static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
>  	kcs_bmc_disable_device(kcs_bmc, &priv->client);
>  
>  	devm_kfree(priv->client.dev->dev, priv);
> -
> -	return 0;
>  }
>  
>  static const struct kcs_bmc_driver_ops kcs_bmc_serio_driver_ops = {

Re: [PATCH 06/10] ipmi: kcs_bmc: Integrate buffers into driver struct

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:18 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> Consolidate several necessary allocations into one to reduce the number
> of possible error paths.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Gets rid of some of the devm_kfree() fun, so I'm in favor of the change :)

One trivial comment inline.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index 712a80c27060..45ac930172ec 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -66,6 +66,10 @@ enum kcs_ipmi_errors {
>  	KCS_UNSPECIFIED_ERROR       = 0xFF
>  };
>  
> +#define DEVICE_NAME "ipmi-kcs"
> +#define KCS_MSG_BUFSIZ    1000
> +#define KCS_ZERO_DATA     0
> +
>  struct kcs_bmc_ipmi {
>  	struct list_head entry;
>  
> @@ -79,24 +83,18 @@ struct kcs_bmc_ipmi {
>  	wait_queue_head_t queue;
>  	bool data_in_avail;
>  	int  data_in_idx;
> -	u8  *data_in;
> +	u8   data_in[KCS_MSG_BUFSIZ];
>  
>  	int  data_out_idx;
>  	int  data_out_len;
> -	u8  *data_out;
> +	u8   data_out[KCS_MSG_BUFSIZ];
>  
>  	struct mutex mutex;
> -	u8 *kbuffer;
> +	u8 kbuffer[KCS_MSG_BUFSIZ];
>  
>  	struct miscdevice miscdev;
>  };
>  
> -#define DEVICE_NAME "ipmi-kcs"
> -
> -#define KCS_MSG_BUFSIZ    1000
> -
> -#define KCS_ZERO_DATA     0
> -
>  /* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
>  #define KCS_STATUS_STATE(state) (state << 6)
>  #define KCS_STATUS_STATE_MASK   GENMASK(7, 6)
> @@ -478,19 +476,15 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	spin_lock_init(&priv->lock);
>  	mutex_init(&priv->mutex);
> -
Unrelated change...

>  	init_waitqueue_head(&priv->queue);
>  
>  	priv->client.dev = kcs_bmc;
>  	priv->client.ops = &kcs_bmc_ipmi_client_ops;
> -	priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	priv->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	priv->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>  
>  	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
>  	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
>  					   kcs_bmc->channel);
> -	if (!priv->data_in || !priv->data_out || !priv->kbuffer || !priv->miscdev.name)
> +	if (!priv->miscdev.name)
>  		return -EINVAL;
>  
>  	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
> @@ -529,9 +523,6 @@ static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	misc_deregister(&priv->miscdev);
>  	kcs_bmc_disable_device(&priv->client);
> -	devm_kfree(kcs_bmc->dev, priv->kbuffer);
> -	devm_kfree(kcs_bmc->dev, priv->data_out);
> -	devm_kfree(kcs_bmc->dev, priv->data_in);
>  	devm_kfree(kcs_bmc->dev, priv);
>  }
>  

Re: [PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device lifetimes

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:19 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> KCS client modules may be removed by actions unrelated to KCS devices.
> As usual, removing a KCS client module requires unbinding all client
> instances from the underlying devices to prevent further use of the code.
> 
> Previously, KCS client resource lifetimes were tied to the underlying
> KCS device instance with the use of `devm_k[mz]alloc()` APIs. This
> requires the use of `devm_free()` as a consequence. It's necessary to
> scrutinise use of explicit `devm_free()`s because they generally
> indicate there's a concerning corner-case in play, but that's not really
> the case here. Switch to explicit kmalloc()/kfree() to align
> expectations with the intent of the code.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Bit more unrelated white space stuff in here that ideally wouldn't be there.
Otherwise makes sense to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 28 ++++++++++++++++++---------
>  drivers/char/ipmi/kcs_bmc_serio.c     | 20 ++++++++++++-------
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index 45ac930172ec..98f231f24c26 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -470,7 +470,7 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
>  	struct kcs_bmc_ipmi *priv;
>  	int rc;
>  
> -	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> @@ -482,26 +482,35 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
>  	priv->client.ops = &kcs_bmc_ipmi_client_ops;
>  
>  	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
> -	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
> -					   kcs_bmc->channel);
> -	if (!priv->miscdev.name)
> -		return -EINVAL;
> +	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
> +	if (!priv->miscdev.name) {
> +		rc = -ENOMEM;
> +		goto cleanup_priv;
> +	}
>  
>  	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
>  
>  	rc = misc_register(&priv->miscdev);
>  	if (rc) {
> -		dev_err(kcs_bmc->dev, "Unable to register device: %d\n", rc);
> -		return rc;
> +		pr_err("Unable to register device: %d\n", rc);
> +		goto cleanup_miscdev_name;
>  	}
>  
>  	spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
>  	list_add(&priv->entry, &kcs_bmc_ipmi_instances);
>  	spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
>  
> -	dev_info(kcs_bmc->dev, "Initialised IPMI client for channel %d", kcs_bmc->channel);
> +	pr_info("Initialised IPMI client for channel %d\n", kcs_bmc->channel);
>  
>  	return 0;
> +
> +cleanup_miscdev_name:
> +	kfree(priv->miscdev.name);
> +
> +cleanup_priv:
> +	kfree(priv);
> +
> +	return rc;
>  }
>  
>  static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
> @@ -523,7 +532,8 @@ static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	misc_deregister(&priv->miscdev);
>  	kcs_bmc_disable_device(&priv->client);
> -	devm_kfree(kcs_bmc->dev, priv);
> +	kfree(priv->miscdev.name);
> +	kfree(priv);
>  }
>  
>  static const struct kcs_bmc_driver_ops kcs_bmc_ipmi_driver_ops = {
> diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> index 713e847bbc4e..0a68c76da955 100644
> --- a/drivers/char/ipmi/kcs_bmc_serio.c
> +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> @@ -71,15 +71,18 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
>  {
>  	struct kcs_bmc_serio *priv;
>  	struct serio *port;
> +	int rc;
>  
> -	priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
>  	port = kzalloc(sizeof(*port), GFP_KERNEL);
> -	if (!port)
> -		return -ENOMEM;
> +	if (!port) {
> +		rc = -ENOMEM;
> +		goto cleanup_priv;
> +	}
>  
>  	port->id.type = SERIO_8042;
>  	port->open = kcs_bmc_serio_open;
> @@ -98,9 +101,14 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	serio_register_port(port);
>  
> -	dev_info(kcs_bmc->dev, "Initialised serio client for channel %d", kcs_bmc->channel);
> +	pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
>  
>  	return 0;
> +
> +cleanup_priv:
> +	kfree(priv);
> +
> +	return rc;
>  }
>  
>  static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
> @@ -122,11 +130,9 @@ static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	/* kfree()s priv->port via put_device() */
>  	serio_unregister_port(priv->port);
> -
>  	/* Ensure the IBF IRQ is disabled if we were the active client */
>  	kcs_bmc_disable_device(&priv->client);
> -
> -	devm_kfree(priv->client.dev->dev, priv);
> +	kfree(priv);
>  }
>  
>  static const struct kcs_bmc_driver_ops kcs_bmc_serio_driver_ops = {

Re: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:20 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> I ran out of spoons before I could come up with a better client tracking
> scheme back in the original refactoring series:
> 
> https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/
> 
> Jonathan prodded Konstantin about the issue in a review of Konstantin's
> MCTP patches[1], prompting an attempt to clean it up.
> 
> [1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/
> 
> Prevent client modules from having to track their own instances by
> requiring they return a pointer to a client object from their
> add_device() implementation. We can then track this in the core, and
> provide it as the argument to the remove_device() implementation to save
> the client module from further work. The usual container_of() pattern
> gets the client module access to its private data.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Hi Andrew,

A few comments inline.
More generally, whilst this is definitely an improvement I'd have been tempted
to make more use of the linux device model for this with the clients added
as devices with a parent of the kcs_bmc_device.  That would then allow for
simple dependency tracking, binding of individual drivers and all that.

What you have here feels fine though and is a much less invasive change.

Jonathan


> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index 98f231f24c26..9fca31f8c7c2 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -71,8 +71,6 @@ enum kcs_ipmi_errors {



> +static struct kcs_bmc_client *
> +kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
>  {
>  	struct kcs_bmc_ipmi *priv;
>  	int rc;
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> -		return -ENOMEM;
> +		return ERR_PTR(ENOMEM);
As below. I thought it took negatives..
>  
>  	spin_lock_init(&priv->lock);
>  	mutex_init(&priv->mutex);
>  	init_waitqueue_head(&priv->queue);
>  
> -	priv->client.dev = kcs_bmc;
> -	priv->client.ops = &kcs_bmc_ipmi_client_ops;
> +	kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev);
>  
>  	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
> -	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
> +	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, dev->channel);
>  	if (!priv->miscdev.name) {
>  		rc = -ENOMEM;
ERR_PTR
>  		goto cleanup_priv;



...

> diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> index 0a68c76da955..3cfda39506f6 100644
> --- a/drivers/char/ipmi/kcs_bmc_serio.c
> +++ b/drivers/char/ipmi/kcs_bmc_serio.c

...


> +static struct kcs_bmc_client *
> +kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
>  {
>  	struct kcs_bmc_serio *priv;
>  	struct serio *port;
> @@ -75,12 +71,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> -		return -ENOMEM;
> +		return ERR_PTR(ENOMEM);
>  
>  	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
>  	port = kzalloc(sizeof(*port), GFP_KERNEL);
>  	if (!port) {
> -		rc = -ENOMEM;
> +		rc = ENOMEM;
Why positive?
Doesn't ERR_PTR() typically get passed negatives? 

>  		goto cleanup_priv;
>  	}
>  
> @@ -88,45 +84,28 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
>  	port->open = kcs_bmc_serio_open;
>  	port->close = kcs_bmc_serio_close;
>  	port->port_data = priv;
> -	port->dev.parent = kcs_bmc->dev;
> +	port->dev.parent = dev->dev;
>  
>  	spin_lock_init(&priv->lock);
>  	priv->port = port;
> -	priv->client.dev = kcs_bmc;
> -	priv->client.ops = &kcs_bmc_serio_client_ops;
>  
> -	spin_lock_irq(&kcs_bmc_serio_instances_lock);
> -	list_add(&priv->entry, &kcs_bmc_serio_instances);
> -	spin_unlock_irq(&kcs_bmc_serio_instances_lock);
> +	kcs_bmc_client_init(&priv->client, &kcs_bmc_serio_client_ops, drv, dev);
>  
>  	serio_register_port(port);
>  
> -	pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
> +	pr_info("Initialised serio client for channel %d\n", dev->channel);
>  
> -	return 0;
> +	return &priv->client;
>  
>  cleanup_priv:
>  	kfree(priv);
>  
> -	return rc;
> +	return ERR_PTR(rc);
>  }

Re: [PATCH 09/10] ipmi: kcs_bmc: Add module_kcs_bmc_driver()

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:21 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> Remove some cruft in the client modules by adding the usual module macro
> for the KCS subsystem.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Good stuff
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 10/10] ipmi: kcs_bmc: Add subsystem kerneldoc

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:22 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> Provide kerneldoc describing the relationships between and the
> behaviours of the structures and functions of the KCS subsystem.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Seems reasonable but I've only a superficial idea of how this all fits
together so no tag from me.

There is the fun question of whether function documentation should be
next to the implementation or in the header.  As long as it's
consistent in a given subsystem I don't personally thing it matters
that much.

Jonathan

Re: [PATCH 05/10] ipmi: kcs_bmc: Define client actions in terms of kcs_bmc_client

[Jonathan Cameron]

On Fri,  3 Nov 2023 16:45:17 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> Operations such as reading and writing from hardware and updating the
> events of interest are operations in which the client is interested, but
> are applied to the device. Strengthen the concept of the client in the
> subsystem and clean up some call-sites by translating between the client
> and device types in the core of the KCS subsystem.
> 
> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/char/ipmi/kcs_bmc.c           | 67 ++++++++++++++++++---------
>  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 50 ++++++++++----------
>  drivers/char/ipmi/kcs_bmc_client.h    | 15 +++---
>  drivers/char/ipmi/kcs_bmc_serio.c     | 10 ++--
>  4 files changed, 81 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index 5a3f199241d2..d70e503041bd 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -22,33 +22,53 @@ static LIST_HEAD(kcs_bmc_drivers);
>  
>  /* Consumer data access */
>  
> -u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
> +static void kcs_bmc_client_validate(struct kcs_bmc_client *client)
>  {
> -	return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
> +	WARN_ONCE(client != READ_ONCE(client->dev->client), "KCS client confusion detected");

Is this intended as runtime validation or to catch bugs?
If just catch bugs then fair enough.

With that question answered based on my somewhat vague understanding of the kcs subsystem.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> +}

Re: [PATCH 02/10] ipmi: kcs_bmc: Include spinlock.h

[Andrew Jeffery]

On Fri, 2023-11-03 at 14:36 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:14 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > struct kcs_bmc_device defines a spinlock member but the header in which
> > it is defined failed to include the spinlock header. In the spirit of
> > include-what-you-use, do what's necessary.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> This is fine, but whilst checking it I noticed there is no
> forwards def of struct device or appropriate include.

Ah, I'll fix that too.

clangd automatically added the spinlock include at one point and so I
figured I'd capture it.

Andrew

Re: [PATCH 03/10] ipmi: kcs_bmc: Make kcs_bmc_update_event_mask() static

[Andrew Jeffery]

On Fri, 2023-11-03 at 14:40 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:15 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > There were no users outside the subsystem core, so let's not expose it.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> Is it worth having the wrapper?

Perhaps not, though aesthetically I prefer it. Also the diff is at
least slightly smaller by not removing it entirely :)

> 
> I guess all the other cases do have wrappers (even if that's because
> they continue to be exported) so fair enough.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Andrew

Re: [PATCH 04/10] ipmi: kcs_bmc: Make remove_device() callback return void

[Andrew Jeffery]

On Fri, 2023-11-03 at 14:43 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:16 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > Don't pretend there's a valid failure path when there's not.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 
> Whilst I agree returning an error code is pointless, it is perhaps
> useful to make sure there is a dev_err() or similar in the paths
> now that you've remove the one at the call site.
> 
> Minor point and up to you if you want to or not.

No, that's reasonable. I'll address this in v2.

Andrew

Re: [PATCH 06/10] ipmi: kcs_bmc: Integrate buffers into driver struct

[Andrew Jeffery]

On Fri, 2023-11-03 at 14:45 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:18 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > Consolidate several necessary allocations into one to reduce the number
> > of possible error paths.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> Gets rid of some of the devm_kfree() fun, so I'm in favor of the change :)
> 
> One trivial comment inline.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks.

> > @@ -478,19 +476,15 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
> >  
> >  	spin_lock_init(&priv->lock);
> >  	mutex_init(&priv->mutex);
> > -
> Unrelated change...

Ack, will drop in v2.

Re: [PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device lifetimes

[Andrew Jeffery]

On Fri, 2023-11-03 at 14:51 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:19 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > KCS client modules may be removed by actions unrelated to KCS devices.
> > As usual, removing a KCS client module requires unbinding all client
> > instances from the underlying devices to prevent further use of the code.
> > 
> > Previously, KCS client resource lifetimes were tied to the underlying
> > KCS device instance with the use of `devm_k[mz]alloc()` APIs. This
> > requires the use of `devm_free()` as a consequence. It's necessary to
> > scrutinise use of explicit `devm_free()`s because they generally
> > indicate there's a concerning corner-case in play, but that's not really
> > the case here. Switch to explicit kmalloc()/kfree() to align
> > expectations with the intent of the code.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> Bit more unrelated white space stuff in here that ideally wouldn't be there.

Ack, I'll address that for v2.

> Otherwise makes sense to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Andrew

Re: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core

[Andrew Jeffery]

On Fri, 2023-11-03 at 15:05 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:20 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > I ran out of spoons before I could come up with a better client tracking
> > scheme back in the original refactoring series:
> > 
> > https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/
> > 
> > Jonathan prodded Konstantin about the issue in a review of Konstantin's
> > MCTP patches[1], prompting an attempt to clean it up.
> > 
> > [1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/
> > 
> > Prevent client modules from having to track their own instances by
> > requiring they return a pointer to a client object from their
> > add_device() implementation. We can then track this in the core, and
> > provide it as the argument to the remove_device() implementation to save
> > the client module from further work. The usual container_of() pattern
> > gets the client module access to its private data.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 
> Hi Andrew,
> 
> A few comments inline.
> More generally, whilst this is definitely an improvement I'd have been tempted
> to make more use of the linux device model for this with the clients added
> as devices with a parent of the kcs_bmc_device.  That would then allow for
> simple dependency tracking, binding of individual drivers and all that.
> 
> What you have here feels fine though and is a much less invasive change.

Yeah, I had this debate with myself before posting the patches. My
reasoning for the current approach is that the clients don't typically
represent a device, rather a protocol implementation that is
communicated over a KCS device (maybe more like pairing a line
discipline with a UART). It was unclear to me whether associating a
`struct device` with a protocol implementation was stretching the
abstraction a bit, or whether I haven't considered some other
perspective hard enough - maybe we treat the client as the remote
device, similar to e.g. a `struct i2c_client`?

> 
> Jonathan
> 
> 
> > diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > index 98f231f24c26..9fca31f8c7c2 100644
> > --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > @@ -71,8 +71,6 @@ enum kcs_ipmi_errors {
> 
> 
> 
> > +static struct kcs_bmc_client *
> > +kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
> >  {
> >  	struct kcs_bmc_ipmi *priv;
> >  	int rc;
> >  
> >  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> > -		return -ENOMEM;
> > +		return ERR_PTR(ENOMEM);
> As below. I thought it took negatives..

I should have double checked that. It requires negatives. Thanks.

> >  
> >  	spin_lock_init(&priv->lock);
> >  	mutex_init(&priv->mutex);
> >  	init_waitqueue_head(&priv->queue);
> >  
> > -	priv->client.dev = kcs_bmc;
> > -	priv->client.ops = &kcs_bmc_ipmi_client_ops;
> > +	kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev);
> >  
> >  	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
> > -	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
> > +	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, dev->channel);
> >  	if (!priv->miscdev.name) {
> >  		rc = -ENOMEM;
> ERR_PTR

I converted it to an ERR_PTR in the return after the cleanup_priv
label. Maybe it's preferable I do the conversion immediately? Easy
enough to change if you think so.

> >  		goto cleanup_priv;
> 
> 
> 
> ...
> 
> > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> > index 0a68c76da955..3cfda39506f6 100644
> > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> 
> ...
> 
> 
> > +static struct kcs_bmc_client *
> > +kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
> >  {
> >  	struct kcs_bmc_serio *priv;
> >  	struct serio *port;
> > @@ -75,12 +71,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
> >  
> >  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> > -		return -ENOMEM;
> > +		return ERR_PTR(ENOMEM);
> >  
> >  	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
> >  	port = kzalloc(sizeof(*port), GFP_KERNEL);
> >  	if (!port) {
> > -		rc = -ENOMEM;
> > +		rc = ENOMEM;
> Why positive?
> Doesn't ERR_PTR() typically get passed negatives? 

Ack, as above.

Thanks for the review,

Andrew

Re: [PATCH 10/10] ipmi: kcs_bmc: Add subsystem kerneldoc

[Andrew Jeffery]

On Fri, 2023-11-03 at 15:12 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:22 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > Provide kerneldoc describing the relationships between and the
> > behaviours of the structures and functions of the KCS subsystem.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> Seems reasonable but I've only a superficial idea of how this all fits
> together so no tag from me.

Thanks for the reviews so far!

> 
> There is the fun question of whether function documentation should be
> next to the implementation or in the header.  As long as it's
> consistent in a given subsystem I don't personally thing it matters
> that much.

Happy to put it where people prefer. I like the consistency of having
it all in the one spot, but I appreciate the idea that it might be
easier to maintain alongside the implementation.

Andrew

Re: [PATCH 05/10] ipmi: kcs_bmc: Define client actions in terms of kcs_bmc_client

[Andrew Jeffery]

On Fri, 2023-11-03 at 15:16 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:17 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > Operations such as reading and writing from hardware and updating the
> > events of interest are operations in which the client is interested, but
> > are applied to the device. Strengthen the concept of the client in the
> > subsystem and clean up some call-sites by translating between the client
> > and device types in the core of the KCS subsystem.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> > ---
> >  drivers/char/ipmi/kcs_bmc.c           | 67 ++++++++++++++++++---------
> >  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 50 ++++++++++----------
> >  drivers/char/ipmi/kcs_bmc_client.h    | 15 +++---
> >  drivers/char/ipmi/kcs_bmc_serio.c     | 10 ++--
> >  4 files changed, 81 insertions(+), 61 deletions(-)
> > 
> > diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> > index 5a3f199241d2..d70e503041bd 100644
> > --- a/drivers/char/ipmi/kcs_bmc.c
> > +++ b/drivers/char/ipmi/kcs_bmc.c
> > @@ -22,33 +22,53 @@ static LIST_HEAD(kcs_bmc_drivers);
> >  
> >  /* Consumer data access */
> >  
> > -u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc)
> > +static void kcs_bmc_client_validate(struct kcs_bmc_client *client)
> >  {
> > -	return kcs_bmc->ops->io_inputb(kcs_bmc, kcs_bmc->ioreg.idr);
> > +	WARN_ONCE(client != READ_ONCE(client->dev->client), "KCS client confusion detected");
> 
> Is this intended as runtime validation or to catch bugs?
> If just catch bugs then fair enough.

Ah, I think I missed replying here.

So for "runtime validation" I assume you mean "things userspace might
do that are not valid - the error condition should be detected and
punted back to userspace", vs "catch bugs" meaning "the implementation
in the kernel failed to uphold an invariant and now there are
Problems".

If that sounds accurate, then it's the latter: The WARN_ONCE() is
asserting "don't operate on a client that doesn't own the device". It
isn't an error that can be punted back for handling in userspace as it
should not be possible for the kernel to get into this state to begin
with. If we reach this state it's an error in the programming of the
kernel module that's a client of the KCS subsystem.

> 
> With that question answered based on my somewhat vague understanding of the kcs subsystem.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Andrew

Re: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core

[Jonathan Cameron]

On Mon, 06 Nov 2023 10:26:38 +1030
Andrew Jeffery <andrew@codeconstruct.com.au> wrote:

> On Fri, 2023-11-03 at 15:05 +0000, Jonathan Cameron wrote:
> > On Fri,  3 Nov 2023 16:45:20 +1030
> > Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> >   
> > > I ran out of spoons before I could come up with a better client tracking
> > > scheme back in the original refactoring series:
> > > 
> > > https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/
> > > 
> > > Jonathan prodded Konstantin about the issue in a review of Konstantin's
> > > MCTP patches[1], prompting an attempt to clean it up.
> > > 
> > > [1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/
> > > 
> > > Prevent client modules from having to track their own instances by
> > > requiring they return a pointer to a client object from their
> > > add_device() implementation. We can then track this in the core, and
> > > provide it as the argument to the remove_device() implementation to save
> > > the client module from further work. The usual container_of() pattern
> > > gets the client module access to its private data.
> > > 
> > > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>  
> > 
> > Hi Andrew,
> > 
> > A few comments inline.
> > More generally, whilst this is definitely an improvement I'd have been tempted
> > to make more use of the linux device model for this with the clients added
> > as devices with a parent of the kcs_bmc_device.  That would then allow for
> > simple dependency tracking, binding of individual drivers and all that.
> > 
> > What you have here feels fine though and is a much less invasive change.  
> 
Sorry for slow reply, been traveling.

> Yeah, I had this debate with myself before posting the patches. My
> reasoning for the current approach is that the clients don't typically
> represent a device, rather a protocol implementation that is
> communicated over a KCS device (maybe more like pairing a line
> discipline with a UART). It was unclear to me whether associating a
> `struct device` with a protocol implementation was stretching the
> abstraction a bit, or whether I haven't considered some other
> perspective hard enough - maybe we treat the client as the remote
> device, similar to e.g. a `struct i2c_client`?

That was my thinking.  The protocol is used to talk to someone - the endpoint
(similar to i2c_client) so represent that. If that device is handling multiple
protocols (no idea if that is possible) - that is fine as well, it just becomes
like having multiple i2c_clients in a single package (fairly common for sensors),
or the many other cases where we use a struct device to represent just part
of a larger device that operates largely independently of other parts (or with
well defined boundaries).

Jonathan



> 
> > 
> > Jonathan
> > 
> >   
> > > diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > > index 98f231f24c26..9fca31f8c7c2 100644
> > > --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > > +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > > @@ -71,8 +71,6 @@ enum kcs_ipmi_errors {  
> > 
> > 
> >   
> > > +static struct kcs_bmc_client *
> > > +kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
> > >  {
> > >  	struct kcs_bmc_ipmi *priv;
> > >  	int rc;
> > >  
> > >  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > >  	if (!priv)
> > > -		return -ENOMEM;
> > > +		return ERR_PTR(ENOMEM);  
> > As below. I thought it took negatives..  
> 
> I should have double checked that. It requires negatives. Thanks.
> 
> > >  
> > >  	spin_lock_init(&priv->lock);
> > >  	mutex_init(&priv->mutex);
> > >  	init_waitqueue_head(&priv->queue);
> > >  
> > > -	priv->client.dev = kcs_bmc;
> > > -	priv->client.ops = &kcs_bmc_ipmi_client_ops;
> > > +	kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev);
> > >  
> > >  	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
> > > -	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
> > > +	priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, dev->channel);
> > >  	if (!priv->miscdev.name) {
> > >  		rc = -ENOMEM;  
> > ERR_PTR  
> 
> I converted it to an ERR_PTR in the return after the cleanup_priv
> label. Maybe it's preferable I do the conversion immediately? Easy
> enough to change if you think so.

I'm not that fussed either way.

> 
> > >  		goto cleanup_priv;  
> > 
> > 
> > 
> > ...
> >   
> > > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> > > index 0a68c76da955..3cfda39506f6 100644
> > > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > > +++ b/drivers/char/ipmi/kcs_bmc_serio.c  
> > 
> > ...
> > 
> >   
> > > +static struct kcs_bmc_client *
> > > +kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
> > >  {
> > >  	struct kcs_bmc_serio *priv;
> > >  	struct serio *port;
> > > @@ -75,12 +71,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
> > >  
> > >  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > >  	if (!priv)
> > > -		return -ENOMEM;
> > > +		return ERR_PTR(ENOMEM);
> > >  
> > >  	/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
> > >  	port = kzalloc(sizeof(*port), GFP_KERNEL);
> > >  	if (!port) {
> > > -		rc = -ENOMEM;
> > > +		rc = ENOMEM;  
> > Why positive?
> > Doesn't ERR_PTR() typically get passed negatives?   
> 
> Ack, as above.
> 
> Thanks for the review,
> 
> Andrew

Re: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core

[Andrew Jeffery]

On Mon, 2023-11-20 at 12:40 +0000, Jonathan Cameron wrote:
> On Mon, 06 Nov 2023 10:26:38 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> 
> > On Fri, 2023-11-03 at 15:05 +0000, Jonathan Cameron wrote:
> > > On Fri,  3 Nov 2023 16:45:20 +1030
> > > Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
> > >   
> > > > I ran out of spoons before I could come up with a better client tracking
> > > > scheme back in the original refactoring series:
> > > > 
> > > > https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/
> > > > 
> > > > Jonathan prodded Konstantin about the issue in a review of Konstantin's
> > > > MCTP patches[1], prompting an attempt to clean it up.
> > > > 
> > > > [1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/
> > > > 
> > > > Prevent client modules from having to track their own instances by
> > > > requiring they return a pointer to a client object from their
> > > > add_device() implementation. We can then track this in the core, and
> > > > provide it as the argument to the remove_device() implementation to save
> > > > the client module from further work. The usual container_of() pattern
> > > > gets the client module access to its private data.
> > > > 
> > > > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>  
> > > 
> > > Hi Andrew,
> > > 
> > > A few comments inline.
> > > More generally, whilst this is definitely an improvement I'd have been tempted
> > > to make more use of the linux device model for this with the clients added
> > > as devices with a parent of the kcs_bmc_device.  That would then allow for
> > > simple dependency tracking, binding of individual drivers and all that.
> > > 
> > > What you have here feels fine though and is a much less invasive change.  
> > 
> Sorry for slow reply, been traveling.

No worries, I've just got back from travel myself :)

> 
> > Yeah, I had this debate with myself before posting the patches. My
> > reasoning for the current approach is that the clients don't typically
> > represent a device, rather a protocol implementation that is
> > communicated over a KCS device (maybe more like pairing a line
> > discipline with a UART). It was unclear to me whether associating a
> > `struct device` with a protocol implementation was stretching the
> > abstraction a bit, or whether I haven't considered some other
> > perspective hard enough - maybe we treat the client as the remote
> > device, similar to e.g. a `struct i2c_client`?
> 
> That was my thinking.  The protocol is used to talk to someone - the endpoint
> (similar to i2c_client) so represent that. If that device is handling multiple
> protocols (no idea if that is possible) - that is fine as well, it just becomes
> like having multiple i2c_clients in a single package (fairly common for sensors),
> or the many other cases where we use a struct device to represent just part
> of a larger device that operates largely independently of other parts (or with
> well defined boundaries).

Alright, let me think about that a bit more.

Thanks for the feedback,

Andrew