From: Tomasz Jankowski <[email protected]>
Add a new PCI driver for Intel(R) Gaussian & Neural Accelerator
with basic support like module loading and unloading. The full
function of the driver will be added by further changes.
Signed-off-by: Tomasz Jankowski <[email protected]>
Tested-by: Savo Novakovic <[email protected]>
Co-developed-by: Jianxun Zhang <[email protected]>
Signed-off-by: Jianxun Zhang <[email protected]>
Co-developed-by: Maciej Kwapulinski <[email protected]>
Signed-off-by: Maciej Kwapulinski <[email protected]>
---
Documentation/misc-devices/gna.rst | 48 ++++++
Documentation/misc-devices/index.rst | 1 +
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 7 +
drivers/misc/Kconfig | 1 +
drivers/misc/Makefile | 1 +
drivers/misc/gna/Kbuild | 5 +
drivers/misc/gna/Kconfig | 13 ++
drivers/misc/gna/gna_device.c | 100 +++++++++++
drivers/misc/gna/gna_device.h | 50 ++++++
drivers/misc/gna/gna_driver.c | 65 ++++++++
drivers/misc/gna/gna_driver.h | 41 +++++
include/uapi/misc/gna.h | 155 ++++++++++++++++++
13 files changed, 488 insertions(+)
create mode 100644 Documentation/misc-devices/gna.rst
create mode 100644 drivers/misc/gna/Kbuild
create mode 100644 drivers/misc/gna/Kconfig
create mode 100644 drivers/misc/gna/gna_device.c
create mode 100644 drivers/misc/gna/gna_device.h
create mode 100644 drivers/misc/gna/gna_driver.c
create mode 100644 drivers/misc/gna/gna_driver.h
create mode 100644 include/uapi/misc/gna.h
diff --git a/Documentation/misc-devices/gna.rst b/Documentation/misc-devices/gna.rst
new file mode 100644
index 000000000000..ed3d5a89271d
--- /dev/null
+++ b/Documentation/misc-devices/gna.rst
@@ -0,0 +1,48 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=====================================================
+Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)
+=====================================================
+
+Acronyms
+--------
+GNA - Gaussian & Neural Accelerator
+GMM - Gaussian Mixer Model
+CNN - Convolutional Neural Network
+RNN - Recurrent Neural Networks
+DNN - Deep Neural Networks
+
+Introduction
+------------
+The Intel(R) GNA is an Internal PCI fixed device available on several Intel platforms/SoCs.
+Feature set depends on the Intel Chipset SKU.
+
+Intel(R) GNA provides hardware accelerated computation for GMMs and Neural Networks.
+It supports several layer types: affine, recurrent, and convolutional among others.
+Hardware also provides helper layer types for copying and transposing matrices.
+
+Linux Driver
+------------
+Intel(R) GNA driver is a pci driver as Intel(R) GNA is a PCI device.
+The driver also registers a character device to expose file operations via dev node.
+
+The driver probes/removes PCI device, implements file operations, handles runtime
+power management, and interacts with hardware through MMIO registers.
+
+Multiple processes can independently file many requests to the driver. These requests are
+processed in a FIFO manner. The hardware can process one request at a time by using a FIFO
+queue.
+
+IOCTL
+-----
+Intel(R) GNA driver controls the device through IOCTL interfaces.
+Following IOCTL commands are supported:
+ GNA_IOCTL_PARAM_GET gets driver and device capabilities.
+
+ GNA_IOCTL_MEMORY_MAP lock user pages and GNA MMU setups for DMA transfer.
+
+ GNA_IOCTL_MEMORY_UNMAP unlocks user pages and releases GNA MMU structures.
+
+ GNA_IOCTL_COMPUTE submits a request to the device queue.
+
+ GNA_IOCTL_WAIT blocks and waits on the submitted request.
diff --git a/Documentation/misc-devices/index.rst b/Documentation/misc-devices/index.rst
index 64420b3314fe..8cc01280e555 100644
--- a/Documentation/misc-devices/index.rst
+++ b/Documentation/misc-devices/index.rst
@@ -19,6 +19,7 @@ fit into other categories.
bh1770glc
eeprom
c2port
+ gna
ibmvmc
ics932s401
isl29003
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index a4c75a28c839..9fad36a43f4a 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -115,6 +115,7 @@ Code Seq# Include File Comments
'B' C0-FF advanced bbus <mailto:[email protected]>
'C' all linux/soundcard.h conflict!
'C' 01-2F linux/capi.h conflict!
+'C' 01-5F uapi/misc/gna.h conflict!
'C' F0-FF drivers/net/wan/cosa.h conflict!
'D' all arch/s390/include/asm/dasd.h
'D' 40-5F drivers/scsi/dpt/dtpi_ioctl.h
diff --git a/MAINTAINERS b/MAINTAINERS
index cc1e6a5ee6e6..c117b872564b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8933,6 +8933,13 @@ S: Maintained
F: Documentation/fb/intelfb.rst
F: drivers/video/fbdev/intelfb/
+INTEL GNA PCI DRIVER
+M: Maciej Kwapulinski <[email protected]>
+S: Maintained
+F: Documentation/misc-devices/gna.rst
+F: drivers/misc/gna/*
+F: include/uapi/misc/gna.h
+
INTEL GPIO DRIVERS
M: Andy Shevchenko <[email protected]>
L: [email protected]
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0d8099..892cdf0ec935 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -481,4 +481,5 @@ source "drivers/misc/ocxl/Kconfig"
source "drivers/misc/cardreader/Kconfig"
source "drivers/misc/habanalabs/Kconfig"
source "drivers/misc/uacce/Kconfig"
+source "drivers/misc/gna/Kconfig"
endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d23231e73330..e756f760692d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_HABANA_AI) += habanalabs/
obj-$(CONFIG_UACCE) += uacce/
obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
+obj-$(CONFIG_INTEL_GNA) += gna/
diff --git a/drivers/misc/gna/Kbuild b/drivers/misc/gna/Kbuild
new file mode 100644
index 000000000000..863956d5761a
--- /dev/null
+++ b/drivers/misc/gna/Kbuild
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+gna-y := gna_device.o gna_driver.o
+
+obj-$(CONFIG_INTEL_GNA) += gna.o
diff --git a/drivers/misc/gna/Kconfig b/drivers/misc/gna/Kconfig
new file mode 100644
index 000000000000..9940f539d8af
--- /dev/null
+++ b/drivers/misc/gna/Kconfig
@@ -0,0 +1,13 @@
+#
+# Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)
+#
+
+config INTEL_GNA
+ tristate "Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)"
+ depends on X86_64 && PCI
+ help
+ This option enables the Intel(R) Gaussian & Neural Accelerator
+ (Intel(R) GNA) driver.
+ User space interface is defined in include/uapi/misc/gna.h, while
+ information about functionality is in
+ Documentation/misc-devices/gna.rst
diff --git a/drivers/misc/gna/gna_device.c b/drivers/misc/gna/gna_device.c
new file mode 100644
index 000000000000..a6ef7e790e9e
--- /dev/null
+++ b/drivers/misc/gna/gna_device.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright(c) 2017-2021 Intel Corporation
+
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#include "gna_device.h"
+#include "gna_driver.h"
+
+static int gna_dev_init(struct gna_private *gna_priv, struct pci_dev *pcidev,
+ const struct pci_device_id *pci_id)
+{
+ pci_set_drvdata(pcidev, gna_priv);
+
+ gna_priv->parent = &pcidev->dev;
+ gna_priv->pdev = pcidev;
+ gna_priv->info = *(struct gna_drv_info *)pci_id->driver_data;
+ gna_priv->drv_priv = &gna_drv_priv;
+
+ return 0;
+}
+
+/* Reverse gna_dev_init() */
+static void gna_dev_deinit(struct gna_private *gna_priv)
+{
+ pci_set_drvdata(gna_priv->pdev, NULL);
+}
+
+int gna_probe(struct pci_dev *pcidev, const struct pci_device_id *pci_id)
+{
+ struct gna_private *gna_priv;
+ int ret;
+
+ ret = pcim_enable_device(pcidev);
+ if (ret) {
+ dev_err(&pcidev->dev, "pci device can't be enabled\n");
+ goto end;
+ }
+
+ ret = pci_request_regions(pcidev, GNA_DRV_NAME);
+ if (ret)
+ goto end;
+
+ ret = pci_set_dma_mask(pcidev, DMA_BIT_MASK(64));
+ if (ret) {
+ dev_err(&pcidev->dev, "pci_set_dma_mask returned error %d\n", ret);
+ goto err_release_regions;
+ }
+
+ pci_set_master(pcidev);
+
+ /* init gna device */
+ gna_priv = devm_kzalloc(&pcidev->dev, sizeof(*gna_priv), GFP_KERNEL);
+ if (!gna_priv) {
+ ret = -ENOMEM;
+ goto err_clear_master;
+ }
+
+ /* Map BAR0 */
+ gna_priv->bar0.iostart = pci_resource_start(pcidev, 0);
+ gna_priv->bar0.iosize = pci_resource_len(pcidev, 0);
+ gna_priv->bar0.mem_addr = pcim_iomap(pcidev, 0, 0);
+ if (!gna_priv->bar0.mem_addr) {
+ dev_err(&pcidev->dev, "could not map BAR 0\n");
+ ret = -EINVAL;
+ goto err_clear_master;
+ }
+
+ dev_dbg(&pcidev->dev, "bar0 io start: 0x%llx\n", (unsigned long long)gna_priv->bar0.iostart);
+ dev_dbg(&pcidev->dev, "bar0 io size: %llu\n", (unsigned long long)gna_priv->bar0.iosize);
+ dev_dbg(&pcidev->dev, "bar0 memory address: %p\n", gna_priv->bar0.mem_addr);
+
+ ret = gna_dev_init(gna_priv, pcidev, pci_id);
+ if (ret) {
+ dev_err(&pcidev->dev, "could not initialize gna private structure\n");
+ goto err_clear_master;
+ }
+
+ return 0;
+
+err_clear_master:
+ pci_clear_master(pcidev);
+err_release_regions:
+ pci_release_regions(pcidev);
+end:
+ dev_err(&pcidev->dev, "gna probe failed with %d\n", ret);
+ return ret;
+}
+
+void gna_remove(struct pci_dev *pcidev)
+{
+ struct gna_private *gna_priv;
+
+ gna_priv = pci_get_drvdata(pcidev);
+
+ gna_dev_deinit(gna_priv);
+
+ pci_clear_master(pcidev);
+ pci_release_regions(pcidev);
+}
diff --git a/drivers/misc/gna/gna_device.h b/drivers/misc/gna/gna_device.h
new file mode 100644
index 000000000000..736bc5af5081
--- /dev/null
+++ b/drivers/misc/gna/gna_device.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2017-2021 Intel Corporation */
+
+#ifndef __GNA_DEVICE_H__
+#define __GNA_DEVICE_H__
+
+#include <linux/cdev.h>
+#include <linux/pci.h>
+#include <linux/types.h>
+
+#include <uapi/misc/gna.h>
+
+struct gna_drv_info {
+ u32 hwid;
+ u32 num_pagetables;
+ u32 num_page_entries;
+ u32 max_layer_count;
+ u64 max_hw_mem;
+};
+
+struct gna_pci_bar {
+ resource_size_t iostart;
+ resource_size_t iosize;
+ void __iomem *mem_addr;
+};
+
+struct gna_hw_info {
+ u8 in_buf_s;
+};
+
+struct gna_private {
+ struct gna_driver_private *drv_priv;
+
+ /* device objects */
+ struct pci_dev *pdev;
+ struct device *parent; /* pdev->dev */
+ struct device dev;
+ struct cdev cdev;
+
+ /* device related resources */
+ struct gna_pci_bar bar0;
+ struct gna_drv_info info;
+ struct gna_hw_info hw_info;
+};
+
+int gna_probe(struct pci_dev *dev, const struct pci_device_id *id);
+
+void gna_remove(struct pci_dev *dev);
+
+#endif /* __GNA_DEVICE_H__ */
diff --git a/drivers/misc/gna/gna_driver.c b/drivers/misc/gna/gna_driver.c
new file mode 100644
index 000000000000..81f0f003f377
--- /dev/null
+++ b/drivers/misc/gna/gna_driver.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright(c) 2017-2021 Intel Corporation
+
+#define pr_fmt(fmt) KBUILD_MODNAME " " fmt
+
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#include "gna_device.h"
+#include "gna_driver.h"
+
+struct gna_driver_private gna_drv_priv;
+
+struct class *gna_class;
+
+static struct pci_driver gna_driver = {
+ .name = GNA_DRV_NAME,
+ .probe = gna_probe,
+ .remove = gna_remove,
+};
+
+static char *gna_devnode(struct device *dev, umode_t *mode)
+{
+ if (mode)
+ *mode = 0666;
+
+ return kasprintf(GFP_KERNEL, "%s", dev_name(dev));
+}
+
+static int __init gna_drv_init(void)
+{
+ int ret;
+
+ mutex_init(&gna_drv_priv.lock);
+
+ gna_class = class_create(THIS_MODULE, "gna");
+ if (IS_ERR(gna_class)) {
+ pr_err("class device create failed\n");
+ return PTR_ERR(gna_class);
+ }
+ gna_class->devnode = gna_devnode;
+
+ ret = pci_register_driver(&gna_driver);
+ if (ret) {
+ pr_err("pci register driver failed\n");
+ class_destroy(gna_class);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit gna_drv_exit(void)
+{
+ pci_unregister_driver(&gna_driver);
+ class_destroy(gna_class);
+}
+
+module_init(gna_drv_init);
+module_exit(gna_drv_exit);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA) Driver");
+MODULE_VERSION(GNA_DRV_VER);
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/gna/gna_driver.h b/drivers/misc/gna/gna_driver.h
new file mode 100644
index 000000000000..4cf144bef8d4
--- /dev/null
+++ b/drivers/misc/gna/gna_driver.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright(c) 2017-2021 Intel Corporation */
+
+#ifndef __GNA_DRIVER_H__
+#define __GNA_DRIVER_H__
+
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#define GNA_DRV_NAME "gna"
+#define GNA_DRV_VER "1.2.0"
+
+#define GNA_MAX_DEVICES 16
+
+struct gna_driver_private {
+ /* device major/minor number facilities */
+ DECLARE_BITMAP(dev_map, GNA_MAX_DEVICES);
+ dev_t devt;
+ int minor;
+
+ struct mutex lock; /* protects this structure */
+};
+
+struct gna_file_private {
+ struct file *fd;
+ struct gna_private *gna_priv;
+
+ struct list_head memory_list;
+ struct mutex memlist_lock; /* protects memory_list */
+
+ struct list_head flist;
+};
+
+extern struct gna_driver_private gna_drv_priv;
+
+extern struct class *gna_class;
+
+extern int recovery_timeout;
+
+#endif /* __GNA_DRIVER_H__ */
diff --git a/include/uapi/misc/gna.h b/include/uapi/misc/gna.h
new file mode 100644
index 000000000000..cd292a85eec6
--- /dev/null
+++ b/include/uapi/misc/gna.h
@@ -0,0 +1,155 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+/* Copyright(c) 2017-2021 Intel Corporation */
+
+#ifndef _UAPI_GNA_H_
+#define _UAPI_GNA_H_
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#ifndef __user
+#define __user
+#endif
+
+/* Operation modes */
+#define GNA_MODE_GMM 0
+#define GNA_MODE_XNN 1
+
+#define GNA_PARAM_DEVICE_ID 1
+#define GNA_PARAM_RECOVERY_TIMEOUT 2
+#define GNA_PARAM_DEVICE_TYPE 3
+#define GNA_PARAM_INPUT_BUFFER_S 4
+
+#define GNA_STS_SCORE_COMPLETED (1 << 0)
+#define GNA_STS_STATISTICS_VALID (1 << 3)
+#define GNA_STS_PCI_MMU_ERR (1 << 4)
+#define GNA_STS_PCI_DMA_ERR (1 << 5)
+#define GNA_STS_PCI_UNEXCOMPL_ERR (1 << 6)
+#define GNA_STS_VA_OOR (1 << 7)
+#define GNA_STS_PARAM_OOR (1 << 8)
+#define GNA_STS_OUTBUF_FULL (1 << 16)
+#define GNA_STS_SATURATE (1 << 17)
+
+#define GNA_ERROR (GNA_STS_PCI_DMA_ERR | \
+ GNA_STS_PCI_MMU_ERR | \
+ GNA_STS_PCI_UNEXCOMPL_ERR | \
+ GNA_STS_PARAM_OOR | \
+ GNA_STS_VA_OOR)
+
+#define GNA_DEV_TYPE_0_9 0x09
+#define GNA_DEV_TYPE_1_0 0x10
+#define GNA_DEV_TYPE_2_0 0x20
+
+/*
+ * Structure describes part of memory to be overwritten before starting GNA
+ */
+struct gna_memory_patch {
+ /* offset from targeted memory */
+ __u64 offset;
+
+ __u64 size;
+ __u64 value;
+};
+
+struct gna_buffer {
+ __u64 memory_id;
+
+ __u64 offset;
+ __u64 size;
+
+ __u64 patch_count;
+ __u64 patches_ptr;
+};
+
+/*
+ * Driver performance timestamps in nanoseconds.
+ * Values regard system boot time, but do not count during suspend.
+ */
+struct gna_drv_perf {
+ __u64 pre_processing; /* driver starts pre-processing */
+ __u64 processing; /* hw starts processing */
+ __u64 hw_completed; /* hw finishes processing */
+ __u64 completion; /* driver finishes post-processing */
+};
+
+struct gna_hw_perf {
+ __u64 total;
+ __u64 stall;
+};
+
+struct gna_compute_cfg {
+ __u32 layer_base;
+ __u32 layer_count;
+
+ /* List of GNA memory buffers */
+ __u64 buffers_ptr;
+ __u64 buffer_count;
+
+ __u8 active_list_on;
+ __u8 gna_mode;
+ __u8 hw_perf_encoding;
+ __u8 pad[5];
+};
+
+union gna_parameter {
+ struct {
+ __u64 id;
+ } in;
+
+ struct {
+ __u64 value;
+ } out;
+};
+
+union gna_memory_map {
+ struct {
+ __u64 address;
+ __u32 size;
+ __u32 pad;
+ } in;
+
+ struct {
+ __u64 memory_id;
+ } out;
+};
+
+union gna_compute {
+ struct {
+ struct gna_compute_cfg config;
+ } in;
+
+ struct {
+ __u64 request_id;
+ } out;
+};
+
+union gna_wait {
+ struct {
+ __u64 request_id;
+ __u32 timeout;
+ __u32 pad;
+ } in;
+
+ struct {
+ __u32 hw_status;
+ __u32 pad;
+ struct gna_drv_perf drv_perf;
+ struct gna_hw_perf hw_perf;
+ } out;
+};
+
+#define GNA_GET_PARAMETER _IOWR('C', 0x01, union gna_parameter)
+#define GNA_MAP_MEMORY _IOWR('C', 0x02, union gna_memory_map)
+#define GNA_UNMAP_MEMORY _IOWR('C', 0x03, __u64)
+#define GNA_COMPUTE _IOWR('C', 0x04, union gna_compute)
+#define GNA_WAIT _IOWR('C', 0x05, union gna_wait)
+
+#if defined(__cplusplus)
+}
+#endif
+
+#endif /* _UAPI_GNA_H_ */
--
2.28.0
On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
<[email protected]> wrote:
>
> From: Tomasz Jankowski <[email protected]>
>
> Add a new PCI driver for Intel(R) Gaussian & Neural Accelerator
> with basic support like module loading and unloading. The full
> function of the driver will be added by further changes.
...
> +config INTEL_GNA
> + tristate "Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)"
Intel (R) Intel (R) RRR!
> + depends on X86_64 && PCI
> + help
> + This option enables the Intel(R) Gaussian & Neural Accelerator
> + (Intel(R) GNA) driver.
> + User space interface is defined in include/uapi/misc/gna.h, while
> + information about functionality is in
> + Documentation/misc-devices/gna.rst
No module name?
...
> +/* Reverse gna_dev_init() */
> +static void gna_dev_deinit(struct gna_private *gna_priv)
> +{
> + pci_set_drvdata(gna_priv->pdev, NULL);
> +}
This is done by device core. Why do you need it?
...
> + ret = pcim_enable_device(pcidev);
> + if (ret) {
> + dev_err(&pcidev->dev, "pci device can't be enabled\n");
> + goto end;
Useless label. Return here.
> + }
...
> + ret = pci_request_regions(pcidev, GNA_DRV_NAME);
> + if (ret)
> + goto end;
Why? Can't you use pcim_iomap_regions() directly?
...
> + ret = pci_set_dma_mask(pcidev, DMA_BIT_MASK(64));
No way. This is an obsoleted API.
> + if (ret) {
> + dev_err(&pcidev->dev, "pci_set_dma_mask returned error %d\n", ret);
> + goto err_release_regions;
> + }
...
> + /* init gna device */
Useless comments here and there.
...
> + gna_priv = devm_kzalloc(&pcidev->dev, sizeof(*gna_priv), GFP_KERNEL);
> + if (!gna_priv) {
> + ret = -ENOMEM;
> + goto err_clear_master;
What? You have used pciM_enabled_device(). Please, read documentation.
> + }
...
> + gna_priv->bar0.iostart = pci_resource_start(pcidev, 0);
> + gna_priv->bar0.iosize = pci_resource_len(pcidev, 0);
> + gna_priv->bar0.mem_addr = pcim_iomap(pcidev, 0, 0);
> + if (!gna_priv->bar0.mem_addr) {
> + dev_err(&pcidev->dev, "could not map BAR 0\n");
> + ret = -EINVAL;
> + goto err_clear_master;
> + }
Why do you need all these?!
...
> + dev_dbg(&pcidev->dev, "bar0 io start: 0x%llx\n", (unsigned long long)gna_priv->bar0.iostart);
> + dev_dbg(&pcidev->dev, "bar0 io size: %llu\n", (unsigned long long)gna_priv->bar0.iosize);
> + dev_dbg(&pcidev->dev, "bar0 memory address: %p\n", gna_priv->bar0.mem_addr);
No, please read printk-formats.rst.
...
> +err_clear_master:
> + pci_clear_master(pcidev);
> +err_release_regions:
> + pci_release_regions(pcidev);
> +end:
> + dev_err(&pcidev->dev, "gna probe failed with %d\n", ret);
> + return ret;
These are all completely redundant.
> +}
...
> +void gna_remove(struct pci_dev *pcidev)
> +{
> + struct gna_private *gna_priv;
> +
> + gna_priv = pci_get_drvdata(pcidev);
> +
> + gna_dev_deinit(gna_priv);
> +
> + pci_clear_master(pcidev);
> + pci_release_regions(pcidev);
> +}
Redundant entire function.
...
> +#include <linux/pci.h>
Haven't noticed how this header is used here.
...
> + struct device dev;
Missed linux/device.h.
...
> +static int __init gna_drv_init(void)
> +{
> + int ret;
> +
> + mutex_init(&gna_drv_priv.lock);
> +
> + gna_class = class_create(THIS_MODULE, "gna");
> + if (IS_ERR(gna_class)) {
> + pr_err("class device create failed\n");
> + return PTR_ERR(gna_class);
> + }
> + gna_class->devnode = gna_devnode;
> +
> + ret = pci_register_driver(&gna_driver);
Is it possible to decouple a PCI glue driver from the class as many
other existing examples are doing?
> + if (ret) {
> + pr_err("pci register driver failed\n");
> + class_destroy(gna_class);
> + return ret;
> + }
> +
> + return 0;
> +}
...
> +#include <linux/kernel.h>
Why do you need this header?
...
> +#define GNA_DRV_VER "1.2.0"
Nowadays the version is the Git SHA sum.
...
> + struct file *fd;
Missed forward declaration.
...
> + struct list_head memory_list;
Missed linux/list.h
> + struct list_head flist;
...
> +extern struct gna_driver_private gna_drv_priv;
> +extern int recovery_timeout;
Global?!
...
> +#define GNA_STS_SCORE_COMPLETED (1 << 0)
> +#define GNA_STS_STATISTICS_VALID (1 << 3)
> +#define GNA_STS_PCI_MMU_ERR (1 << 4)
> +#define GNA_STS_PCI_DMA_ERR (1 << 5)
> +#define GNA_STS_PCI_UNEXCOMPL_ERR (1 << 6)
> +#define GNA_STS_VA_OOR (1 << 7)
> +#define GNA_STS_PARAM_OOR (1 << 8)
> +#define GNA_STS_OUTBUF_FULL (1 << 16)
> +#define GNA_STS_SATURATE (1 << 17)
You can use _BITUL() from const.h, but it's up to you.
...
> +#define GNA_ERROR (GNA_STS_PCI_DMA_ERR | \
When definitions start on the next line it will be easier to read.
> + GNA_STS_PCI_MMU_ERR | \
> + GNA_STS_PCI_UNEXCOMPL_ERR | \
> + GNA_STS_PARAM_OOR | \
> + GNA_STS_VA_OOR)
--
With Best Regards,
Andy Shevchenko
> On Feb 16, 2021, at 8:54 AM, Andy Shevchenko <[email protected]> wrote:
>
>> +config INTEL_GNA
>> + tristate "Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)"
>
> Intel (R) Intel (R) RRR!
This is (from my interpretation) of requirements and guidance specific on how to address this HW IP from Intel’s legal before upstream.
On Tue, Feb 16, 2021 at 05:05:14PM +0100, Maciej Kwapulinski wrote:
> --- /dev/null
> +++ b/drivers/misc/gna/gna_driver.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright(c) 2017-2021 Intel Corporation
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME " " fmt
You are a driver, you should never need a pr_* call, so this should not
be needed. You should always just use dev_* instead.
> --- /dev/null
> +++ b/drivers/misc/gna/gna_driver.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2017-2021 Intel Corporation */
> +
> +#ifndef __GNA_DRIVER_H__
> +#define __GNA_DRIVER_H__
> +
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +#define GNA_DRV_NAME "gna"
Way too generic, no one knows what "gna" is.
> +#define GNA_DRV_VER "1.2.0"
As Andy said, this means nothing within the kernel (or really, outside
the kernel either), so please drop.
> +
> +#define GNA_MAX_DEVICES 16
Why 16?
And if that's all, then just use the misc device api, that saves you so
much overhead and mess and you don't have to worry about sysfs and
classes or anything like that at all.
thanks,
greg k-h
Hi--
On 2/16/21 8:05 AM, Maciej Kwapulinski wrote:
> diff --git a/Documentation/misc-devices/gna.rst b/Documentation/misc-devices/gna.rst
> new file mode 100644
> index 000000000000..ed3d5a89271d
> --- /dev/null
> +++ b/Documentation/misc-devices/gna.rst
> @@ -0,0 +1,48 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=====================================================
> +Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)
> +=====================================================
> +
> +Acronyms
> +--------
> +GNA - Gaussian & Neural Accelerator
> +GMM - Gaussian Mixer Model
> +CNN - Convolutional Neural Network
> +RNN - Recurrent Neural Networks
> +DNN - Deep Neural Networks
> +
> +Introduction
> +------------
> +The Intel(R) GNA is an Internal PCI fixed device available on several Intel platforms/SoCs.
internal
> +Feature set depends on the Intel Chipset SKU.
chipset
> +
> +Intel(R) GNA provides hardware accelerated computation for GMMs and Neural Networks.
> +It supports several layer types: affine, recurrent, and convolutional among others.
> +Hardware also provides helper layer types for copying and transposing matrices.
> +
> +Linux Driver
> +------------
> +Intel(R) GNA driver is a pci driver as Intel(R) GNA is a PCI device.
PCI
although that entire sentence seems to be unneeded IMO.
> +The driver also registers a character device to expose file operations via dev node.
> +
> +The driver probes/removes PCI device, implements file operations, handles runtime
> +power management, and interacts with hardware through MMIO registers.
> +
> +Multiple processes can independently file many requests to the driver. These requests are
> +processed in a FIFO manner. The hardware can process one request at a time by using a FIFO
> +queue.
> +
> +IOCTL
> +-----
> +Intel(R) GNA driver controls the device through IOCTL interfaces.
> +Following IOCTL commands are supported:
> + GNA_IOCTL_PARAM_GET gets driver and device capabilities.
> +
> + GNA_IOCTL_MEMORY_MAP lock user pages and GNA MMU setups for DMA transfer.
locks
> +
> + GNA_IOCTL_MEMORY_UNMAP unlocks user pages and releases GNA MMU structures.
> +
> + GNA_IOCTL_COMPUTE submits a request to the device queue.
> +
> + GNA_IOCTL_WAIT blocks and waits on the submitted request.
--
~Randy
Greg Kroah-Hartman <[email protected]> writes:
> On Tue, Feb 16, 2021 at 05:05:14PM +0100, Maciej Kwapulinski wrote:
>> --- /dev/null
>> +++ b/drivers/misc/gna/gna_driver.c
>> @@ -0,0 +1,65 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright(c) 2017-2021 Intel Corporation
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME " " fmt
>
> You are a driver, you should never need a pr_* call, so this should not
> be needed. You should always just use dev_* instead.
>
Hi Greg and all other Reviewers.
Thank You for all the comments so far.
I'm starting preparing PATCH v2 series based on them.
I'll also answer comments individually where need arises.
regards,
Maciej
Andy Shevchenko <[email protected]> writes:
> On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> <[email protected]> wrote:
>>
....
>> +err_clear_master:
>> + pci_clear_master(pcidev);
>> +err_release_regions:
>> + pci_release_regions(pcidev);
>> +end:
>> + dev_err(&pcidev->dev, "gna probe failed with %d\n", ret);
>> + return ret;
>
> These are all completely redundant.
>
following is refactor of gna_probe(), but without pci_release_regions(),
smatch (v7fcfe259) produces warning:
drivers/misc/gna/gna_device.c:78 gna_probe() warn: 'pcidev' not
released on lines: 56,65.
here's the code refactored:
int gna_probe(struct pci_dev *pcidev, const struct pci_device_id *pci_id)
{
struct gna_private *gna_priv;
int ret;
ret = pcim_enable_device(pcidev);
if (ret) {
dev_err(&pcidev->dev, "pci device can't be enabled\n");
return ret;
}
ret = pci_request_regions(pcidev, GNA_DRV_NAME);
if (ret)
return ret;
ret = pci_set_dma_mask(pcidev, DMA_BIT_MASK(64));
if (ret) {
dev_err(&pcidev->dev, "pci_set_dma_mask returned error %d\n", ret);
return ret;
}
pci_set_master(pcidev);
/* init gna device */
gna_priv = devm_kzalloc(&pcidev->dev, sizeof(*gna_priv), GFP_KERNEL);
if (!gna_priv) {
//pci_release_regions(pcidev);
return -ENOMEM; // line 56
}
/* Map BAR0 */
gna_priv->bar0.iostart = pci_resource_start(pcidev, 0);
gna_priv->bar0.iosize = pci_resource_len(pcidev, 0);
gna_priv->bar0.mem_addr = pcim_iomap(pcidev, 0, 0);
if (!gna_priv->bar0.mem_addr) {
//pci_release_regions(pcidev);
dev_err(&pcidev->dev, "could not map BAR 0\n");
return -EINVAL; // line 65
}
dev_dbg(&pcidev->dev, "bar0 io start: 0x%llx\n", (unsigned long long)gna_priv->bar0.iostart);
dev_dbg(&pcidev->dev, "bar0 io size: %llu\n", (unsigned long long)gna_priv->bar0.iosize);
dev_dbg(&pcidev->dev, "bar0 memory address: %p\n", gna_priv->bar0.mem_addr);
ret = gna_dev_init(gna_priv, pcidev, pci_id);
if (ret) {
dev_err(&pcidev->dev, "could not initialize gna private structure\n");
return ret;
}
return 0;
}
I've also added 'noinline' directive to pci_release_regions(), to see if
it is called by the core code on "rmmod gna", but can't see the call.
Is the smatch tool that causes problems here?
Do You suggest other way to handle the problem?
On Fri, Feb 19, 2021 at 3:21 PM Maciej Kwapulinski
<[email protected]> wrote:
> Andy Shevchenko <[email protected]> writes:
> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> > <[email protected]> wrote:
> >>
> ....
> >> +err_clear_master:
> >> + pci_clear_master(pcidev);
> >> +err_release_regions:
> >> + pci_release_regions(pcidev);
> >> +end:
> >> + dev_err(&pcidev->dev, "gna probe failed with %d\n", ret);
> >> + return ret;
> >
> > These are all completely redundant.
> >
>
> following is refactor of gna_probe(), but without pci_release_regions(),
> smatch (v7fcfe259) produces warning:
> drivers/misc/gna/gna_device.c:78 gna_probe() warn: 'pcidev' not
> released on lines: 56,65.
Then the tool is mistaken.
> here's the code refactored:
>
> int gna_probe(struct pci_dev *pcidev, const struct pci_device_id *pci_id)
> {
> struct gna_private *gna_priv;
> int ret;
>
> ret = pcim_enable_device(pcidev);
> if (ret) {
> dev_err(&pcidev->dev, "pci device can't be enabled\n");
> return ret;
> }
>
> ret = pci_request_regions(pcidev, GNA_DRV_NAME);
> if (ret)
> return ret;
>
> ret = pci_set_dma_mask(pcidev, DMA_BIT_MASK(64));
> if (ret) {
> dev_err(&pcidev->dev, "pci_set_dma_mask returned error %d\n", ret);
> return ret;
> }
>
> pci_set_master(pcidev);
>
> /* init gna device */
> gna_priv = devm_kzalloc(&pcidev->dev, sizeof(*gna_priv), GFP_KERNEL);
> if (!gna_priv) {
> //pci_release_regions(pcidev);
> return -ENOMEM; // line 56
> }
> /* Map BAR0 */
> gna_priv->bar0.iostart = pci_resource_start(pcidev, 0);
> gna_priv->bar0.iosize = pci_resource_len(pcidev, 0);
> gna_priv->bar0.mem_addr = pcim_iomap(pcidev, 0, 0);
> if (!gna_priv->bar0.mem_addr) {
> //pci_release_regions(pcidev);
> dev_err(&pcidev->dev, "could not map BAR 0\n");
> return -EINVAL; // line 65
> }
>
> dev_dbg(&pcidev->dev, "bar0 io start: 0x%llx\n", (unsigned long long)gna_priv->bar0.iostart);
> dev_dbg(&pcidev->dev, "bar0 io size: %llu\n", (unsigned long long)gna_priv->bar0.iosize);
> dev_dbg(&pcidev->dev, "bar0 memory address: %p\n", gna_priv->bar0.mem_addr);
>
> ret = gna_dev_init(gna_priv, pcidev, pci_id);
> if (ret) {
> dev_err(&pcidev->dev, "could not initialize gna private structure\n");
> return ret;
> }
>
> return 0;
> }
>
> I've also added 'noinline' directive to pci_release_regions(), to see if
> it is called by the core code on "rmmod gna", but can't see the call.
>
> Is the smatch tool that causes problems here?
> Do You suggest other way to handle the problem?
File a bug against that tool.
Before you are doing v2, I would suggest you to go thru all comments
and address them. Above "refactored" code doesn't even have a half of
the comments I gave being addressed. If you have concerns, please,
reply to the review email with a specific excerpt as a context.
--
With Best Regards,
Andy Shevchenko
Greg Kroah-Hartman <[email protected]> writes:
> On Tue, Feb 16, 2021 at 05:05:14PM +0100, Maciej Kwapulinski wrote:
....
>> --- /dev/null
>> +++ b/drivers/misc/gna/gna_driver.h
>> @@ -0,0 +1,41 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/* Copyright(c) 2017-2021 Intel Corporation */
>> +
>> +#ifndef __GNA_DRIVER_H__
>> +#define __GNA_DRIVER_H__
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +#define GNA_DRV_NAME "gna"
>
> Way too generic, no one knows what "gna" is.
>
"intel gna" is much more verbose in search engines.
As we do not (plan to) have more "gna" drivers, is the following ok?:
intel-gna
the change would imply the following:
prompt$ lspci -s 00:00.3 -vvvv
00:00.3 System peripheral: Intel Corporation Device 3190 (rev 03)
Subsystem: Intel Corporation Device 2072
....
Kernel driver in use: intel-gna
Kernel modules: gna
is it ok?
also, how about the interface to library (it's part of one of next patches)?:
prompt$ file /dev/gna0
/dev/gna0: character special (235/0)
can "gna" stay intact here?
I'm pointing this out, because gna exists on the market for a while and
changing the above may have some impact we'd like to avoid.
>
....
On Fri, Feb 26, 2021 at 01:59:14PM +0100, Maciej Kwapulinski wrote:
>
> Greg Kroah-Hartman <[email protected]> writes:
>
> > On Tue, Feb 16, 2021 at 05:05:14PM +0100, Maciej Kwapulinski wrote:
> ....
> >> --- /dev/null
> >> +++ b/drivers/misc/gna/gna_driver.h
> >> @@ -0,0 +1,41 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/* Copyright(c) 2017-2021 Intel Corporation */
> >> +
> >> +#ifndef __GNA_DRIVER_H__
> >> +#define __GNA_DRIVER_H__
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/types.h>
> >> +
> >> +#define GNA_DRV_NAME "gna"
> >
> > Way too generic, no one knows what "gna" is.
> >
>
> "intel gna" is much more verbose in search engines.
> As we do not (plan to) have more "gna" drivers, is the following ok?:
>
> intel-gna
>
> the change would imply the following:
>
> prompt$ lspci -s 00:00.3 -vvvv
> 00:00.3 System peripheral: Intel Corporation Device 3190 (rev 03)
> Subsystem: Intel Corporation Device 2072
> ....
> Kernel driver in use: intel-gna
> Kernel modules: gna
>
> is it ok?
Why not intel-gna as the kernel module as well?
> also, how about the interface to library (it's part of one of next patches)?:
> prompt$ file /dev/gna0
> /dev/gna0: character special (235/0)
>
> can "gna" stay intact here?
Again, I have no idea what "gna" is, so you might want to pick something
more descriptive?
> I'm pointing this out, because gna exists on the market for a while and
> changing the above may have some impact we'd like to avoid.
If it exists but Linux does not support it, how would anyone know about
it? :)
Please use real terms where possible.
thanks,
greg k-h
Andy Shevchenko <[email protected]> writes:
> On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> <[email protected]> wrote:
>>
....
>> +#define GNA_DRV_VER "1.2.0"
>
> Nowadays the version is the Git SHA sum.
>
right, "version" is present in about 7% of all modules
do You mean version should be skipped over (automatically generated)
srcversion field? or maybe You suggest any (better) technique?
> On Feb 26, 2021, at 10:29 AM, Maciej Kwapulinski <[email protected]> wrote:
>
>
> Andy Shevchenko <[email protected]> writes:
>
>> On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
>> <[email protected]> wrote:
>>>
> ....
>>> +#define GNA_DRV_VER "1.2.0"
>>
>> Nowadays the version is the Git SHA sum.
>>
>
> right, "version" is present in about 7% of all modules
>
> do You mean version should be skipped over (automatically generated)
> srcversion field? or maybe You suggest any (better) technique?
Just my 0.02. We should identify who will consume this version string for what purposes. Then we can decide if a hardcoded macro or any auto-gen tags should be used. If we don’t find such need, perhaps we just remove it since a snapshot of the code is always tagged by commit SHA1 in git. (maybe this is what Andy suggested?).
Having such hardcoded version string requires an update policy in return, to make it really useful, IMO.
On Fri, Feb 26, 2021 at 07:29:39PM +0100, Maciej Kwapulinski wrote:
>
> Andy Shevchenko <[email protected]> writes:
>
> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> > <[email protected]> wrote:
> >>
> ....
> >> +#define GNA_DRV_VER "1.2.0"
> >
> > Nowadays the version is the Git SHA sum.
> >
>
> right, "version" is present in about 7% of all modules
And it should be removed from them.
> do You mean version should be skipped over (automatically generated)
> srcversion field? or maybe You suggest any (better) technique?
Drop it entirely from the driver code, it means nothing when the driver
is merged into the kernel tree.
thanks,
greg k-h
Greg Kroah-Hartman <[email protected]> writes:
> On Mon, Mar 01, 2021 at 11:18:59AM +0100, Maciej Kwapulinski wrote:
>>
>> Andy Shevchenko <[email protected]> writes:
>>
>> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
>> > <[email protected]> wrote:
>> >>
>> ....
>> >> +static int __init gna_drv_init(void)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + mutex_init(&gna_drv_priv.lock);
>> >> +
>> >> + gna_class = class_create(THIS_MODULE, "gna");
>> >> + if (IS_ERR(gna_class)) {
>> >> + pr_err("class device create failed\n");
>> >> + return PTR_ERR(gna_class);
>> >> + }
>> >> + gna_class->devnode = gna_devnode;
>> >> +
>> >> + ret = pci_register_driver(&gna_driver);
>> >
>> > Is it possible to decouple a PCI glue driver from the class as many
>> > other existing examples are doing?
>> >
>>
>> I see many pci drivers (including staging) that do have it glued though.
>>
>> Examples are:
>> 1. "static int __init kp2000_pcie_init(void)" (commit on May 20 09:34:11
>> 2019)
>> 2. "static int __init hl_init(void)" (commit on Mon Feb 18 09:46:43 2019)
>>
>> Please give me more details.
>
> Never use a staging driver for any type of example, _EXECPT_ for a bad
> one. There's a reason the code is in staging and not in the "real" part
> of the kernel.
ok.
another one (1) is not staging..
Maciej Kwapulinski <[email protected]> writes:
> Greg Kroah-Hartman <[email protected]> writes:
>
>> On Mon, Mar 01, 2021 at 11:18:59AM +0100, Maciej Kwapulinski wrote:
>>>
>>> Andy Shevchenko <[email protected]> writes:
>>>
>>> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
>>> > <[email protected]> wrote:
>>> >>
>>> ....
>>> >> +static int __init gna_drv_init(void)
>>> >> +{
>>> >> + int ret;
>>> >> +
>>> >> + mutex_init(&gna_drv_priv.lock);
>>> >> +
>>> >> + gna_class = class_create(THIS_MODULE, "gna");
>>> >> + if (IS_ERR(gna_class)) {
>>> >> + pr_err("class device create failed\n");
>>> >> + return PTR_ERR(gna_class);
>>> >> + }
>>> >> + gna_class->devnode = gna_devnode;
>>> >> +
>>> >> + ret = pci_register_driver(&gna_driver);
>>> >
>>> > Is it possible to decouple a PCI glue driver from the class as many
>>> > other existing examples are doing?
>>> >
>>>
>>> I see many pci drivers (including staging) that do have it glued though.
>>>
>>> Examples are:
>>> 1. "static int __init kp2000_pcie_init(void)" (commit on May 20 09:34:11
>>> 2019)
>>> 2. "static int __init hl_init(void)" (commit on Mon Feb 18 09:46:43 2019)
>>>
>>> Please give me more details.
>>
>> Never use a staging driver for any type of example, _EXECPT_ for a bad
>> one. There's a reason the code is in staging and not in the "real" part
>> of the kernel.
>
> ok.
>
> another one (1) is not staging..
I meant "static int __init hl_init(void)" is not staging one....
Greg Kroah-Hartman <[email protected]> writes:
> On Mon, Mar 01, 2021 at 11:39:23AM +0100, Maciej Kwapulinski wrote:
>>
>> Maciej Kwapulinski <[email protected]> writes:
>>
>> > Greg Kroah-Hartman <[email protected]> writes:
>> >
>> >> On Mon, Mar 01, 2021 at 11:18:59AM +0100, Maciej Kwapulinski wrote:
>> >>>
>> >>> Andy Shevchenko <[email protected]> writes:
>> >>>
>> >>> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
>> >>> > <[email protected]> wrote:
>> >>> >>
>> >>> ....
>> >>> >> +static int __init gna_drv_init(void)
>> >>> >> +{
>> >>> >> + int ret;
>> >>> >> +
>> >>> >> + mutex_init(&gna_drv_priv.lock);
>> >>> >> +
>> >>> >> + gna_class = class_create(THIS_MODULE, "gna");
>> >>> >> + if (IS_ERR(gna_class)) {
>> >>> >> + pr_err("class device create failed\n");
>> >>> >> + return PTR_ERR(gna_class);
>> >>> >> + }
>> >>> >> + gna_class->devnode = gna_devnode;
>> >>> >> +
>> >>> >> + ret = pci_register_driver(&gna_driver);
>> >>> >
>> >>> > Is it possible to decouple a PCI glue driver from the class as many
>> >>> > other existing examples are doing?
>> >>> >
>> >>>
>> >>> I see many pci drivers (including staging) that do have it glued though.
>> >>>
>> >>> Examples are:
>> >>> 1. "static int __init kp2000_pcie_init(void)" (commit on May 20 09:34:11
>> >>> 2019)
>> >>> 2. "static int __init hl_init(void)" (commit on Mon Feb 18 09:46:43 2019)
>> >>>
>> >>> Please give me more details.
>> >>
>> >> Never use a staging driver for any type of example, _EXECPT_ for a bad
>> >> one. There's a reason the code is in staging and not in the "real" part
>> >> of the kernel.
>> >
>> > ok.
>> >
>> > another one (1) is not staging..
>>
>> I meant "static int __init hl_init(void)" is not staging one....
>
> Still doesn't mean it is a good thing to do. Again, why isn't this
> driver just using the misc driver interface instead? It's much simpler
> to use and should work just fine for this tiny driver, instead of having
> to create a custom class just for it.
>
ok
Andy Shevchenko <[email protected]> writes:
> On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> <[email protected]> wrote:
>>
....
>> +static int __init gna_drv_init(void)
>> +{
>> + int ret;
>> +
>> + mutex_init(&gna_drv_priv.lock);
>> +
>> + gna_class = class_create(THIS_MODULE, "gna");
>> + if (IS_ERR(gna_class)) {
>> + pr_err("class device create failed\n");
>> + return PTR_ERR(gna_class);
>> + }
>> + gna_class->devnode = gna_devnode;
>> +
>> + ret = pci_register_driver(&gna_driver);
>
> Is it possible to decouple a PCI glue driver from the class as many
> other existing examples are doing?
>
I see many pci drivers (including staging) that do have it glued though.
Examples are:
1. "static int __init kp2000_pcie_init(void)" (commit on May 20 09:34:11
2019)
2. "static int __init hl_init(void)" (commit on Mon Feb 18 09:46:43 2019)
Please give me more details.
On Mon, Mar 01, 2021 at 11:18:59AM +0100, Maciej Kwapulinski wrote:
>
> Andy Shevchenko <[email protected]> writes:
>
> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> > <[email protected]> wrote:
> >>
> ....
> >> +static int __init gna_drv_init(void)
> >> +{
> >> + int ret;
> >> +
> >> + mutex_init(&gna_drv_priv.lock);
> >> +
> >> + gna_class = class_create(THIS_MODULE, "gna");
> >> + if (IS_ERR(gna_class)) {
> >> + pr_err("class device create failed\n");
> >> + return PTR_ERR(gna_class);
> >> + }
> >> + gna_class->devnode = gna_devnode;
> >> +
> >> + ret = pci_register_driver(&gna_driver);
> >
> > Is it possible to decouple a PCI glue driver from the class as many
> > other existing examples are doing?
> >
>
> I see many pci drivers (including staging) that do have it glued though.
>
> Examples are:
> 1. "static int __init kp2000_pcie_init(void)" (commit on May 20 09:34:11
> 2019)
> 2. "static int __init hl_init(void)" (commit on Mon Feb 18 09:46:43 2019)
>
> Please give me more details.
Never use a staging driver for any type of example, _EXECPT_ for a bad
one. There's a reason the code is in staging and not in the "real" part
of the kernel.
thanks,
greg k-h
On Mon, Mar 01, 2021 at 11:39:23AM +0100, Maciej Kwapulinski wrote:
>
> Maciej Kwapulinski <[email protected]> writes:
>
> > Greg Kroah-Hartman <[email protected]> writes:
> >
> >> On Mon, Mar 01, 2021 at 11:18:59AM +0100, Maciej Kwapulinski wrote:
> >>>
> >>> Andy Shevchenko <[email protected]> writes:
> >>>
> >>> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> >>> > <[email protected]> wrote:
> >>> >>
> >>> ....
> >>> >> +static int __init gna_drv_init(void)
> >>> >> +{
> >>> >> + int ret;
> >>> >> +
> >>> >> + mutex_init(&gna_drv_priv.lock);
> >>> >> +
> >>> >> + gna_class = class_create(THIS_MODULE, "gna");
> >>> >> + if (IS_ERR(gna_class)) {
> >>> >> + pr_err("class device create failed\n");
> >>> >> + return PTR_ERR(gna_class);
> >>> >> + }
> >>> >> + gna_class->devnode = gna_devnode;
> >>> >> +
> >>> >> + ret = pci_register_driver(&gna_driver);
> >>> >
> >>> > Is it possible to decouple a PCI glue driver from the class as many
> >>> > other existing examples are doing?
> >>> >
> >>>
> >>> I see many pci drivers (including staging) that do have it glued though.
> >>>
> >>> Examples are:
> >>> 1. "static int __init kp2000_pcie_init(void)" (commit on May 20 09:34:11
> >>> 2019)
> >>> 2. "static int __init hl_init(void)" (commit on Mon Feb 18 09:46:43 2019)
> >>>
> >>> Please give me more details.
> >>
> >> Never use a staging driver for any type of example, _EXECPT_ for a bad
> >> one. There's a reason the code is in staging and not in the "real" part
> >> of the kernel.
> >
> > ok.
> >
> > another one (1) is not staging..
>
> I meant "static int __init hl_init(void)" is not staging one....
Still doesn't mean it is a good thing to do. Again, why isn't this
driver just using the misc driver interface instead? It's much simpler
to use and should work just fine for this tiny driver, instead of having
to create a custom class just for it.
thanks,
greg k-h
Greg Kroah-Hartman <[email protected]> writes:
> On Fri, Feb 26, 2021 at 01:59:14PM +0100, Maciej Kwapulinski wrote:
>>
>> Greg Kroah-Hartman <[email protected]> writes:
>>
>> > On Tue, Feb 16, 2021 at 05:05:14PM +0100, Maciej Kwapulinski wrote:
>> ....
>> >> --- /dev/null
>> >> +++ b/drivers/misc/gna/gna_driver.h
>> >> @@ -0,0 +1,41 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0-only */
>> >> +/* Copyright(c) 2017-2021 Intel Corporation */
>> >> +
>> >> +#ifndef __GNA_DRIVER_H__
>> >> +#define __GNA_DRIVER_H__
>> >> +
>> >> +#include <linux/kernel.h>
>> >> +#include <linux/mutex.h>
>> >> +#include <linux/types.h>
>> >> +
>> >> +#define GNA_DRV_NAME "gna"
>> >
>> > Way too generic, no one knows what "gna" is.
>> >
>>
>> "intel gna" is much more verbose in search engines.
>> As we do not (plan to) have more "gna" drivers, is the following ok?:
>>
>> intel-gna
>>
>> the change would imply the following:
>>
>> prompt$ lspci -s 00:00.3 -vvvv
>> 00:00.3 System peripheral: Intel Corporation Device 3190 (rev 03)
>> Subsystem: Intel Corporation Device 2072
>> ....
>> Kernel driver in use: intel-gna
>> Kernel modules: gna
>>
>> is it ok?
>
> Why not intel-gna as the kernel module as well?
>
>> also, how about the interface to library (it's part of one of next patches)?:
>> prompt$ file /dev/gna0
>> /dev/gna0: character special (235/0)
>>
>> can "gna" stay intact here?
>
> Again, I have no idea what "gna" is, so you might want to pick something
> more descriptive?
>
>> I'm pointing this out, because gna exists on the market for a while and
>> changing the above may have some impact we'd like to avoid.
>
> If it exists but Linux does not support it, how would anyone know about
> it? :)
>
> Please use real terms where possible.
>
> thanks,
>
> greg k-h
summarizing gna name justification topic, is the intel_gna.ko driver's
following layout within kernel code OK for You?:
1. driver/module name:
prompt$ lspci -s 00:00.3 -vvvv
00:00.3 System peripheral: Intel Corporation Device 3190 (rev 03)
....
Kernel driver in use: intel_gna
Kernel modules: intel_gna
2. mv drivers/misc/gna/* drivers/misc/intel_gna/
3. prompt$ file /dev/intel_gna0
/dev/intel_gna0: character special (10/120)
# ..., /dev/intel_gna1, /dev/intel_gna2 for subsequent devices