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

[Scott Wood]

On Sun, 2015-10-25 at 17:41 -0500, Lijun Pan wrote:
> 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

ioctl

> + *
> + * 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)
> +
> +#define RESTOOL_SEND_MC_COMMAND \
> +     _IOWR(RESTOOL_IOCTL_TYPE, 0x4, struct mc_command)

Look at Documentation/ioctl/ioctl-number.txt and reserve a range within 'R' 
that doesn't conflict.

Add thorough documentation of this API.  

I'm not sure how it's usually handled with staging drivers, but eventually 
this will need to move to an appropriate uapi header.  Is this functionality 
even needed before the driver comes out of staging?  I don't see "userspace 
restool support" in drivers/staging/fsl-mc/TODO.

Don't reference struct mc_command without including the header that defines 
it.

> +#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

> +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;
> +             }
> +     }

No synchronization on the list?

> +
> +     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;
> +     }

Why is the first user handled differently from subsequent users?  Does each 
user really need its own portal, or can you just synchronize access?

-Scott

--
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/