Messages in this thread | | | From | Ioana Ciornei <> | Subject | RE: [PATCH 1/3] bus: fsl-mc: add restool userspace support | Date | Tue, 13 Mar 2018 09:57:36 +0000 |
| |
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
| |