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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxx>

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