Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp583395pxj; Thu, 13 May 2021 11:49:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyYy7rznTqszXDw5eMHTQJKF6NbTut9IYk1A0TnwkXNEotvcaGA60hmh7jorHYzJacFe3kt X-Received: by 2002:a05:6808:3b9:: with SMTP id n25mr15179099oie.54.1620931789937; Thu, 13 May 2021 11:49:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1620931789; cv=none; d=google.com; s=arc-20160816; b=mLfZpGiaJcvF6j8/IBbdOEycqr2nfydFGnEgF8yyN/6pVLfnmzKw0fewKM4rzAByw3 GdtLlKzHZ/FC3a6zy8jMxCnJ5bTDMDsjyKrsm2vwDb8qkH/lhCvZUG5ttPn2kW33CEuN GvBPuyhTD0A+t0/Ct0YnKMC/sUztt5teDbBtkP+ZAH9GI2bjMgSxu61/GfGNpAr0TEGi v6V7WKXvF2lY9Pc9r6OhNHYtI7XbBWXwW0nm1fHDB4amKh0bU/Bz1bdADRWMlBAFOC5l f8008bav0zO8XmR6By9NxqIEklkoDhGrN1d4Snn+1mqqfDJGUMQxH3QJkL4ljM9HmU+V AUMA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=8cBOtfCDCEsR5MA2OR/N3UeiiPynUylFzfbN6r7N1Iw=; b=ubraQaKUMUnyy4vfW4Jk7b1T47n4ho1E66xDVrAH7X9R/58kTbG/x9rUigz7c8i4Da ZBWntLlFT1P/AntY5+rRtXZMY7KRp0bIKb89JRq//3twXe7t3kD4uZsSsOQ3POflno9M YPJ8io/FChbxIOVYu6GMvmJgfJCl+pX16LNCAbLPoeb5a5ZiXwvVK7VGjbqomICecyy8 MlFDahXm/fsT4yc/AQPYGt83Aq0lG2vDFRBf6e8kXT3TRsTOv+VPdvnQWjMk/1HS4HU2 2LgA7ltL3vZlnSjwE7N7pbVSmsNRzNt4+XR0sQCA1Q0rGy4f7slEfP06V6LwfRZShDrX LhEA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=dD7VcROX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id w19si4476032ook.8.2021.05.13.11.49.36; Thu, 13 May 2021 11:49:49 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=dD7VcROX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232487AbhEMLZb (ORCPT + 99 others); Thu, 13 May 2021 07:25:31 -0400 Received: from mail.kernel.org ([198.145.29.99]:40400 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232450AbhEMLZZ (ORCPT ); Thu, 13 May 2021 07:25:25 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id A7E87613DE; Thu, 13 May 2021 11:24:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1620905054; bh=jzzh35qDGkFoSJTAN06Jm3tfQzmk5CV26trrlgg59EQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dD7VcROXWD6E7T8dtHaIFbqf9fFpHi4QIi8aUjcZMwxD+0fLBFO4P1oh2R1dq4eiy epx0Yo4aBCJPBJOpU/xeByjcnzYhaSvZox03GCygcQo7WkhIxhji0lI6Bn8wcfV2Ls 8AY37DTxYLhVBRD+gl8TvPwv4vARpQpWVr5wVkPw= Date: Thu, 13 May 2021 13:24:12 +0200 From: Greg Kroah-Hartman To: Maciej Kwapulinski Cc: Arnd Bergmann , Jonathan Corbet , Derek Kiernan , Dragan Cvetic , Andy Shevchenko , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Tomasz Jankowski , Savo Novakovic , Jianxun Zhang Subject: Re: [PATCH v3 11/14] intel_gna: add ioctl handler Message-ID: References: <20210513110040.2268-1-maciej.kwapulinski@linux.intel.com> <20210513110040.2268-12-maciej.kwapulinski@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210513110040.2268-12-maciej.kwapulinski@linux.intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 13, 2021 at 01:00:37PM +0200, Maciej Kwapulinski wrote: > From: Tomasz Jankowski > > Add ioctl handler into GNA driver. > The ioctl interface provides the ability to do the following: > - Map and unmap memory buffers for GNA computation requests. > - Retrieve capabilities of the underlying GNA IP. > - Submit GNA computation requests. > - Request notification of scoring completion. Do you have a pointer to the userspace code that uses this ioctl? That's kind of required here, otherwise we have no idea how this all works. > > Signed-off-by: Tomasz Jankowski > Tested-by: Savo Novakovic > Co-developed-by: Jianxun Zhang > Signed-off-by: Jianxun Zhang > Co-developed-by: Maciej Kwapulinski > Signed-off-by: Maciej Kwapulinski > --- > drivers/misc/intel/gna/Kbuild | 2 +- > drivers/misc/intel/gna/device.c | 47 ++++++ > drivers/misc/intel/gna/device.h | 2 + > drivers/misc/intel/gna/ioctl.c | 257 ++++++++++++++++++++++++++++++++ > drivers/misc/intel/gna/ioctl.h | 11 ++ > include/uapi/misc/intel/gna.h | 53 +++++++ > 6 files changed, 371 insertions(+), 1 deletion(-) > create mode 100644 drivers/misc/intel/gna/ioctl.c > create mode 100644 drivers/misc/intel/gna/ioctl.h > > diff --git a/drivers/misc/intel/gna/Kbuild b/drivers/misc/intel/gna/Kbuild > index 38ff97360ed8..745a192a7304 100644 > --- a/drivers/misc/intel/gna/Kbuild > +++ b/drivers/misc/intel/gna/Kbuild > @@ -1,5 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0-only > > -intel_gna-y := device.o hw.o mem.o pci.o request.o score.o > +intel_gna-y := device.o hw.o ioctl.o mem.o pci.o request.o score.o > > obj-$(CONFIG_INTEL_GNA) += intel_gna.o > diff --git a/drivers/misc/intel/gna/device.c b/drivers/misc/intel/gna/device.c > index 75d8e1675485..0e31b8c6bb70 100644 > --- a/drivers/misc/intel/gna/device.c > +++ b/drivers/misc/intel/gna/device.c > @@ -6,8 +6,11 @@ > #include > #include > > +#include > + > #include "device.h" > #include "hw.h" > +#include "ioctl.h" > #include "request.h" > > static int recovery_timeout = 60; > @@ -145,6 +148,50 @@ int gna_probe(struct device *parent, struct gna_dev_info *dev_info, void __iomem > return 0; > } > > +static u32 gna_device_type_by_hwid(u32 hwid) > +{ > + switch (hwid) { > + case GNA_DEV_HWID_CNL: > + return GNA_DEV_TYPE_0_9; > + case GNA_DEV_HWID_GLK: > + case GNA_DEV_HWID_EHL: > + case GNA_DEV_HWID_ICL: > + return GNA_DEV_TYPE_1_0; > + case GNA_DEV_HWID_JSL: > + case GNA_DEV_HWID_TGL: > + case GNA_DEV_HWID_RKL: > + return GNA_DEV_TYPE_2_0; > + case GNA_DEV_HWID_ADL: > + case GNA_DEV_HWID_RPL: > + return GNA_DEV_TYPE_3_0; > + default: > + return 0; > + } > +} > + > +int gna_getparam(struct gna_private *gna_priv, union gna_parameter *param) > +{ > + switch (param->in.id) { > + case GNA_PARAM_DEVICE_ID: > + param->out.value = gna_priv->info.hwid; > + break; Why do you need an ioctl to get the device id? What's wrong with sysfs? > + case GNA_PARAM_RECOVERY_TIMEOUT: > + param->out.value = jiffies_to_msecs(gna_priv->recovery_timeout_jiffies) / 1000; > + break; > + case GNA_PARAM_INPUT_BUFFER_S: > + param->out.value = gna_priv->hw_info.in_buf_s; > + break; > + case GNA_PARAM_DEVICE_TYPE: > + param->out.value = gna_device_type_by_hwid(gna_priv->info.hwid); Same here, why isn't this a sysfs file? > + break; > + default: > + dev_err(gna_dev(gna_priv), "unknown parameter id %llu\n", param->in.id); Userspace can cause syslog DoS? Not nice :( Please just eat the error and move on. > + return -EINVAL; Wrong error value for invalid ioctl value. > + } > + > + return 0; > +} > + > MODULE_AUTHOR("Intel Corporation"); > MODULE_DESCRIPTION("Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA) Driver"); > MODULE_LICENSE("GPL"); > diff --git a/drivers/misc/intel/gna/device.h b/drivers/misc/intel/gna/device.h > index d3c86d649b5c..75784882f57c 100644 > --- a/drivers/misc/intel/gna/device.h > +++ b/drivers/misc/intel/gna/device.h > @@ -17,6 +17,7 @@ > #define GNA_DV_NAME "intel_gna" > > struct workqueue_struct; > +union gna_parameter; > struct device; > struct file; > > @@ -71,6 +72,7 @@ struct gna_private { > }; > > int gna_probe(struct device *parent, struct gna_dev_info *dev_info, void __iomem *iobase, int irq); > +int gna_getparam(struct gna_private *gna_priv, union gna_parameter *param); > > static inline u32 gna_reg_read(struct gna_private *gna_priv, u32 reg) > { > diff --git a/drivers/misc/intel/gna/ioctl.c b/drivers/misc/intel/gna/ioctl.c > new file mode 100644 > index 000000000000..4a90135b3cc6 > --- /dev/null > +++ b/drivers/misc/intel/gna/ioctl.c > @@ -0,0 +1,257 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright(c) 2017-2021 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include "device.h" > +#include "ioctl.h" > +#include "mem.h" > +#include "request.h" > +#include "score.h" > + > +static int gna_ioctl_score(struct gna_file_private *file_priv, void __user *argptr) > +{ > + union gna_compute score_args; > + struct gna_private *gna_priv; > + u64 request_id; > + int ret; > + > + gna_priv = file_priv->gna_priv; > + > + if (copy_from_user(&score_args, argptr, sizeof(score_args))) { > + dev_err(gna_dev(gna_priv), "could not copy score ioctl config from user\n"); No need for errors that userspace can cause, please drop, you already got a message if there needed to be one. > + return -EFAULT; > + } > + > + ret = gna_validate_score_config(&score_args.in.config, file_priv); This function is in a different patch? Now I have to dig through that to try to figure out if you really are validating the data properly? That's just mean to reviewers, would you want to review code like this? Please fix. > + if (ret) { > + dev_err(gna_dev(gna_priv), "request not valid\n"); Same here, clean up all error reporting in your ioctl to be none at all please. > + return ret; > + } > + > + ret = gna_enqueue_request(&score_args.in.config, file_priv, &request_id); Same here, where is this function to review? Same for all your other ioctl handlers, please fix up, this is rough to review... greg k-h