lkml.org 
[lkml]   [2018]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/3] bus: fsl-mc: add restool userspace support
Date
Hi, 

Comments inline.

> > Adding kernel support for restool, a userspace tool for resource
> > management, means exporting an ioctl capable device file representing
> > the root resource container.
> > This new functionality in the fsl-mc bus driver intends to provide
> > restool an interface to interact with the MC firmware.
> > Commands that are composed in userspace are sent to the MC firmware
> > through the RESTOOL_SEND_MC_COMMAND ioctl.
> > By default the implicit MC I/O portal is used for this operation, but
> > if the implicit one is busy, a dynamic portal is allocated and then
> > freed upon execution.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> > Documentation/ioctl/ioctl-number.txt | 1 +
> > Documentation/networking/dpaa2/overview.rst | 4 +
> > drivers/bus/fsl-mc/Kconfig | 7 +
> > drivers/bus/fsl-mc/Makefile | 3 +
> > drivers/bus/fsl-mc/fsl-mc-allocator.c | 5 +
> > drivers/bus/fsl-mc/fsl-mc-bus.c | 19 +++
> > drivers/bus/fsl-mc/fsl-mc-private.h | 56 +++++++
> > drivers/bus/fsl-mc/fsl-mc-restool.c | 219
> ++++++++++++++++++++++++++++
>
> This is a "tiny" patch, yet I think it needs to be broken up more, as you are
> mixing a few different things in the same patch, and you forgot one big thing...

I will break the patch into multiple ones in the next version.

>
> > 8 files changed, 314 insertions(+)
> > create mode 100644 drivers/bus/fsl-mc/fsl-mc-restool.c
> >
> > diff --git a/Documentation/ioctl/ioctl-number.txt
> > b/Documentation/ioctl/ioctl-number.txt
> > index 6501389..d427397 100644
> > --- a/Documentation/ioctl/ioctl-number.txt
> > +++ b/Documentation/ioctl/ioctl-number.txt
> > @@ -170,6 +170,7 @@ Code Seq#(hex) Include File
> Comments
> > 'R' 00-1F linux/random.h conflict!
> > 'R' 01 linux/rfkill.h conflict!
> > 'R' C0-DF net/bluetooth/rfcomm.h
> > +'R' E0 drivers/bus/fsl-mc/fsl-mc-private.h
> > 'S' all linux/cdrom.h conflict!
> > 'S' 80-81 scsi/scsi_ioctl.h conflict!
> > 'S' 82-FF scsi/scsi.h conflict!
> > diff --git a/Documentation/networking/dpaa2/overview.rst
> > b/Documentation/networking/dpaa2/overview.rst
> > index 79fede4..1056445 100644
> > --- a/Documentation/networking/dpaa2/overview.rst
> > +++ b/Documentation/networking/dpaa2/overview.rst
> > @@ -127,6 +127,10 @@ level.
> >
> > DPRCs can be defined statically and populated with objects via a
> > config file passed to the MC when firmware starts it.
> > +There is also a Linux user space tool called "restool" that can be
> > +used to create/destroy containers and objects dynamically. The latest
> > +version of restool can be found at:
> > +
> > + https://github.com/qoriq-open-source/restool
> > DPAA2 Objects for an Ethernet Network Interface
> > -----------------------------------------------
> > diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig
> > index c23c77c..66ec3b9 100644
> > --- a/drivers/bus/fsl-mc/Kconfig
> > +++ b/drivers/bus/fsl-mc/Kconfig
> > @@ -14,3 +14,10 @@ config FSL_MC_BUS
> > architecture. The fsl-mc bus driver handles discovery of
> > DPAA2 objects (which are represented as Linux devices) and
> > binding objects to drivers.
> > +
> > +config FSL_MC_RESTOOL
> > + bool "Management Complex (MC) restool support"
> > + depends on FSL_MC_BUS
> > + help
> > + Provides kernel support for the Management Complex resource
> > + manager user-space tool - restool.
>
> Why would you want to make this a build option? Why would you ever _not_
> want this?

Using the restool user-space tool for managing MC resources is not the only possibility for creating/destroying MC objects, changing their properties, etc.
While restool's intended use is in a dynamic context, users also have the option to deploy a static configuration using a Data Path Layout file that describes the MC resource configuration.
In this case, the restool support is no longer needed.

>
>
> > diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile
> > index 6a97f2c..9a155e3 100644
> > --- a/drivers/bus/fsl-mc/Makefile
> > +++ b/drivers/bus/fsl-mc/Makefile
> > @@ -14,3 +14,6 @@ mc-bus-driver-objs := fsl-mc-bus.o \
> > fsl-mc-allocator.o \
> > fsl-mc-msi.o \
> > dpmcp.o
> > +
> > +# MC restool kernel support
> > +obj-$(CONFIG_FSL_MC_RESTOOL) += fsl-mc-restool.o
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > index 452c5d7..fb1442b 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-allocator.c
> > @@ -646,3 +646,8 @@ int __init fsl_mc_allocator_driver_init(void)
> > {
> > return fsl_mc_driver_register(&fsl_mc_allocator_driver);
> > }
> > +
> > +void fsl_mc_allocator_driver_exit(void)
> > +{
> > + fsl_mc_driver_unregister(&fsl_mc_allocator_driver);
> > +}
>
> Why are you mixing the bus/driver changes in with the addition of the ioctl?
> That should be broken out into the "first" patch of this series, to make the
> addition of the ioctl easier to see and review.

Will split the bus changes into a separate patch.

>
> > +#define RESTOOL_IOCTL_TYPE 'R'
> > +#define RESTOOL_IOCTL_SEQ 0xE0
> > +
> > +#define RESTOOL_SEND_MC_COMMAND \
> > + _IOWR(RESTOOL_IOCTL_TYPE, RESTOOL_IOCTL_SEQ, struct
> mc_command)
>
> "struct mc_command" is not defined as a structure that can cross the
> user/kernel boundry at all. At the least it is not in a public uapi header file. It
> also does not use the correct variable types, and it is a very generic name for
> a global kernel structure that the whole world is now going to be able to see.
>
> Please fix all of that up first, before adding the ioctl itself :)

I will move the mc_command structure into a uapi header file for the fsl-mc bus in the next version of the patchset.


>
> > +static int fsl_mc_restool_send_command(unsigned long arg,
> > + struct fsl_mc_io *mc_io)
> > +{
> > + struct mc_command mc_cmd;
> > + int error;
> > +
> > + error = copy_from_user(&mc_cmd, (void __user *)arg,
> sizeof(mc_cmd));
> > + if (error)
> > + return -EFAULT;
> > +
> > + error = mc_send_command(mc_io, &mc_cmd);
>
> are you doing correct error and validation checking of this user-provided
> structure? Remember, you can not trust this data at all.

The Management Complex is the one validating the commands received.
The restool support in the bus driver is just a passthrough for the commands passed from user-space and their associated responses from the MC firmware.

>
> All input is evil.
>
> thanks,
>
> greg k-h

\
 
 \ /
  Last update: 2018-03-13 10:58    [W:0.106 / U:23.028 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site