diff mbox series

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

Message ID 20231103061522.1268637-8-andrew@codeconstruct.com.au
State New
Headers
Series ipmi: kcs_bmc: Miscellaneous cleanups |

Commit Message

Andrew Jeffery Nov. 3, 2023, 6:15 a.m. UTC 
  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(-)

Comments

Andrew Jeffery Nov. 5, 2023, 11:01 p.m. UTC | #1 
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
diff mbox series

Patch

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 = {