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

[Lijun Pan]

> -----Original Message-----
> From: Pan Lijun-B44306
> Sent: Thursday, October 29, 2015 6:55 PM
> To: Wood Scott-B07421 <scottwood@xxxxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Yoder Stuart-B08248
> <stuart.yoder@xxxxxxxxxxxxx>; katz Itai-RM05202 <itai.katz@xxxxxxxxxxxxx>;
> Rivera Jose-B46482 <German.Rivera@xxxxxxxxxxxxx>; Li Yang-Leo-R58472
> <LeoLi@xxxxxxxxxxxxx>; agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1
> <bhamciu1@xxxxxxxxxxxxx>; Marginean Alexandru-R89243
> <R89243@xxxxxxxxxxxxx>; Sharma Bhupesh-B45370
> <bhupesh.sharma@xxxxxxxxxxxxx>; Erez Nir-RM30794
> <nir.erez@xxxxxxxxxxxxx>; Schmitt Richard-B43082
> <richard.schmitt@xxxxxxxxxxxxx>; dan.carpenter@xxxxxxxxxx
> Subject: RE: [PATCH 5/5] staging: fsl-mc: Management Complex restool driver
> 
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, October 27, 2015 12:17 AM
> > To: Pan Lijun-B44306 <Lijun.Pan@xxxxxxxxxxxxx>
> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx;
> > devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Yoder
> > Stuart-B08248 <stuart.yoder@xxxxxxxxxxxxx>; katz Itai-RM05202
> > <itai.katz@xxxxxxxxxxxxx>; Rivera Jose-B46482
> > <German.Rivera@xxxxxxxxxxxxx>; Li Yang-Leo-R58472
> > <LeoLi@xxxxxxxxxxxxx>; agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1
> > <bhamciu1@xxxxxxxxxxxxx>; Marginean Alexandru-R89243
> > <R89243@xxxxxxxxxxxxx>; Sharma Bhupesh-B45370
> > <bhupesh.sharma@xxxxxxxxxxxxx>; Erez Nir-RM30794
> > <nir.erez@xxxxxxxxxxxxx>; Schmitt Richard-B43082
> > <richard.schmitt@xxxxxxxxxxxxx>; dan.carpenter@xxxxxxxxxx
> > Subject: Re: [PATCH 5/5] staging: fsl-mc: Management Complex restool
> > driver
> >
> > 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.
> 
> What path do you recommend me to put documentation of this API?

Is Documentation/misc-devices/mc-restool.txt the correct location? 

> >
> > 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?
> 
> First user get the statically allocated portal.
> Second user need to dynamically allocate one.
> Some operations may take a long time (say 1 second) in MC object creation.
> In order to support asynchronous portal operation, We allocate portal for each
> user.
> 
> Lijun
> 
> >
> > -Scott

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå