Messages in this thread | | | Date | Fri, 3 Nov 2023 14:51:29 +0000 | From | Jonathan Cameron <> | Subject | Re: [PATCH 07/10] ipmi: kcs_bmc: Disassociate client from device lifetimes |
| |
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 = {
| |