RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver

[Stuart Yoder]

> -----Original Message-----
> From: Lijun Pan [mailto:Lijun.Pan@xxxxxxxxxxxxx]
> Sent: Sunday, October 25, 2015 5:41 PM
> To: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Rivera Jose-B46482; Li Yang-Leo-R58472; Wood Scott-B07421;
> agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1; Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; dan.carpenter@xxxxxxxxxx; Pan Lijun-B44306
> Subject: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> The kernel support for the restool (a user space resource management
> tool) is a driver for the /dev/dprc.N device file.
> Its purpose is to provide an ioctl interface,
> which the restool uses to interact with the MC bus driver
> and with the MC firmware.
> We allocate a dpmcp at driver initialization,
> and keep that dpmcp until driver exit.
> We use that dpmcp by default.
> If that dpmcp is in use, we create another portal at run time
> and destroy the newly created portal after use.
> The ioctl RESTOOL_SEND_MC_COMMAND sends user space command to fsl-mc
> bus and utilizes the fsl-mc bus to communicate with MC firmware.
> The ioctl RESTOOL_DPRC_SYNC request the mc-bus launch
> objects scan under root dprc.
> In order to support multiple root dprc, we utilize the bus notify
> mechanism to scan fsl_mc_bus_type for the newly added root dprc.
> After discovering the root dprc, it creates a miscdevice
> /dev/dprc.N to associate with this root dprc.
> 
> Signed-off-by: Lijun Pan <Lijun.Pan@xxxxxxxxxxxxx>
> ---
>  drivers/staging/fsl-mc/bus/Kconfig      |   7 +-
>  drivers/staging/fsl-mc/bus/Makefile     |   3 +
>  drivers/staging/fsl-mc/bus/mc-ioctl.h   |  24 ++
>  drivers/staging/fsl-mc/bus/mc-restool.c | 488 ++++++++++++++++++++++++++++++++
>  4 files changed, 521 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-ioctl.h
>  create mode 100644 drivers/staging/fsl-mc/bus/mc-restool.c
> 
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..39c6ef9 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -21,4 +21,9 @@ config FSL_MC_BUS
>  	  Only enable this option when building the kernel for
>  	  Freescale QorQIQ LS2xxxx SoCs.
> 
> -
> +config FSL_MC_RESTOOL
> +        tristate "Freescale Management Complex (MC) restool driver"
> +        depends on FSL_MC_BUS
> +        help
> +          Driver that provides kernel support for the Freescale Management
> +	  Complex resource manager user-space tool.
> diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-mc/bus/Makefile
> index 25433a9..28b5fc0 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -15,3 +15,6 @@ mc-bus-driver-objs := mc-bus.o \
>  		      mc-allocator.o \
>  		      dpmcp.o \
>  		      dpbp.o
> +
> +# MC restool kernel support
> +obj-$(CONFIG_FSL_MC_RESTOOL) += mc-restool.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-ioctl.h b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> new file mode 100644
> index 0000000..e52f907
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-ioctl.h
> @@ -0,0 +1,24 @@
> +/*
> + * Freescale Management Complex (MC) ioclt interface
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@xxxxxxxxxxxxx>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#ifndef _FSL_MC_IOCTL_H_
> +#define _FSL_MC_IOCTL_H_
> +
> +#include <linux/ioctl.h>
> +
> +#define RESTOOL_IOCTL_TYPE   'R'
> +
> +#define RESTOOL_DPRC_SYNC \
> +	_IO(RESTOOL_IOCTL_TYPE, 0x2)

We no longer need a sync ioctl, as a the /sys/bus/fsl-mc/rescan mechanism
in the previous patch replaces this.

> +#define RESTOOL_SEND_MC_COMMAND \
> +	_IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)
> +
> +#endif /* _FSL_MC_IOCTL_H_ */
> diff --git a/drivers/staging/fsl-mc/bus/mc-restool.c b/drivers/staging/fsl-mc/bus/mc-restool.c
> new file mode 100644
> index 0000000..a219172
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-restool.c
> @@ -0,0 +1,488 @@
> +/*
> + * Freescale Management Complex (MC) restool driver
> + *
> + * Copyright (C) 2014 Freescale Semiconductor, Inc.
> + * Author: Lijun Pan <Lijun.Pan@xxxxxxxxxxxxx>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include "../include/mc-private.h"
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include "mc-ioctl.h"
> +#include "../include/mc-sys.h"
> +#include "../include/mc-cmd.h"
> +#include "../include/dpmng.h"
> +
> +/**
> + * Maximum number of DPRCs that can be opened at the same time
> + */
> +#define MAX_DPRC_HANDLES	    64
> +
> +/**
> + * restool_misc - information associated with the newly added miscdevice
> + * @misc: newly created miscdevice associated with root dprc
> + * @miscdevt: device id of this miscdevice
> + * @list: a linked list node representing this miscdevcie
> + * @static_mc_io: pointer to the static MC I/O object used by the restool
> + * @dynamic_instance_count: number of dynamically created instances
> + * @static_instance_in_use: static instance is in use or not
> + * @mutex: mutex lock to serialze the operations
> + * @dev: root dprc associated with this miscdevice
> + */
> +struct restool_misc {
> +	struct miscdevice misc;
> +	dev_t miscdevt;
> +	struct list_head list;
> +	struct fsl_mc_io *static_mc_io;
> +	uint32_t dynamic_instance_count;
> +	bool static_instance_in_use;
> +	struct mutex mutex;
> +	struct device *dev;
> +};
> +
> +/*
> + * initialize a global list to link all
> + * the miscdevice nodes (struct restool_misc)
> + */
> +LIST_HEAD(misc_list);
> +
> +static int fsl_mc_restool_dev_open(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_device *root_mc_dev;
> +	int error = 0;
> +	struct fsl_mc_io *dynamic_mc_io = NULL;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	if (WARN_ON(restool_misc->dev == NULL))
> +		return -EINVAL;
> +
> +	mutex_lock(&restool_misc->mutex);
> +
> +	if (!restool_misc->static_instance_in_use) {
> +		restool_misc->static_instance_in_use = true;
> +		filep->private_data = restool_misc->static_mc_io;
> +	} else {
> +		dynamic_mc_io = kzalloc(sizeof(struct fsl_mc_io), GFP_KERNEL);
> +		if (dynamic_mc_io == NULL) {
> +			error = -ENOMEM;
> +			goto error;
> +		}
> +
> +		root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0, &dynamic_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto error;
> +		}
> +		++restool_misc->dynamic_instance_count;
> +		filep->private_data = dynamic_mc_io;
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return 0;
> +error:
> +	if (dynamic_mc_io != NULL) {
> +		fsl_mc_portal_free(dynamic_mc_io);
> +		kfree(dynamic_mc_io);
> +	}
> +
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return error;
> +}
> +
> +static int fsl_mc_restool_dev_release(struct inode *inode, struct file *filep)
> +{
> +	struct fsl_mc_io *local_mc_io = filep->private_data;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	if (WARN_ON(filep->private_data == NULL))
> +		return -EINVAL;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	mutex_lock(&restool_misc->mutex);
> +
> +	if (WARN_ON(restool_misc->dynamic_instance_count == 0 &&
> +	    !restool_misc->static_instance_in_use)) {
> +		mutex_unlock(&restool_misc->mutex);
> +		return -EINVAL;
> +	}
> +
> +	/* Globally clean up opened/untracked handles */
> +	fsl_mc_portal_reset(local_mc_io);
> +
> +	pr_debug("dynamic instance count: %d\n",
> +		restool_misc->dynamic_instance_count);
> +	pr_debug("static instance count: %d\n",
> +		restool_misc->static_instance_in_use);
> +
> +	/*
> +	 * must check
> +	 * whether local_mc_io is dynamic or static instance
> +	 * Otherwise it will free up the reserved portal by accident
> +	 * or even not free up the dynamic allocated portal
> +	 * if 2 or more instances running concurrently
> +	 */
> +	if (local_mc_io == restool_misc->static_mc_io) {
> +		pr_debug("this is reserved portal");
> +		pr_debug("reserved portal not in use\n");
> +		restool_misc->static_instance_in_use = false;
> +	} else {
> +		pr_debug("this is dynamically allocated  portal");
> +		pr_debug("free one dynamically allocated portal\n");
> +		fsl_mc_portal_free(local_mc_io);
> +		kfree(filep->private_data);
> +		--restool_misc->dynamic_instance_count;
> +	}
> +
> +	filep->private_data = NULL;
> +	mutex_unlock(&restool_misc->mutex);
> +
> +	return 0;
> +}
> +
> +static int restool_dprc_sync(struct inode *inode)
> +{
> +	int error = 0;
> +	struct fsl_mc_device *root_mc_dev;
> +	struct fsl_mc_bus *root_mc_bus;
> +	struct restool_misc *restool_misc;
> +	struct restool_misc *restool_misc_cursor;
> +
> +	pr_debug("%s: inode's dev_t == %u\n", __func__, inode->i_rdev);
> +
> +	list_for_each_entry(restool_misc_cursor, &misc_list, list) {
> +		if (restool_misc_cursor->miscdevt == inode->i_rdev) {
> +			pr_debug("%s: Found the restool_misc\n", __func__);
> +			restool_misc = restool_misc_cursor;
> +			break;
> +		}
> +	}
> +
> +	if (!restool_misc)
> +		return -EINVAL;
> +
> +	root_mc_dev = to_fsl_mc_device(restool_misc->dev);
> +	root_mc_bus = to_fsl_mc_bus(root_mc_dev);
> +
> +	mutex_lock(&root_mc_bus->scan_mutex);
> +	error = dprc_scan_objects(root_mc_dev);
> +	mutex_unlock(&root_mc_bus->scan_mutex);
> +	pr_debug("sync_error = %d\n", error);
> +
> +	return error;
> +}

We no longer need/use the ioctl interface for sync.

> +static int restool_send_mc_command(unsigned long arg,
> +				struct fsl_mc_io *local_mc_io)
> +{
> +	int error = -EINVAL;
> +	struct mc_command mc_cmd;
> +
> +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}
> +
> +	/*
> +	 * Send MC command to the MC:
> +	 */
> +	error = mc_send_command(local_mc_io, &mc_cmd);
> +	if (error < 0)
> +		goto error;
> +
> +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> +	if (error < 0) {
> +		pr_err("copy_to_user() failed with error %d\n", error);
> +		goto error;
> +	}
> +
> +	return 0;
> +error:
> +	return error;
> +}
> +
> +static long
> +fsl_mc_restool_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	int error = 0;
> +
> +	switch (cmd) {
> +	case RESTOOL_DPRC_SYNC:
> +		pr_debug("syncing...\n");
> +		error = restool_dprc_sync(file->f_inode);
> +		pr_debug("syncing finished...\n");
> +		break;

Remove the sync ioctl.

> +	case RESTOOL_SEND_MC_COMMAND:
> +		error = restool_send_mc_command(arg, file->private_data);
> +		break;
> +	default:
> +		pr_err("%s: unexpected ioctl call number\n", __func__);
> +		error = -EINVAL;
> +	}
> +
> +	return error;
> +}
> +
> +static const struct file_operations fsl_mc_restool_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = fsl_mc_restool_dev_open,
> +	.release = fsl_mc_restool_dev_release,
> +	.unlocked_ioctl = fsl_mc_restool_dev_ioctl,
> +};
> +
> +static int restool_add_device_file(struct device *dev)
> +{
> +	uint32_t name1 = 0;
> +	char name2[20] = {0};
> +	int error = 0;
> +	struct fsl_mc_device *root_mc_dev;
> +	struct restool_misc *restool_misc;
> +
> +	pr_debug("newly scanned/notified device: %s, whose parent:%s\n",
> +		 dev_name(dev), dev_name(dev->parent));
> +
> +	if (dev->bus == &platform_bus_type && dev->driver_data) {
> +		if (sscanf(dev_name(dev), "%x.%s", &name1, name2) != 2) {
> +			pr_err("sscanf failure\n");
> +			return -EINVAL;
> +		}
> +		if (strcmp(name2, "fsl-mc") == 0)
> +			pr_debug("platform's root dprc name is: %s\n",
> +			dev_name(&(((struct fsl_mc *)(dev->driver_data))
> +			->root_mc_bus_dev->dev)));
> +	}
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	if (is_root_dprc(dev)) {
> +		pr_debug("I am root dprc, create /dev/%s\n", dev_name(dev));
> +		restool_misc = kzalloc(sizeof(struct restool_misc), GFP_KERNEL);
> +		if (restool_misc == NULL)
> +			return -ENOMEM;
> +
> +		restool_misc->dev = dev;
> +		root_mc_dev = to_fsl_mc_device(dev);
> +		error = fsl_mc_portal_allocate(root_mc_dev, 0,
> +				&restool_misc->static_mc_io);
> +		if (error < 0) {
> +			pr_err("Not able to allocate MC portal\n");
> +			goto err_portal;
> +		}
> +
> +		restool_misc->misc.minor = MISC_DYNAMIC_MINOR;
> +		restool_misc->misc.name = dev_name(dev);
> +		restool_misc->misc.fops = &fsl_mc_restool_dev_fops;
> +
> +		error = misc_register(&restool_misc->misc);
> +		if (error < 0) {
> +			pr_err("misc_register() failed: %d\n", error);
> +			goto err_reg;
> +		}
> +
> +		restool_misc->miscdevt = restool_misc->misc.this_device->devt;
> +		mutex_init(&restool_misc->mutex);
> +		list_add(&restool_misc->list, &misc_list);
> +		pr_info("/dev/%s driver registered\n", dev_name(dev));
> +	} else
> +		pr_info("%s is not root dprc, miscdevice cannot be created/associated\n",
> +			dev_name(dev));
> +
> +	return 0;
> +err_reg:
> +	misc_deregister(&restool_misc->misc);
> +
> +err_portal:
> +	if (restool_misc->static_mc_io)
> +		fsl_mc_portal_free(restool_misc->static_mc_io);
> +	if (restool_misc)
> +		kfree(restool_misc);
> +
> +	return error;
> +}
> +
> +static int restool_bus_notifier(struct notifier_block *nb,
> +			      unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +
> +	pr_debug("entering %s...\n", __func__);
> +	pr_debug("being notified by device: %s\n", dev_name(dev));
> +
> +	if (dev->bus == &fsl_mc_bus_type)
> +		pr_debug("%s's bus type: fsl_mc_bus_type\n", dev_name(dev));
> +	else if (dev->bus == &platform_bus_type)
> +		pr_debug("%s's bus type: platform_bus_type\n", dev_name(dev));
> +	else
> +		pr_debug("%s's bus type: NEITHER fsl_mc_bus_type NOR platform_bus_type\n",
> +			 dev_name(dev));
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		pr_info("bus notify device added: %s\n", dev_name(dev));
> +		restool_add_device_file(dev);

The only time we want to do add a new device file is for root DPRCs.

This code would seem to add a device file any time any device is
added to the bus?  How are you filtering out all the other
object types?...it's not clear to me.

> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		pr_info("bus notify device to be removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_REMOVED_DEVICE:
> +		pr_info("bus notify device removed: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		pr_info("bus notify driver about to be bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		pr_info("bus notify driver bound to device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		pr_info("bus notify driver about to unbind from device: %s\n", dev_name(dev));
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		pr_info("bus notify driver unbind from device: %s\n", dev_name(dev));
> +		break;
> +	default:
> +		pr_err("%s: unrecognized device action from %s\n", __func__, dev_name(dev));
> +		return -EINVAL;
> +	}
> +
> +	pr_debug("leaving %s...\n", __func__);
> +
> +	return 0;
> +}

There is only a single line of functionality in the above function.  All the
rest pr_debug and pr_info.   We certainly don't need pr_info statements
every time a bus notify occurs.

In general I think this driver has too many debug prints in it.

Thanks,
Stuart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/