From: Kah Jing Lee <[email protected]>
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 <[email protected]>
Reviewed-by: Zhou, Furong <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
---
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 <[email protected]>
+ * Kah Jing Lee <[email protected]>
+ */
+
+#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 <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Intel System ID driver");
+MODULE_ALIAS("platform:" DRV_NAME);
--
2.25.1
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/[email protected]/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 <[email protected]>
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
On Thu, Jul 21, 2022 at 08:31:50PM +0800, [email protected] wrote:
> From: Kah Jing Lee <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Zhou, Furong <[email protected]>
> Reviewed-by: Pierre-Louis Bossart <[email protected]>
> ---
> 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 <[email protected]>
> + * Kah Jing Lee <[email protected]>
> + */
> +
> +#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
On Thu, Jul 21, 2022 at 08:31:50PM +0800, [email protected] wrote:
> From: Kah Jing Lee <[email protected]>
>
> 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
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
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
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.
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
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
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/[email protected]
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
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/[email protected]/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 <[email protected]>
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