[PATCH 0/2] New driver for Intel(Altera) FPGA System ID softIP

[PATCH 0/2] New driver for Intel(Altera) FPGA System ID softIP

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

Hi,
Patches have been internally reviewed by colleagues at Intel.

New sysid driver for Altera(Intel) Sysid component is generally part of an
FPGA design. The component can be hotplugged when the FPGA is reconfigured.
This patch fixes the driver to support the component being hotplugged.

Thanks,
KJ

Kah Jing Lee (2):
  drivers: misc: intel_sysid: Add sysid from arch to drivers
  dt-bindings: misc: Add the system id binding for Altera(Intel) FPGA
    platform

 .../misc/intel,socfpga-sysid-1.0.yaml         |  39 +++++
 drivers/misc/Kconfig                          |   9 ++
 drivers/misc/Makefile                         |   1 +
 drivers/misc/intel_sysid.c                    | 142 ++++++++++++++++++
 4 files changed, 191 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/
intel,socfpga-sysid-1.0.yaml
 create mode 100644 drivers/misc/intel_sysid.c

-- 
2.25.1

Re: [PATCH 0/2] New driver for Intel(Altera) FPGA System ID softIP

[Greg KH]

On Thu, Jun 02, 2022 at 08:20:09PM +0800, kah.jing.lee@intel.com wrote:
> From: Kah Jing Lee <kah.jing.lee@intel.com>
> 
> Hi,
> Patches have been internally reviewed by colleagues at Intel.

But you did not send them as a series, or as a v2, or send out patch
1/2?

Please take a look at the kernel documentation for how to do this all
properly.

thanks,

greg k-h

Re: [PATCH 0/2] New driver for Intel(Altera) FPGA System ID softIP

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

Opps, I think I missed out the in-reply-to for the patch series.
Will do that for v2 once I get back from Intel OTC folks, and resent =).

Thanks,
KJ

[PATCH v2 0/3] New driver for Intel(Altera) FPGA System ID softIP

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

Hi,

I would like to request review for the new System ID driver for Intel FPGA
platform.

New sysid driver for Altera(Intel) Sysid component is generally part of an
FPGA design. The component can be hotplugged when the FPGA is reconfigured.

There are two basic ways to use the system ID core:
- Verify the system ID before downloading new software to a system. This
method can be used by software development tools, before downloading a
program to a target hardware system, if the program is compiled for
different hardware.

- Check system ID after reset. If a program is running on hardware other
than the expected Platform Designer system, the program may fail to
function altogether. If the program does not crash, it can behave
erroneously in subtle ways that are difficult to debug. To protect against
this case, a program can compare the expected system ID against the
system ID core, and report an error if they do not match.

Documentation link:
https://rocketboards.org/foswiki/Documentation/AgilexSoCGSRD#PR_Files

Feedback from maintainers:
https://lore.kernel.org/lkml/YpmqeHt5Y31ffh5Q@kroah.com/#t

Kah Jing Lee (3):
  drivers: misc: intel_sysid: Add sysid from arch to drivers
  dt-bindings: misc: intel_sysid: Add the system id binding for
    Altera(Intel) FPGA platform
  documentation: misc: intel_sysid: Add the system id sysfs
    documentation for Altera(Intel) FPGA platform

 .../testing/sysfs-devices-platform-soc-sysid  |  27 +++++
 .../bindings/misc/intel,socfpga-sysid.yaml    |  41 +++++++
 drivers/misc/Kconfig                          |   9 ++
 drivers/misc/Makefile                         |   1 +
 drivers/misc/intel_sysid.c                    | 114 ++++++++++++++++++
 5 files changed, 192 insertions(+)
 create mode 100644 Documentation/ABI/testing/
sysfs-devices-platform-soc-sysid
 create mode 100644 Documentation/devicetree/bindings/misc/
intel,socfpga-sysid.yaml
 create mode 100644 drivers/misc/intel_sysid.c

-- 
2.25.1

[PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

Add new sysid driver. The Altera(Intel) Sysid component is generally part
of an FPGA design. The component can be hotplugged when the FPGA is
reconfigured. This driver support the component being hotplugged.
The sysid driver stores unique 32-bit id value which is similar to a
check-sum value; different components, different configuration options,
or both, can be configured to produce different id values. Timestamp field
is the unique 32-bit value that is based on the system generation time.

There are two basic ways to use the system ID core:
- Verify the system ID before downloading new software to a system. This
method can be used by software development tools, before downloading a
program to a target hardware system, if the program is compiled for
different hardware.

- Check system ID after reset. If a program is running on hardware other
than the expected Platform Designer system, the program may fail to
function altogether. If the program does not crash, it can behave
erroneously in subtle ways that are difficult to debug. To protect against
this case, a program can compare the expected system ID against the system
ID core, and report an error if they do not match.

Usage:
  cat /sys/bus/platform/devices/soc:base_fpga_region/
		soc:base_fpga_region:fpga_pr_region0/[addr.sysid]/sysid/id
  cat /sys/bus/platform/devices/soc:base_fpga_region/
		soc:base_fpga_region:fpga_pr_region0/[addr.sysid]/sysid/buildtime

Based on an initial contribution from Ley Foon Tan at Altera
Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
Reviewed-by: Zhou, Furong <furong.zhou@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
v2:
- Updated license header, commit message and author since original author
no longer here
- Updated driver description
- Removed redundant word in Kconfig
- Updated timestamp function, renamed timestamp -> buildtime due to programmed
  time during design generation instead of real-time timestamp reading
- Updated Kconfig description
- Updated sysfs add to devm_add_group
- Add the Documentation/ABI/testing file
- Updated header and newline formatting
---
---
 drivers/misc/Kconfig       |   9 +++
 drivers/misc/Makefile      |   1 +
 drivers/misc/intel_sysid.c | 114 +++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 drivers/misc/intel_sysid.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 41d2bb0ae23a..30cf36916593 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -483,6 +483,15 @@ config OPEN_DICE
 
 	  If unsure, say N.
 
+config INTEL_SYSID
+	tristate "Intel System ID"
+	help
+	  This enables the Intel System ID driver for a soft core.
+	  Say Y here if you want to build a driver for Intel System ID.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called intel_sysid. If unsure, say N here.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 70e800e9127f..301c854b4cd3 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_PCH_PHUB)		+= pch_phub.o
 obj-y				+= ti-st/
 obj-y				+= lis3lv02d/
 obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
+obj-$(CONFIG_INTEL_SYSID)	+= intel_sysid.o
 obj-$(CONFIG_INTEL_MEI)		+= mei/
 obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
 obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
diff --git a/drivers/misc/intel_sysid.c b/drivers/misc/intel_sysid.c
new file mode 100644
index 000000000000..b63dbde63347
--- /dev/null
+++ b/drivers/misc/intel_sysid.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022, Intel Corporation.
+ * Copyright (C) 2013-2015, Altera Corporation.
+ *
+ * Ley Foon Tan <lftan@altera.com>
+ * Kah Jing Lee <kah.jing.lee@intel.com>
+ */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define DRV_NAME	"intel_sysid"
+
+struct intel_sysid {
+	void __iomem		*regs;
+};
+
+/* System ID Registers*/
+#define SYSID_REG_ID		0x0
+#define SYSID_REG_TIMESTAMP	0x4
+
+static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct intel_sysid *sysid = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", readl(sysid->regs + SYSID_REG_ID));
+}
+
+static void convert_readable_timestamp(struct tm *buildtime)
+{
+	buildtime->tm_year += 1900;
+	buildtime->tm_mon += 1;
+}
+
+static ssize_t buildtime_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	unsigned int reg;
+	struct tm buildtime;
+	struct intel_sysid *sysid = dev_get_drvdata(dev);
+
+	reg = readl(sysid->regs + SYSID_REG_TIMESTAMP);
+	time64_to_tm(reg, 0, &buildtime);
+	convert_readable_timestamp(&buildtime);
+
+	return sprintf(buf, "%u (%u-%u-%u %u:%u:%u UTC)\n", reg,
+		(unsigned int)(buildtime.tm_year),
+		buildtime.tm_mon, buildtime.tm_mday, buildtime.tm_hour,
+		buildtime.tm_min, buildtime.tm_sec);
+}
+
+static DEVICE_ATTR_RO(id);
+static DEVICE_ATTR_RO(buildtime);
+
+static struct attribute *intel_sysid_attrs[] = {
+	&dev_attr_id.attr,
+	&dev_attr_buildtime.attr,
+	NULL
+};
+
+struct attribute_group intel_sysid_attr_group = {
+	.name = "sysid",
+	.attrs = intel_sysid_attrs,
+};
+
+static int intel_sysid_probe(struct platform_device *pdev)
+{
+	struct intel_sysid *sysid;
+	struct resource	*regs;
+
+	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
+		GFP_KERNEL);
+	if (!sysid)
+		return -ENOMEM;
+
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!regs)
+		return -ENXIO;
+
+	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
+	if (IS_ERR(sysid->regs))
+		return PTR_ERR(sysid->regs);
+
+	platform_set_drvdata(pdev, sysid);
+
+	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
+}
+
+static const struct of_device_id intel_sysid_match[] = {
+	{ .compatible = "intel,socfpga-sysid-1.0" },
+	{ /* Sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, intel_sysid_match);
+
+static struct platform_driver intel_sysid_platform_driver = {
+	.driver = {
+		.name		= DRV_NAME,
+		.of_match_table	= of_match_ptr(intel_sysid_match),
+	},
+	.probe			= intel_sysid_probe,
+};
+
+module_platform_driver(intel_sysid_platform_driver);
+
+MODULE_AUTHOR("Kah Jing Lee <kah.jing.lee@intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Intel System ID driver");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.25.1

[PATCH v2 2/3] dt-bindings: misc: intel_sysid: Add the system id binding for Altera(Intel) FPGA platform

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

This binding is created for Altera(Intel) FPGA platform System ID soft IP.
The Altera(Intel) Sysid component is generally part of an FPGA design.
The component can be hotplugged when the FPGA is reconfigured.

Based on an initial contribution from Ley Foon Tan at Altera
Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
Reviewed-by: Zhou Furong <furong.zhou@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../bindings/misc/intel,socfpga-sysid.yaml    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml

diff --git a/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml b/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml
new file mode 100644
index 000000000000..055f4cb305ac
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022, Intel Corporation.
+# Copyright (C) 2013-2015, Altera Corporation.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/intel,socfpga-sysid.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Altera(Intel) Sysid IP core driver
+
+maintainers:
+  - Arnd Bergmann <arnd@arndb.de>
+  - Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+
+description: |
+  The Altera(Intel) Sysid component is generally part of an FPGA design. The
+  component can be hotplugged when the FPGA is reconfigured.  This patch
+  fixes the driver to support the component being hotplugged.
+
+properties:
+  compatible:
+    items:
+      - const: intel,socfpga-sysid-1.0
+
+  reg:
+    items:
+      - description: physical address and length of the registers which
+          contain revision and debug features
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    sysid_qsys: sysid@10000 {
+        compatible = "intel,socfpga-sysid-1.0";
+        reg = < 0x10000 0x00000008 >;
+    };
-- 
2.25.1

[PATCH v2 3/3] documentation: misc: intel_sysid: Add the system id sysfs documentation for Altera(Intel) FPGA platform

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

This sysfs documentation is created for Altera(Intel) FPGA platform
System ID soft IP. The Altera(Intel) Sysid component is generally
part of an FPGA design.
The component can be hotplugged when the FPGA is reconfigured.

Based on an initial contribution from Ley Foon Tan at Altera
Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
Reviewed-by: Zhou Furong <furong.zhou@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../testing/sysfs-devices-platform-soc-sysid  | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-soc-sysid

diff --git a/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
new file mode 100644
index 000000000000..9fa58fd88dc0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
@@ -0,0 +1,27 @@
+What:		/sys/devices/platform/soc@X/soc:base_fpga_region/
+soc:base_fpga_region:fpga_pr_region0/XXXXXXXX.sysid/
+Date:		May 2022
+KernelVersion:	v5.18
+Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
+Description:
+		The soc@X/soc:base_fpga_region/soc:base_fpga_region:fpga_pr_region0/
+		XXXXXXXX.sysid/ directory contains read-only attributes exposing
+		information about an System ID soft IP device. The X values could vary,
+		based on the FPGA platform System ID soft IP register address.
+
+What:		.../XXXXXXX.sysid/sysid
+Date:		May 2022
+KernelVersion:	v5.18
+Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
+Description:
+		The .../XXXXXXX.sysid/sysid file contains the System ID for the FPGA
+		platform which is unique for the platform type and can be used for
+		checking the platform type for software download purposes.
+
+What:		.../XXXXXXX.sysid/buildtime
+Date:		May 2022
+KernelVersion:	v5.18
+Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
+Description:
+		The .../XXXXXXX.sysid/buildtime file contains the buildtime for the
+		FPGA board file generation.
-- 
2.25.1

Re: [PATCH v2 3/3] documentation: misc: intel_sysid: Add the system id sysfs documentation for Altera(Intel) FPGA platform

[Greg KH]

On Thu, Jul 21, 2022 at 08:32:59PM +0800, kah.jing.lee@intel.com wrote:
> From: Kah Jing Lee <kah.jing.lee@intel.com>
> 
> This sysfs documentation is created for Altera(Intel) FPGA platform
> System ID soft IP. The Altera(Intel) Sysid component is generally
> part of an FPGA design.
> The component can be hotplugged when the FPGA is reconfigured.
> 
> Based on an initial contribution from Ley Foon Tan at Altera
> Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
> Reviewed-by: Zhou Furong <furong.zhou@intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  .../testing/sysfs-devices-platform-soc-sysid  | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
> new file mode 100644
> index 000000000000..9fa58fd88dc0
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
> @@ -0,0 +1,27 @@
> +What:		/sys/devices/platform/soc@X/soc:base_fpga_region/
> +soc:base_fpga_region:fpga_pr_region0/XXXXXXXX.sysid/
> +Date:		May 2022
> +KernelVersion:	v5.18

5.18 is long released.  And it's after May 2022 now :(


> +Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
> +Description:
> +		The soc@X/soc:base_fpga_region/soc:base_fpga_region:fpga_pr_region0/
> +		XXXXXXXX.sysid/ directory contains read-only attributes exposing
> +		information about an System ID soft IP device. The X values could vary,
> +		based on the FPGA platform System ID soft IP register address.
> +
> +What:		.../XXXXXXX.sysid/sysid
> +Date:		May 2022
> +KernelVersion:	v5.18
> +Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
> +Description:
> +		The .../XXXXXXX.sysid/sysid file contains the System ID for the FPGA
> +		platform which is unique for the platform type and can be used for
> +		checking the platform type for software download purposes.

What format is this data in?

> +
> +What:		.../XXXXXXX.sysid/buildtime
> +Date:		May 2022
> +KernelVersion:	v5.18
> +Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
> +Description:
> +		The .../XXXXXXX.sysid/buildtime file contains the buildtime for the
> +		FPGA board file generation.

What format is this data in?

Please be specific.

thanks,

greg k-h

Re: [PATCH v2 2/3] dt-bindings: misc: intel_sysid: Add the system id binding for Altera(Intel) FPGA platform

[Greg KH]

On Thu, Jul 21, 2022 at 08:32:17PM +0800, kah.jing.lee@intel.com wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022, Intel Corporation.
> +# Copyright (C) 2013-2015, Altera Corporation.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/intel,socfpga-sysid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Altera(Intel) Sysid IP core driver
> +
> +maintainers:
> +  - Arnd Bergmann <arnd@arndb.de>
> +  - Greg Kroah-Hartman <gregkh@linuxfoundation.org>

You want me to maintain an Intel-only file?   Great, where do I send my
billing rates to?  :)

thanks,

greg k-h

Re: [PATCH v2 2/3] dt-bindings: misc: intel_sysid: Add the system id binding for Altera(Intel) FPGA platform

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

> You want me to maintain an Intel-only file?   Great, where do I send my
> billing rates to?  :)
Updated, and will resend.
Too bad that's beyond my paygrade to approve ;)

Regards,
Lee, Kah Jing

[PATCH v3 2/3] dt-bindings: misc: intel_sysid: Add the system id binding for Altera(Intel) FPGA platform

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

This binding is created for Altera(Intel) FPGA platform System ID soft IP.
The Altera(Intel) Sysid component is generally part of an FPGA design.
The component can be hotplugged when the FPGA is reconfigured.

Based on an initial contribution from Ley Foon Tan at Altera
Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
Reviewed-by: Zhou Furong <furong.zhou@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
v2->v3:
- Updated maintainer
---
---
 .../bindings/misc/intel,socfpga-sysid.yaml    | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml

diff --git a/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml b/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml
new file mode 100644
index 000000000000..7426cbe4462b
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/intel,socfpga-sysid.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022, Intel Corporation.
+# Copyright (C) 2013-2015, Altera Corporation.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/intel,socfpga-sysid.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Altera(Intel) Sysid IP core driver
+
+maintainers:
+  - Kah Jing Lee <kah.jing.lee@intel.com>
+
+description: |
+  The Altera(Intel) Sysid component is generally part of an FPGA design. The
+  component can be hotplugged when the FPGA is reconfigured.  This patch
+  fixes the driver to support the component being hotplugged.
+
+properties:
+  compatible:
+    items:
+      - const: intel,socfpga-sysid-1.0
+
+  reg:
+    items:
+      - description: physical address and length of the registers which
+          contain revision and debug features
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    sysid_qsys: sysid@10000 {
+        compatible = "intel,socfpga-sysid-1.0";
+        reg = < 0x10000 0x00000008 >;
+    };
-- 
2.25.1

[PATCH v3 3/3] documentation: misc: intel_sysid: Add the system id sysfs documentation for Altera(Intel) FPGA platform

[kah.jing.lee]

From: Kah Jing Lee <kah.jing.lee@intel.com>

This sysfs documentation is created for Altera(Intel) FPGA platform
System ID soft IP. The Altera(Intel) Sysid component is generally
part of an FPGA design.
The component can be hotplugged when the FPGA is reconfigured.

Based on an initial contribution from Ley Foon Tan at Altera
Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
Reviewed-by: Zhou Furong <furong.zhou@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

---
v2->v3:
- Updated kernel version & date
- Added format for sysid & builtime output
---
---
 .../testing/sysfs-devices-platform-soc-sysid  | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-soc-sysid

diff --git a/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
new file mode 100644
index 000000000000..6e40d154601f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
@@ -0,0 +1,41 @@
+What:		/sys/devices/platform/soc@X/soc:base_fpga_region/
+soc:base_fpga_region:fpga_pr_region0/XXXXXXXX.sysid/
+Date:		July 2022
+KernelVersion:	v5.20
+Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
+Description:
+		The soc@X/soc:base_fpga_region/soc:base_fpga_region:fpga_pr_region0/
+		XXXXXXXX.sysid/ directory contains read-only attributes exposing
+		information about an System ID soft IP device. The X values could vary,
+		based on the FPGA platform System ID soft IP register address.
+
+What:		.../XXXXXXX.sysid/sysid
+Date:		July 2022
+KernelVersion:	v5.20
+Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
+Description:
+		The .../XXXXXXX.sysid/sysid file contains the System ID for the FPGA
+		platform which is unique for the platform type and can be used for
+		checking the platform type for software download purposes. Sysid value
+		reported is in numerical format, and can also be printed in hex format
+		for human-readable string.
+		E.g:
+			root@agilex:~# cat /sys/bus/platform/drivers/altera_sysid/
+				f9000900.sysid/sysid/id
+			4207856382
+			root@agilex:~# cat /sys/bus/platform/drivers/altera_sysid/
+				f9000900.sysid/sysid/id | xargs printf "0x%08x\n"
+			0xfacecafe
+
+What:		.../XXXXXXX.sysid/buildtime
+Date:		July 2022
+KernelVersion:	v5.20
+Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
+Description:
+		The .../XXXXXXX.sysid/buildtime file contains the buildtime for the
+		FPGA board file generation. Buildtime value reported in
+		<Unix timestamp> (YYYY-mm-dd HH:MM:SS UTC) format.
+		E.g:
+			root@agilex:~# cat /sys/bus/platform/drivers/altera_sysid/
+				f9000900.sysid/sysid/timestamp
+			1637751409 (2021-11-24 10:56:49 UTC)
-- 
2.25.1

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[kernel test robot]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on soc/for-next linus/master v5.19-rc8]
[cannot apply to char-misc/char-misc-testing next-20220727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/kah-jing-lee-intel-com/drivers-misc-intel_sysid-Add-sysid-from-arch-to-drivers/20220721-213214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 8af028c2b22bc04f5ab59cd39fa97ccf14aa8f25
config: s390-randconfig-p001-20220727 (https://download.01.org/0day-ci/archive/20220728/202207280422.mCMrDg9E-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5e0d691312542fbb751afb99bd7b537b9a975750
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review kah-jing-lee-intel-com/drivers-misc-intel_sysid-Add-sysid-from-arch-to-drivers/20220721-213214
        git checkout 5e0d691312542fbb751afb99bd7b537b9a975750
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
reproduce (cppcheck warning):
        # apt-get install cppcheck
        git checkout 5e0d691312542fbb751afb99bd7b537b9a975750
        cppcheck --quiet --enable=style,performance,portability --template=gcc FILE

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   s390-linux-ld: drivers/misc/intel_sysid.o: in function `intel_sysid_probe':
>> drivers/misc/intel_sysid.c:85: undefined reference to `devm_ioremap_resource'
   s390-linux-ld: drivers/net/ethernet/altera/altera_tse_main.o: in function `request_and_map':
>> drivers/net/ethernet/altera/altera_tse_main.c:1339: undefined reference to `devm_ioremap'
   pahole: .tmp_vmlinux.btf: No such file or directory
   .btf.vmlinux.bin.o: file not recognized: file format not recognized


cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

>> arch/s390/kernel/perf_cpum_sf.c:805:8: warning: Using pointer that is a temporary. [danglingTemporaryLifetime]
     si = cpuhw->qsi;
          ^
   arch/s390/kernel/perf_cpum_sf.c:804:11: note: Address of variable taken here.
     cpuhw = &per_cpu(cpu_hw_sf, event->cpu);
             ^
   arch/s390/kernel/perf_cpum_sf.c:804:19: note: Temporary created here.
     cpuhw = &per_cpu(cpu_hw_sf, event->cpu);
                     ^
   arch/s390/kernel/perf_cpum_sf.c:805:8: note: Using pointer that is a temporary.
     si = cpuhw->qsi;
          ^
   arch/s390/kernel/perf_cpum_sf.c:867:27: warning: Using pointer that is a temporary. [danglingTemporaryLifetime]
      err = allocate_buffers(cpuhw, hwc);
                             ^
   arch/s390/kernel/perf_cpum_sf.c:866:12: note: Address of variable taken here.
      cpuhw = &per_cpu(cpu_hw_sf, cpu);
              ^
   arch/s390/kernel/perf_cpum_sf.c:866:20: note: Temporary created here.
      cpuhw = &per_cpu(cpu_hw_sf, cpu);
                      ^
   arch/s390/kernel/perf_cpum_sf.c:867:27: note: Using pointer that is a temporary.
      err = allocate_buffers(cpuhw, hwc);
                             ^
   arch/s390/kernel/perf_cpum_sf.c:1825:8: warning: Using pointer that is a temporary. [danglingTemporaryLifetime]
     si = cpuhw->qsi;
          ^
   arch/s390/kernel/perf_cpum_sf.c:1823:29: note: Address of variable taken here.
     struct cpu_hw_sf *cpuhw = &per_cpu(cpu_hw_sf, event->cpu);
                               ^
   arch/s390/kernel/perf_cpum_sf.c:1823:37: note: Temporary created here.
     struct cpu_hw_sf *cpuhw = &per_cpu(cpu_hw_sf, event->cpu);
                                       ^
   arch/s390/kernel/perf_cpum_sf.c:1825:8: note: Using pointer that is a temporary.
     si = cpuhw->qsi;
          ^

vim +85 drivers/misc/intel_sysid.c

    70	
    71	static int intel_sysid_probe(struct platform_device *pdev)
    72	{
    73		struct intel_sysid *sysid;
    74		struct resource	*regs;
    75	
    76		sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
    77			GFP_KERNEL);
    78		if (!sysid)
    79			return -ENOMEM;
    80	
    81		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    82		if (!regs)
    83			return -ENXIO;
    84	
  > 85		sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
    86		if (IS_ERR(sysid->regs))
    87			return PTR_ERR(sysid->regs);
    88	
    89		platform_set_drvdata(pdev, sysid);
    90	
    91		return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
    92	}
    93	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 3/3] documentation: misc: intel_sysid: Add the system id sysfs documentation for Altera(Intel) FPGA platform

[Greg KH]

On Mon, Jul 25, 2022 at 11:59:41AM +0800, kah.jing.lee@intel.com wrote:
> From: Kah Jing Lee <kah.jing.lee@intel.com>
> 
> This sysfs documentation is created for Altera(Intel) FPGA platform
> System ID soft IP. The Altera(Intel) Sysid component is generally
> part of an FPGA design.
> The component can be hotplugged when the FPGA is reconfigured.
> 
> Based on an initial contribution from Ley Foon Tan at Altera
> Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>

You always need a blank line before the signed-off-by, didn't checkpatch
complain about this?

> Reviewed-by: Zhou Furong <furong.zhou@intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> ---
> v2->v3:
> - Updated kernel version & date
> - Added format for sysid & builtime output

Please resend patches as whole new series, not as odd responses to
responses like this as it is impossible to figure out this way.

Please take some time and review patches from others to see just how
this process works.

> ---
> ---
>  .../testing/sysfs-devices-platform-soc-sysid  | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
> new file mode 100644
> index 000000000000..6e40d154601f
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-platform-soc-sysid
> @@ -0,0 +1,41 @@
> +What:		/sys/devices/platform/soc@X/soc:base_fpga_region/
> +soc:base_fpga_region:fpga_pr_region0/XXXXXXXX.sysid/
> +Date:		July 2022

July is almost over.

> +KernelVersion:	v5.20
> +Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
> +Description:
> +		The soc@X/soc:base_fpga_region/soc:base_fpga_region:fpga_pr_region0/
> +		XXXXXXXX.sysid/ directory contains read-only attributes exposing
> +		information about an System ID soft IP device. The X values could vary,
> +		based on the FPGA platform System ID soft IP register address.
> +
> +What:		.../XXXXXXX.sysid/sysid
> +Date:		July 2022
> +KernelVersion:	v5.20
> +Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
> +Description:
> +		The .../XXXXXXX.sysid/sysid file contains the System ID for the FPGA
> +		platform which is unique for the platform type and can be used for
> +		checking the platform type for software download purposes. Sysid value
> +		reported is in numerical format, and can also be printed in hex format
> +		for human-readable string.
> +		E.g:
> +			root@agilex:~# cat /sys/bus/platform/drivers/altera_sysid/
> +				f9000900.sysid/sysid/id

Why the line wrapping?

> +			4207856382
> +			root@agilex:~# cat /sys/bus/platform/drivers/altera_sysid/
> +				f9000900.sysid/sysid/id | xargs printf "0x%08x\n"
> +			0xfacecafe

If userspace wants this in hex format, just have the sysfs file export a
hex format.  No need to provide a tutorial for how to convert bases in
userspace, right?

> +
> +What:		.../XXXXXXX.sysid/buildtime
> +Date:		July 2022
> +KernelVersion:	v5.20
> +Contact:	Kah Jing Lee <kah.jing.lee@intel.com>
> +Description:
> +		The .../XXXXXXX.sysid/buildtime file contains the buildtime for the
> +		FPGA board file generation. Buildtime value reported in
> +		<Unix timestamp> (YYYY-mm-dd HH:MM:SS UTC) format.
> +		E.g:
> +			root@agilex:~# cat /sys/bus/platform/drivers/altera_sysid/
> +				f9000900.sysid/sysid/timestamp
> +			1637751409 (2021-11-24 10:56:49 UTC)

Why not use the proper RFC format for time here instead of making up a
new one?  And who will use this file, why does it have to be parsed like
this?

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Greg KH]

On Thu, Jul 21, 2022 at 08:31:50PM +0800, kah.jing.lee@intel.com wrote:
> From: Kah Jing Lee <kah.jing.lee@intel.com>
> 
> Add new sysid driver. The Altera(Intel) Sysid component is generally part
> of an FPGA design. The component can be hotplugged when the FPGA is
> reconfigured. This driver support the component being hotplugged.
> The sysid driver stores unique 32-bit id value which is similar to a
> check-sum value; different components, different configuration options,
> or both, can be configured to produce different id values. Timestamp field
> is the unique 32-bit value that is based on the system generation time.
> 
> There are two basic ways to use the system ID core:
> - Verify the system ID before downloading new software to a system. This
> method can be used by software development tools, before downloading a
> program to a target hardware system, if the program is compiled for
> different hardware.
> 
> - Check system ID after reset. If a program is running on hardware other
> than the expected Platform Designer system, the program may fail to
> function altogether. If the program does not crash, it can behave
> erroneously in subtle ways that are difficult to debug. To protect against
> this case, a program can compare the expected system ID against the system
> ID core, and report an error if they do not match.
> 
> Usage:
>   cat /sys/bus/platform/devices/soc:base_fpga_region/
> 		soc:base_fpga_region:fpga_pr_region0/[addr.sysid]/sysid/id
>   cat /sys/bus/platform/devices/soc:base_fpga_region/
> 		soc:base_fpga_region:fpga_pr_region0/[addr.sysid]/sysid/buildtime
> 
> Based on an initial contribution from Ley Foon Tan at Altera
> Signed-off-by: Kah Jing Lee <kah.jing.lee@intel.com>
> Reviewed-by: Zhou, Furong <furong.zhou@intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> v2:
> - Updated license header, commit message and author since original author
> no longer here
> - Updated driver description
> - Removed redundant word in Kconfig
> - Updated timestamp function, renamed timestamp -> buildtime due to programmed
>   time during design generation instead of real-time timestamp reading
> - Updated Kconfig description
> - Updated sysfs add to devm_add_group
> - Add the Documentation/ABI/testing file
> - Updated header and newline formatting
> ---
> ---
>  drivers/misc/Kconfig       |   9 +++
>  drivers/misc/Makefile      |   1 +
>  drivers/misc/intel_sysid.c | 114 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
>  create mode 100644 drivers/misc/intel_sysid.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 41d2bb0ae23a..30cf36916593 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -483,6 +483,15 @@ config OPEN_DICE
>  
>  	  If unsure, say N.
>  
> +config INTEL_SYSID
> +	tristate "Intel System ID"
> +	help
> +	  This enables the Intel System ID driver for a soft core.
> +	  Say Y here if you want to build a driver for Intel System ID.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called intel_sysid. If unsure, say N here.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 70e800e9127f..301c854b4cd3 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_PCH_PHUB)		+= pch_phub.o
>  obj-y				+= ti-st/
>  obj-y				+= lis3lv02d/
>  obj-$(CONFIG_ALTERA_STAPL)	+=altera-stapl/
> +obj-$(CONFIG_INTEL_SYSID)	+= intel_sysid.o
>  obj-$(CONFIG_INTEL_MEI)		+= mei/
>  obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
>  obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
> diff --git a/drivers/misc/intel_sysid.c b/drivers/misc/intel_sysid.c
> new file mode 100644
> index 000000000000..b63dbde63347
> --- /dev/null
> +++ b/drivers/misc/intel_sysid.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022, Intel Corporation.
> + * Copyright (C) 2013-2015, Altera Corporation.
> + *
> + * Ley Foon Tan <lftan@altera.com>
> + * Kah Jing Lee <kah.jing.lee@intel.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define DRV_NAME	"intel_sysid"

KBUILD_MODNAME?

> +
> +struct intel_sysid {
> +	void __iomem		*regs;
> +};
> +
> +/* System ID Registers*/
> +#define SYSID_REG_ID		0x0
> +#define SYSID_REG_TIMESTAMP	0x4
> +
> +static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct intel_sysid *sysid = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", readl(sysid->regs + SYSID_REG_ID));

sysfs_emit() please.

> +}
> +
> +static void convert_readable_timestamp(struct tm *buildtime)
> +{
> +	buildtime->tm_year += 1900;
> +	buildtime->tm_mon += 1;
> +}
> +
> +static ssize_t buildtime_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int reg;
> +	struct tm buildtime;
> +	struct intel_sysid *sysid = dev_get_drvdata(dev);
> +
> +	reg = readl(sysid->regs + SYSID_REG_TIMESTAMP);
> +	time64_to_tm(reg, 0, &buildtime);
> +	convert_readable_timestamp(&buildtime);
> +
> +	return sprintf(buf, "%u (%u-%u-%u %u:%u:%u UTC)\n", reg,
> +		(unsigned int)(buildtime.tm_year),
> +		buildtime.tm_mon, buildtime.tm_mday, buildtime.tm_hour,
> +		buildtime.tm_min, buildtime.tm_sec);

As said in the documentation review, use a standard format, do not make
up a new one.

> +}
> +
> +static DEVICE_ATTR_RO(id);
> +static DEVICE_ATTR_RO(buildtime);
> +
> +static struct attribute *intel_sysid_attrs[] = {
> +	&dev_attr_id.attr,
> +	&dev_attr_buildtime.attr,
> +	NULL
> +};
> +
> +struct attribute_group intel_sysid_attr_group = {
> +	.name = "sysid",
> +	.attrs = intel_sysid_attrs,
> +};
> +
> +static int intel_sysid_probe(struct platform_device *pdev)
> +{
> +	struct intel_sysid *sysid;
> +	struct resource	*regs;
> +
> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
> +		GFP_KERNEL);
> +	if (!sysid)
> +		return -ENOMEM;
> +
> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!regs)
> +		return -ENXIO;
> +
> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
> +	if (IS_ERR(sysid->regs))
> +		return PTR_ERR(sysid->regs);
> +
> +	platform_set_drvdata(pdev, sysid);
> +
> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);

You just raced with userspace and lost.  Please use the default group
for the platform device.

I need to go remove this function, it should not be used at all as it is
broken.

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Greg KH]

On Thu, Jul 21, 2022 at 08:31:50PM +0800, kah.jing.lee@intel.com wrote:
> From: Kah Jing Lee <kah.jing.lee@intel.com>
> 
> Add new sysid driver. The Altera(Intel) Sysid component is generally part
> of an FPGA design. The component can be hotplugged when the FPGA is
> reconfigured. This driver support the component being hotplugged.
> The sysid driver stores unique 32-bit id value which is similar to a
> check-sum value; different components, different configuration options,
> or both, can be configured to produce different id values. Timestamp field
> is the unique 32-bit value that is based on the system generation time.

I really do not understand what this driver does at all, sorry.  It only
exports 2 sysfs files.  Who will use those sysfs files?  What are they
for?  Why is a driver needed at all as all you are doing is reading 2
memory values from the device, right?  Why is the kernel responsible for
doing the data conversion logic and not userspace?

> There are two basic ways to use the system ID core:
> - Verify the system ID before downloading new software to a system. This
> method can be used by software development tools, before downloading a
> program to a target hardware system, if the program is compiled for
> different hardware.

verify it how?  This is just a random value that we have no idea how to
treat it.

> - Check system ID after reset. If a program is running on hardware other
> than the expected Platform Designer system, the program may fail to
> function altogether. If the program does not crash, it can behave
> erroneously in subtle ways that are difficult to debug. To protect against
> this case, a program can compare the expected system ID against the system
> ID core, and report an error if they do not match.

Where are these ids listed to be able to verify anything?

What userspace tools use this new driver?

thanks,

greg k-h

Re: [PATCH v2 0/3] New driver for Intel(Altera) FPGA System ID softIP

[Greg KH]

On Thu, Jul 21, 2022 at 08:30:19PM +0800, kah.jing.lee@intel.com wrote:
> From: Kah Jing Lee <kah.jing.lee@intel.com>
> 
> Hi,
> 
> I would like to request review for the new System ID driver for Intel FPGA
> platform.
> 
> New sysid driver for Altera(Intel) Sysid component is generally part of an
> FPGA design. The component can be hotplugged when the FPGA is reconfigured.
> 
> There are two basic ways to use the system ID core:
> - Verify the system ID before downloading new software to a system. This
> method can be used by software development tools, before downloading a
> program to a target hardware system, if the program is compiled for
> different hardware.
> 
> - Check system ID after reset. If a program is running on hardware other
> than the expected Platform Designer system, the program may fail to
> function altogether. If the program does not crash, it can behave
> erroneously in subtle ways that are difficult to debug. To protect against
> this case, a program can compare the expected system ID against the
> system ID core, and report an error if they do not match.
> 
> Documentation link:
> https://rocketboards.org/foswiki/Documentation/AgilexSoCGSRD#PR_Files
> 
> Feedback from maintainers:
> https://lore.kernel.org/lkml/YpmqeHt5Y31ffh5Q@kroah.com/#t

That does not describe what changed from the previous version at all, it
just points to a response from me saying you all need to work on this
driver.

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Pierre-Louis Bossart]

Thanks for the review Greg,

>> +static int intel_sysid_probe(struct platform_device *pdev)
>> +{
>> +	struct intel_sysid *sysid;
>> +	struct resource	*regs;
>> +
>> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
>> +		GFP_KERNEL);
>> +	if (!sysid)
>> +		return -ENOMEM;
>> +
>> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!regs)
>> +		return -ENXIO;
>> +
>> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
>> +	if (IS_ERR(sysid->regs))
>> +		return PTR_ERR(sysid->regs);
>> +
>> +	platform_set_drvdata(pdev, sysid);
>> +
>> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
> 
> You just raced with userspace and lost.  Please use the default group
> for the platform device.
> 
> I need to go remove this function, it should not be used at all as it is
> broken.

Can you elaborate on the issue and suggested replacement?

We used this function for the SoundWire sysfs based on your review
comments (2 years ago?) that we should not muck with kobj, and that
function devm_device_add_group() is also used in a probe function.

Thanks!
-Pierre

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Greg KH]

On Thu, Jul 28, 2022 at 10:37:37AM -0500, Pierre-Louis Bossart wrote:
> Thanks for the review Greg,
> 
> >> +static int intel_sysid_probe(struct platform_device *pdev)
> >> +{
> >> +	struct intel_sysid *sysid;
> >> +	struct resource	*regs;
> >> +
> >> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
> >> +		GFP_KERNEL);
> >> +	if (!sysid)
> >> +		return -ENOMEM;
> >> +
> >> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	if (!regs)
> >> +		return -ENXIO;
> >> +
> >> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
> >> +	if (IS_ERR(sysid->regs))
> >> +		return PTR_ERR(sysid->regs);
> >> +
> >> +	platform_set_drvdata(pdev, sysid);
> >> +
> >> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
> > 
> > You just raced with userspace and lost.  Please use the default group
> > for the platform device.
> > 
> > I need to go remove this function, it should not be used at all as it is
> > broken.
> 
> Can you elaborate on the issue and suggested replacement?
> 
> We used this function for the SoundWire sysfs based on your review
> comments (2 years ago?) that we should not muck with kobj, and that
> function devm_device_add_group() is also used in a probe function.

Use the default_groups pointer in the driver structure.

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Pierre-Louis Bossart]

On 7/28/22 10:59, Greg KH wrote:
> On Thu, Jul 28, 2022 at 10:37:37AM -0500, Pierre-Louis Bossart wrote:
>> Thanks for the review Greg,
>>
>>>> +static int intel_sysid_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct intel_sysid *sysid;
>>>> +	struct resource	*regs;
>>>> +
>>>> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
>>>> +		GFP_KERNEL);
>>>> +	if (!sysid)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!regs)
>>>> +		return -ENXIO;
>>>> +
>>>> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
>>>> +	if (IS_ERR(sysid->regs))
>>>> +		return PTR_ERR(sysid->regs);
>>>> +
>>>> +	platform_set_drvdata(pdev, sysid);
>>>> +
>>>> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
>>>
>>> You just raced with userspace and lost.  Please use the default group
>>> for the platform device.
>>>
>>> I need to go remove this function, it should not be used at all as it is
>>> broken.
>>
>> Can you elaborate on the issue and suggested replacement?
>>
>> We used this function for the SoundWire sysfs based on your review
>> comments (2 years ago?) that we should not muck with kobj, and that
>> function devm_device_add_group() is also used in a probe function.
> 
> Use the default_groups pointer in the driver structure.

did you mean dev_groups?

I am not following the idea, for SoundWire all the attributes are really
device-specific or described by ACPI and cannot be hard-coded in the
driver structure.

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Greg KH]

On Thu, Jul 28, 2022 at 11:53:33AM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 7/28/22 10:59, Greg KH wrote:
> > On Thu, Jul 28, 2022 at 10:37:37AM -0500, Pierre-Louis Bossart wrote:
> >> Thanks for the review Greg,
> >>
> >>>> +static int intel_sysid_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct intel_sysid *sysid;
> >>>> +	struct resource	*regs;
> >>>> +
> >>>> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
> >>>> +		GFP_KERNEL);
> >>>> +	if (!sysid)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>> +	if (!regs)
> >>>> +		return -ENXIO;
> >>>> +
> >>>> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
> >>>> +	if (IS_ERR(sysid->regs))
> >>>> +		return PTR_ERR(sysid->regs);
> >>>> +
> >>>> +	platform_set_drvdata(pdev, sysid);
> >>>> +
> >>>> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
> >>>
> >>> You just raced with userspace and lost.  Please use the default group
> >>> for the platform device.
> >>>
> >>> I need to go remove this function, it should not be used at all as it is
> >>> broken.
> >>
> >> Can you elaborate on the issue and suggested replacement?
> >>
> >> We used this function for the SoundWire sysfs based on your review
> >> comments (2 years ago?) that we should not muck with kobj, and that
> >> function devm_device_add_group() is also used in a probe function.
> > 
> > Use the default_groups pointer in the driver structure.
> 
> did you mean dev_groups?

Yes, sorry, that's the correct name.

> I am not following the idea, for SoundWire all the attributes are really
> device-specific or described by ACPI and cannot be hard-coded in the
> driver structure.

That's what the is_visible() callback is for in the groups structure,
you determine if the attribute is visable or not at runtime, you don't
rely on the driver itself to add/remove attributes, that does not scale
and again, is racy.

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Greg KH]

On Fri, Jul 29, 2022 at 01:43:55PM +0200, Greg KH wrote:
> On Thu, Jul 28, 2022 at 11:53:33AM -0500, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 7/28/22 10:59, Greg KH wrote:
> > > On Thu, Jul 28, 2022 at 10:37:37AM -0500, Pierre-Louis Bossart wrote:
> > >> Thanks for the review Greg,
> > >>
> > >>>> +static int intel_sysid_probe(struct platform_device *pdev)
> > >>>> +{
> > >>>> +	struct intel_sysid *sysid;
> > >>>> +	struct resource	*regs;
> > >>>> +
> > >>>> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
> > >>>> +		GFP_KERNEL);
> > >>>> +	if (!sysid)
> > >>>> +		return -ENOMEM;
> > >>>> +
> > >>>> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >>>> +	if (!regs)
> > >>>> +		return -ENXIO;
> > >>>> +
> > >>>> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
> > >>>> +	if (IS_ERR(sysid->regs))
> > >>>> +		return PTR_ERR(sysid->regs);
> > >>>> +
> > >>>> +	platform_set_drvdata(pdev, sysid);
> > >>>> +
> > >>>> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
> > >>>
> > >>> You just raced with userspace and lost.  Please use the default group
> > >>> for the platform device.
> > >>>
> > >>> I need to go remove this function, it should not be used at all as it is
> > >>> broken.
> > >>
> > >> Can you elaborate on the issue and suggested replacement?
> > >>
> > >> We used this function for the SoundWire sysfs based on your review
> > >> comments (2 years ago?) that we should not muck with kobj, and that
> > >> function devm_device_add_group() is also used in a probe function.
> > > 
> > > Use the default_groups pointer in the driver structure.
> > 
> > did you mean dev_groups?
> 
> Yes, sorry, that's the correct name.
> 
> > I am not following the idea, for SoundWire all the attributes are really
> > device-specific or described by ACPI and cannot be hard-coded in the
> > driver structure.
> 
> That's what the is_visible() callback is for in the groups structure,
> you determine if the attribute is visable or not at runtime, you don't
> rely on the driver itself to add/remove attributes, that does not scale
> and again, is racy.

In looking at your attribute code, ick, you dynamically create a ton of
them.  But for the ones that you do not, you can just have the driver
core add them.  Let me make up a patch that shows what I am thinking
of...

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[Greg KH]

On Fri, Jul 29, 2022 at 01:56:25PM +0200, Greg KH wrote:
> On Fri, Jul 29, 2022 at 01:43:55PM +0200, Greg KH wrote:
> > On Thu, Jul 28, 2022 at 11:53:33AM -0500, Pierre-Louis Bossart wrote:
> > > 
> > > 
> > > On 7/28/22 10:59, Greg KH wrote:
> > > > On Thu, Jul 28, 2022 at 10:37:37AM -0500, Pierre-Louis Bossart wrote:
> > > >> Thanks for the review Greg,
> > > >>
> > > >>>> +static int intel_sysid_probe(struct platform_device *pdev)
> > > >>>> +{
> > > >>>> +	struct intel_sysid *sysid;
> > > >>>> +	struct resource	*regs;
> > > >>>> +
> > > >>>> +	sysid = devm_kzalloc(&pdev->dev, sizeof(struct intel_sysid),
> > > >>>> +		GFP_KERNEL);
> > > >>>> +	if (!sysid)
> > > >>>> +		return -ENOMEM;
> > > >>>> +
> > > >>>> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >>>> +	if (!regs)
> > > >>>> +		return -ENXIO;
> > > >>>> +
> > > >>>> +	sysid->regs = devm_ioremap_resource(&pdev->dev, regs);
> > > >>>> +	if (IS_ERR(sysid->regs))
> > > >>>> +		return PTR_ERR(sysid->regs);
> > > >>>> +
> > > >>>> +	platform_set_drvdata(pdev, sysid);
> > > >>>> +
> > > >>>> +	return devm_device_add_group(&pdev->dev, &intel_sysid_attr_group);
> > > >>>
> > > >>> You just raced with userspace and lost.  Please use the default group
> > > >>> for the platform device.
> > > >>>
> > > >>> I need to go remove this function, it should not be used at all as it is
> > > >>> broken.
> > > >>
> > > >> Can you elaborate on the issue and suggested replacement?
> > > >>
> > > >> We used this function for the SoundWire sysfs based on your review
> > > >> comments (2 years ago?) that we should not muck with kobj, and that
> > > >> function devm_device_add_group() is also used in a probe function.
> > > > 
> > > > Use the default_groups pointer in the driver structure.
> > > 
> > > did you mean dev_groups?
> > 
> > Yes, sorry, that's the correct name.
> > 
> > > I am not following the idea, for SoundWire all the attributes are really
> > > device-specific or described by ACPI and cannot be hard-coded in the
> > > driver structure.
> > 
> > That's what the is_visible() callback is for in the groups structure,
> > you determine if the attribute is visable or not at runtime, you don't
> > rely on the driver itself to add/remove attributes, that does not scale
> > and again, is racy.
> 
> In looking at your attribute code, ick, you dynamically create a ton of
> them.  But for the ones that you do not, you can just have the driver
> core add them.  Let me make up a patch that shows what I am thinking
> of...

Here's a series for this:
	https://lore.kernel.org/r/20220729135041.2285908-1-gregkh@linuxfoundation.org

I'll go fix up the other devm_device_add_groups() users, and then start
tackling devm_device_add_group() as well.

thanks,

greg k-h

Re: [PATCH v2 1/3] drivers: misc: intel_sysid: Add sysid from arch to drivers

[kernel test robot]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on soc/for-next v5.19]
[cannot apply to char-misc/char-misc-testing linus/master next-20220812]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/kah-jing-lee-intel-com/drivers-misc-intel_sysid-Add-sysid-from-arch-to-drivers/20220721-213214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 8af028c2b22bc04f5ab59cd39fa97ccf14aa8f25
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20220814/202208141908.sFdK5QCc-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/5e0d691312542fbb751afb99bd7b537b9a975750
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review kah-jing-lee-intel-com/drivers-misc-intel_sysid-Add-sysid-from-arch-to-drivers/20220721-213214
        git checkout 5e0d691312542fbb751afb99bd7b537b9a975750
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/misc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> drivers/misc/intel_sysid.c:66:24: sparse: sparse: symbol 'intel_sysid_attr_group' was not declared. Should it be static?

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp