| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <34191e6f-9a00-114e-4fd8-15ccaec3535b@linaro.org>
Date: Tue, 10 Jan 2023 10:46:57 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: clayc@....com, linux-kernel@...r.kernel.org, soc@...nel.org,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
verdun@....com, nick.hawkins@....com, arnd@...db.de,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
linux@...linux.org.uk, olof@...om.net
Subject: Re: [PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver
On 10/01/2023 05:25, clayc@....com wrote:
> From: Clay Chang <clayc@....com>
>
> The GXP SROM control register can be used to configure LPC related
> legacy I/O registers. Currently only the SROM RAM Offset Register
> (vromoff) is exported.
>
> The GXP SOCLIB is a common library used for creating the common
> "soc" class in the kernel.
>
> Signed-off-by: Clay Chang <clayc@....com>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/hpe/Kconfig | 29 +++++++
> drivers/soc/hpe/Makefile | 2 +
> drivers/soc/hpe/gxp-soclib.c | 17 +++++
> drivers/soc/hpe/gxp-soclib.h | 9 +++
> drivers/soc/hpe/gxp-srom.c | 141 +++++++++++++++++++++++++++++++++++
> 7 files changed, 200 insertions(+)
> create mode 100644 drivers/soc/hpe/Kconfig
> create mode 100644 drivers/soc/hpe/Makefile
> create mode 100644 drivers/soc/hpe/gxp-soclib.c
> create mode 100644 drivers/soc/hpe/gxp-soclib.h
> create mode 100644 drivers/soc/hpe/gxp-srom.c
>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 5dbb09f843f7..faff0f036b61 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -10,6 +10,7 @@ source "drivers/soc/bcm/Kconfig"
> source "drivers/soc/canaan/Kconfig"
> source "drivers/soc/fsl/Kconfig"
> source "drivers/soc/fujitsu/Kconfig"
> +source "drivers/soc/hpe/Kconfig"
> source "drivers/soc/imx/Kconfig"
> source "drivers/soc/ixp4xx/Kconfig"
> source "drivers/soc/litex/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index fff513bd522d..d257b9d654b3 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MACH_DOVE) += dove/
> obj-y += fsl/
> obj-y += fujitsu/
> obj-$(CONFIG_ARCH_GEMINI) += gemini/
> +obj-$(CONFIG_ARCH_HPE) += hpe/
> obj-y += imx/
> obj-y += ixp4xx/
> obj-$(CONFIG_SOC_XWAY) += lantiq/
> diff --git a/drivers/soc/hpe/Kconfig b/drivers/soc/hpe/Kconfig
> new file mode 100644
> index 000000000000..88f5d46b06b6
> --- /dev/null
> +++ b/drivers/soc/hpe/Kconfig
> @@ -0,0 +1,29 @@
> +#
> +# HPE GXP SoC drivers
> +#
> +menu "HPE GXP SoC drivers"
> + depends on ARCH_HPE || COMPILE_TEST
> +
> +config HPE_GXP_SOCLIB
> + bool "GXP Common SoC Library"
> + default y
> + depends on ARCH_HPE_GXP || COMPILE_TEST
> + help
> + This is for the common library for all HPE SoC drivers. It
> + creates the root soc class (/sys/class/soc) for all GXP SoC
> + drivers. It must be yes if any one of the GXP SoC drivers were
> + added, so the config of all GXP SoC drivers must select this.
Don't open-code Kconfig dependencies in free-form text. IOW, drop last
sentence.
> +
> +
> +config HPE_GXP_SROM
> + bool "GXP SROM Configuration Driver"
> + default y
> + depends on ARCH_HPE_GXP || COMPILE_TEST
> + select HPE_GXP_SOCLIB
> + help
> + Say yes here to add support for SROM Configuration. The GXP SROM
> + control register can be used to configure LPC related legacy I/O
> + registers. Currently only the SROM RAM Offset Register (vromoff)
> + is exported.
> +
> +endmenu
> diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
> new file mode 100644
> index 000000000000..78de24ecb606
> --- /dev/null
> +++ b/drivers/soc/hpe/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
> +obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
> diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
> new file mode 100644
> index 000000000000..11b0afe09070
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-soclib.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2023 Hewlett Packard Enteprise Development Company, L.P. */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +
> +struct class *soc_class;
> +
> +static int __init gxp_soclib_init(void)
> +{
> + soc_class = class_create(THIS_MODULE, "soc");
> + if (IS_ERR(soc_class))
> + return PTR_ERR(soc_class);
> + return 0;
> +}
> +
> +module_init(gxp_soclib_init);
I don't see a point of making it a shared object and a "kernel library".
Module inits are not libraries. Drop entire file.
> diff --git a/drivers/soc/hpe/gxp-soclib.h b/drivers/soc/hpe/gxp-soclib.h
> new file mode 100644
> index 000000000000..eb0e72b67aee
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-soclib.h
> @@ -0,0 +1,9 @@
> +
> +static int sysfs_register(struct device *parent,
> + struct gxp_srom_drvdata *drvdata)
> +{
> + struct device *dev;
> +
> + dev = device_create_with_groups(soc_class, parent, 0,
> + drvdata, srom_groups, "srom");
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> + drvdata->dev = dev;
> + return 0;
> +}
> +
> +static int gxp_srom_probe(struct platform_device *pdev)
> +{
> + struct gxp_srom_drvdata *drvdata;
> + struct resource *res;
> + int ret;
> +
> + drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_srom_drvdata),
sizeof(*)
> + GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->pdev = pdev;
> + platform_set_drvdata(pdev, drvdata);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "no srom resource defined\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + drvdata->base = devm_ioremap_resource(&pdev->dev, res);
Combine these two. There is a helper for this.
> + if (IS_ERR(drvdata->base)) {
> + dev_err(&pdev->dev, "ioremap_resource error\n");
> + ret = PTR_ERR(drvdata->base);
return dev_err_probe().
> + goto ioremap_err;
> + }
> +
> + mutex_init(&drvdata->mutex);
> +
> + ret = sysfs_register(&pdev->dev, drvdata);
Missing ABI documentation.
> + if (ret != 0) {
> + dev_err(&pdev->dev, "sysfs error\n");
return dev_err_probe
> + goto sysfs_err;
> + }
> +
> + dev_info(&pdev->dev, "initialized\n");
Drop silly probe successes.
> + return 0;
> +
> +sysfs_err:
> +ioremap_err:
> + platform_set_drvdata(pdev, NULL);
> +out:
> + if (drvdata)
> + devm_kfree(&pdev->dev, (void *)drvdata);
1. Why? if it is here, it must be in remove() callback. If it is invalid
(because it is really not correct) for remove(), it cannot be here, right?
2. Why cast?
> + return ret;
> +}
> +
> +static const struct of_device_id gxp_srom_of_match[] = {
> + { .compatible = "hpe,gxp-srom" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, gxp_srom_of_match);
> +
> +static struct platform_driver gxp_srom_driver = {
> + .probe = gxp_srom_probe,
> + .driver = {
> + .name = "gxp-srom",
> + .of_match_table = of_match_ptr(gxp_srom_of_match),
That will cause a warning. Drop of_match_ptr.
> + },
> +};
> +module_platform_driver(gxp_srom_driver);
> +
> +MODULE_AUTHOR("Clay Chang <clayc@....com>");
> +MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver");
> +MODULE_LICENSE("GPL");
Best regards,
Krzysztof
Powered by blists - more mailing lists