Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Ioana Ciornei <ioana.ciornei@nxp.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     Laurentiu Tudor <laurentiu.tudor@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stuyoder@gmail.com" <stuyoder@gmail.com>,
        Ruxandra Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>,
        "arnd@arndb.de" <arnd@arndb.de>
Subject: RE: [PATCH 1/3] bus: fsl-mc: add restool userspace support
Thread-Topic: [PATCH 1/3] bus: fsl-mc: add restool userspace support
Thread-Index: AQHTtjQexfPnB3EJWkeApMJaRzBDq6PITp4AgAWeNSA=
Date:   Tue, 13 Mar 2018 09:57:36 +0000
Message-ID: <HE1PR04MB3212C7D32214AC5045FB0949E0D20@HE1PR04MB3212.eurprd04.prod.outlook.com>
References: <cover.1520440367.git.ioana.ciornei@nxp.com>
 <e3d050dcdc575d65f9786eaa1b7a55aaa197dfc3.1520440367.git.ioana.ciornei@nxp.com>
 <20180309193334.GA20133@kroah.com>
In-Reply-To: <20180309193334.GA20133@kroah.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4799fe52-60af-420e-5b8f-08d588c8d9bf
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Mar 2018 09:57:36.9107
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR04MB1660
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,=20

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
> ++++++++++++++++++++++++++++
>=20
> 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 t=
hing...

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

>=20
> >  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.
>=20
> 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.
=20
>=20
>=20
> > 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 :=3D fsl-mc-bus.o \
> >  		      fsl-mc-allocator.o \
> >  		      fsl-mc-msi.o \
> >  		      dpmcp.o
> > +
> > +# MC restool kernel support
> > +obj-$(CONFIG_FSL_MC_RESTOOL) +=3D 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);
> > +}
>=20
> Why are you mixing the bus/driver changes in with the addition of the ioc=
tl?
> 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.

>=20
> > +#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)
>=20
> "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 head=
er file.  It
> also does not use the correct variable types, and it is a very generic na=
me for
> a global kernel structure that the whole world is now going to be able to=
 see.
>=20
> 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.


>=20
> > +static int fsl_mc_restool_send_command(unsigned long arg,
> > +				       struct fsl_mc_io *mc_io)
> > +{
> > +	struct mc_command mc_cmd;
> > +	int error;
> > +
> > +	error =3D copy_from_user(&mc_cmd, (void __user *)arg,
> sizeof(mc_cmd));
> > +	if (error)
> > +		return -EFAULT;
> > +
> > +	error =3D mc_send_command(mc_io, &mc_cmd);
>=20
> 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 command=
s passed from user-space and their associated responses from the MC firmwar=
e.

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