| 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: <20200828100236.GF1826686@dell>
Date: Fri, 28 Aug 2020 11:02:36 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Xu Yilun <yilun.xu@...el.com>
Cc: broonie@...nel.org, linux-kernel@...r.kernel.org, trix@...hat.com,
matthew.gerlach@...ux.intel.com, russell.h.weight@...el.com,
lgoncalv@...hat.com, hao.wu@...el.com
Subject: Re: [PATCH v4 2/2] mfd: intel-m10-bmc: add Max10 BMC chip support
for Intel FPGA PAC
On Wed, 19 Aug 2020, Xu Yilun wrote:
> This patch implements the basic functions of the BMC chip for some Intel
> FPGA PCIe Acceleration Cards (PAC). The BMC is implemented using the
> intel max10 CPLD.
Nit: "Intel MAX 10"
> This BMC chip is connected to FPGA by a SPI bus. To provide direct
Nit: "to *the* FPGA"
> register access from FPGA, the "SPI slave to Avalon Master Bridge"
Nit: "from *the* FPGA"
> (spi-avmm) IP is integrated in the chip. It converts encoded streams of
> bytes from the host to the internal register read/write on Avalon bus.
Nit: "on *the* Avalon bus"
> So This driver uses the regmap-spi-avmm for register accessing.
>
> Signed-off-by: Xu Yilun <yilun.xu@...el.com>
> Signed-off-by: Wu Hao <hao.wu@...el.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> Reviewed-by: Tom Rix <trix@...hat.com>
> ---
> v2: Split out the regmap-spi-avmm part.
> Rename the file intel-m10-bmc-main.c to intel-m10-bmc.c, cause
> there is only one source file left for this module now.
> v3: add the sub devices in mfd_cell.
> some minor fixes.
> v4: no change.
> ---
> .../ABI/testing/sysfs-driver-intel-m10-bmc | 15 ++
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 2 +
> drivers/mfd/intel-m10-bmc.c | 169 +++++++++++++++++++++
> include/linux/mfd/intel-m10-bmc.h | 57 +++++++
> 5 files changed, 256 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> create mode 100644 drivers/mfd/intel-m10-bmc.c
> create mode 100644 include/linux/mfd/intel-m10-bmc.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> new file mode 100644
> index 0000000..979a2d6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> @@ -0,0 +1,15 @@
> +What: /sys/bus/spi/devices/.../bmc_version
> +Date: June 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@...el.com>
> +Description: Read only. Returns the hardware build version of Intel
> + MAX10 BMC chip.
> + Format: "0x%x".
> +
> +What: /sys/bus/spi/devices/.../bmcfw_version
> +Date: June 2020
> +KernelVersion: 5.10
> +Contact: Xu Yilun <yilun.xu@...el.com>
> +Description: Read only. Returns the firmware version of Intel MAX10
> + BMC chip.
> + Format: "0x%x".
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df083..57745f5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2118,5 +2118,18 @@ config SGI_MFD_IOC3
> If you have an SGI Origin, Octane, or a PCI IOC3 card,
> then say Y. Otherwise say N.
>
> +config MFD_INTEL_M10_BMC
> + tristate "Intel MAX10 Board Management Controller"
> + depends on SPI_MASTER
> + select REGMAP_SPI_AVMM
> + select MFD_CORE
> + help
> + Support for the Intel MAX10 board management controller using the
> + SPI interface.
> +
> + This driver provides common support for accessing the device,
> + additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f8..dd2cc7b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_STMFX) += stmfx.o
> obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o
>
> obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
> +
> +obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> new file mode 100644
> index 0000000..0dfd73a
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Max10 Board Management Controller chip
"Intel MAX 10"
> + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> + *
Remove this line.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
Alphabetical.
> +enum m10bmc_type {
> + M10_N3000,
> +};
Seems over-kill. Will there be others?
> +static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
> + {
> + .name = "n3000bmc-hwmon",
> + },
> + {
> + .name = "n3000bmc-pkvl",
> + },
> + {
> + .name = "m10bmc-secure",
> + },
Each entry on one line please.
> +
Remove this line.
> +};
> +
> +static struct regmap_config intel_m10bmc_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = M10BMC_MEM_END,
> +};
> +
> +static ssize_t bmc_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
Does this line up to the '(' in code?
> + struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
> + if (ret)
> + return ret;
> +
> + return sprintf(buf, "0x%x\n", val);
Is this safe? Have you considered snprintf()?
> +}
> +static DEVICE_ATTR_RO(bmc_version);
> +
> +static ssize_t bmcfw_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct intel_m10bmc *max10 = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + ret = m10bmc_sys_read(max10, NIOS2_FW_VERSION, &val);
> + if (ret)
> + return ret;
> +
> + return sprintf(buf, "0x%x\n", val);
As above.
> +}
> +static DEVICE_ATTR_RO(bmcfw_version);
> +
> +static struct attribute *m10bmc_attrs[] = {
> + &dev_attr_bmc_version.attr,
> + &dev_attr_bmcfw_version.attr,
> + NULL,
> +};
Can this be const?
> +static struct attribute_group m10bmc_attr_group = {
> + .attrs = m10bmc_attrs,
> +};
Can this be const?
> +static const struct attribute_group *m10bmc_dev_groups[] = {
> + &m10bmc_attr_group,
> + NULL
Comma (like above).
> +};
> +
> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> +{
> + unsigned int v;
> +
> + if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> + &v))
> + return -ENODEV;
Please break functions out of if statements.
Does m10bmc_raw_read() return 0 on success?
Seems odd for a read function.
> + if (v != 0xffffffff) {
> + dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> + return -ENODEV;
> + }
The only acceptable version is -1?
> + return 0;
> +}
> +
> +static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> +{
> + const struct spi_device_id *id = spi_get_device_id(spi);
> + struct device *dev = &spi->dev;
> + struct mfd_cell *cells;
> + struct intel_m10bmc *m10bmc;
Prefer the generic 'ddata'.
> + int ret, n_cell;
> +
> + m10bmc = devm_kzalloc(dev, sizeof(*m10bmc), GFP_KERNEL);
> + if (!m10bmc)
> + return -ENOMEM;
> +
> + m10bmc->dev = dev;
> +
> + m10bmc->regmap =
> + devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
> + if (IS_ERR(m10bmc->regmap)) {
> + ret = PTR_ERR(m10bmc->regmap);
> + dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> + return ret;
> + }
> +
> + spi_set_drvdata(spi, m10bmc);
> +
> + ret = check_m10bmc_version(m10bmc);
> + if (ret) {
> + dev_err(dev, "Failed to identify m10bmc hardware\n");
> + return ret;
> + }
> +
> + switch (id->driver_data) {
> + case M10_N3000:
> + cells = m10bmc_pacn3000_subdevs;
> + n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> + break;
> + default:
> + return -ENODEV;
> + }
Will there be other versions?
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> + NULL, 0, NULL);
> + if (ret)
> + dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static const struct spi_device_id m10bmc_spi_id[] = {
> + { "m10-n3000", M10_N3000 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> +
> +static struct spi_driver intel_m10bmc_spi_driver = {
> + .driver = {
> + .name = "intel-m10-bmc",
> + .dev_groups = m10bmc_dev_groups,
> + },
> + .probe = intel_m10_bmc_spi_probe,
> + .id_table = m10bmc_spi_id,
> +};
> +
Remove this line.
> +module_spi_driver(intel_m10bmc_spi_driver);
> +
> +MODULE_DESCRIPTION("Intel Max10 BMC Device Driver");
"Intel MAX 10"
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:intel-m10-bmc");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> new file mode 100644
> index 0000000..4979756
> --- /dev/null
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Driver Header File for Intel Max10 Board Management Controller chip.
Please drop the "Driver Header File for" part.
> + * Copyright (C) 2018-2020 Intel Corporation, Inc.
> + *
Remove this line.
> + */
> +#ifndef __INTEL_M10_BMC_H
> +#define __INTEL_M10_BMC_H
"__MFD_INTEL..."
> +#include <linux/regmap.h>
> +
> +#define M10BMC_LEGACY_SYS_BASE 0x300400
> +#define M10BMC_SYS_BASE 0x300800
> +#define M10BMC_MEM_END 0x200000fc
> +
> +/* Register offset of system registers */
> +#define NIOS2_FW_VERSION 0x0
> +#define M10BMC_TEST_REG 0x3c
> +#define M10BMC_BUILD_VER 0x68
> +#define M10BMC_VER_MAJOR_MSK GENMASK(23, 16)
> +#define M10BMC_VER_PCB_INFO_MSK GENMASK(31, 24)
> +
> +/**
> + * struct intel_m10bmc - Intel Max10 BMC MFD device private data structure
"Intel MAX 10 BMC parent driver data structure"
> + * @dev: this device
> + * @regmap: the regmap used to access registers by m10bmc itself
> + */
> +struct intel_m10bmc {
> + struct device *dev;
> + struct regmap *regmap;
> +};
> +
> +/*
> + * register access helper functions.
> + *
> + * m10bmc_raw_read - read m10bmc register per addr
> + * m10bmc_sys_read - read m10bmc system register per offset
> + */
> +static inline int
> +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> + unsigned int *val)
> +{
> + int ret;
> +
> + ret = regmap_read(m10bmc->regmap, addr, val);
> + if (ret)
> + dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> + addr, ret);
> +
> + return ret;
> +}
> +
> +#define m10bmc_sys_read(m10bmc, offset, val) \
> + m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
No unnecessary abstractions.
Just use the Regmap API directly please.
> +#endif /* __INTEL_M10_BMC_H */
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists