linux-kernel - Re: [PATCH 08/10] ipmi: kcs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231103150522.00004539@Huawei.com>
Date:   Fri, 3 Nov 2023 15:05:22 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Andrew Jeffery <andrew@...econstruct.com.au>
CC:     <minyard@....org>, <openipmi-developer@...ts.sourceforge.net>,
        <linux-kernel@...r.kernel.org>, <aladyshev22@...il.com>,
        <jk@...econstruct.com.au>
Subject: Re: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core

On Fri,  3 Nov 2023 16:45:20 +1030
Andrew Jeffery <andrew@...econstruct.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@...econstruct.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);
>  }