[prev in list] [next in list] [prev in thread] [next in thread]
List: fwts-devel
Subject: Re: [PATCH] [V2] opal: mtd: Add OPAL MTD Validation
From: Colin Ian King <colin.king () canonical ! com>
Date: 2016-08-30 17:48:21
Message-ID: 9cf79769-0020-ced8-f433-b5266fab1389 () canonical ! com
[Download RAW message or body]
On 30/08/16 18:46, Deb McLemore wrote:
> Hi Colin,
>
> Let me call out here the two items left, I think we missed one of the
> inline clarifications I needed:
>
> # 1 -
>
> do we expect bytes_read to be sizeof(fdt_node_path) if successful too?
>
> On success, bytes_read will be the number of bytes actually read
>
> yup, and should that be sizeof(fdt_node_path)? if so, it may be worth
> checking for that.
>
> sizeof(fdt_node_path) is PATH_MAX so probably not going to be the full
> size normally.
Ah, OK, then ignore my request for another check. I don't think it's
necessary.
>
>
> #2 -
>
> minor nitpick - the above has the indentation from what I can tell.
>
> Can you clarify this comment, not sure what is requested ?
Oh, stupid me, I should have written:
minor nitpick - the above has the indentation missing from what I can
tell. (there seems to be one more tab required).
Sorry for the confusion.
>
>
> On 08/30/2016 12:40 PM, Colin Ian King wrote:
>> On 30/08/16 18:36, Deb McLemore wrote:
>>> Hi Colin,
>>>
>>> See replies in line and some follow-up clarifications.
>>>
>>> On 08/30/2016 12:24 PM, Colin Ian King wrote:
>>>> Thanks deb,
>>>>
>>>> I've re-reviewed the patch and it builds OK w/o any static analysis
>>>> issues too. However, I have just a couple of minor nit-picks;
>>>> apologies, I should have spotted those on the first review.
>>>>
>>>> Colin
>>>>
>>>> On 30/08/16 17:49, Deb McLemore wrote:
>>>>> Check that the MTD system device for OPAL is properly setup.
>>>>>
>>>>> Signed-off-by: Deb McLemore <debmc@linux.vnet.ibm.com>
>>>>> ---
>>>>> configure.ac | 2 +
>>>>> src/Makefile.am | 1 +
>>>>> src/opal/mtd_info.c | 379
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 382 insertions(+)
>>>>> create mode 100644 src/opal/mtd_info.c
>>>>>
>>>>> diff --git a/configure.ac b/configure.ac
>>>>> index e3e7512..042d45b 100644
>>>>> --- a/configure.ac
>>>>> +++ b/configure.ac
>>>>> @@ -64,7 +64,9 @@
>>>>> AC_CHECK_HEADERS([glib.h])
>>>>> AC_CHECK_HEADERS([gio/gio.h])
>>>>> AC_CHECK_HEADERS([asm/opal-prd.h])
>>>>> + AC_CHECK_HEADERS([mtd/mtd-abi.h])
>>>>> AM_CONDITIONAL([HAVE_ASM_OPAL_PRD_H], [test
>>>>> "x$ac_cv_header_asm_opal_prd_h" = "xyes"])
>>>>> + AM_CONDITIONAL([HAVE_MTD_ABI_H], [test
>>>>> "x$ac_cv_header_mtd_abi_h" = "xyes"])
>>>>> #AC_CHECK_LIB(pcre, pcre_compile)
>>>>> AC_SEARCH_LIBS([fdt_check_header], [fdt], [
>>>>> AC_DEFINE([HAVE_LIBFDT], [1], [Define if we have libfdt])
>>>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>>>> index 137a429..eeef7a8 100644
>>>>> --- a/src/Makefile.am
>>>>> +++ b/src/Makefile.am
>>>>> @@ -134,6 +134,7 @@ fwts_SOURCES = main.c \
>>>>> kernel/olog/olog.c \
>>>>> kernel/oops/oops.c \
>>>>> kernel/version/version.c \
>>>>> + opal/mtd_info.c \
>>>>> opal/prd_info.c \
>>>>> pci/aspm/aspm.c \
>>>>> pci/crs/crs.c \
>>>>> diff --git a/src/opal/mtd_info.c b/src/opal/mtd_info.c
>>>>> new file mode 100644
>>>>> index 0000000..edb25fa
>>>>> --- /dev/null
>>>>> +++ b/src/opal/mtd_info.c
>>>>> @@ -0,0 +1,379 @@
>>>>> +/*
>>>>> + * Copyright (C) 2010-2016 Canonical
>>>>> + * Some of this work - Copyright (C) 2016 IBM
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU General Public License
>>>>> + * as published by the Free Software Foundation; either version 2
>>>>> + * of the License, or (at your option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License
>>>>> + * along with this program; if not, write to the Free Software
>>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>>>> 02110-1301, USA.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#define _GNU_SOURCE /* added for asprintf */
>>>>> +#include <fcntl.h>
>>>>> +#include <sys/ioctl.h>
>>>>> +#include <stdio.h>
>>>>> +
>>>>> +#include "fwts.h"
>>>>> +
>>>>> +#ifdef HAVE_MTD_MTD_ABI_H
>>>>> +#include <mtd/mtd-abi.h>
>>>>> +#endif
>>>>> +
>>>>> +#ifdef HAVE_LIBFDT
>>>>> +#include <libfdt.h>
>>>>> +#endif
>>>>> +
>>>>> +#define FDT_FLASH_PATH "/proc/device-tree/chosen/ibm,system-flash"
>>>>> +#define SYSFS_MTD_PATH "/sys/class/mtd"
>>>>> +
>>>>> +bool mtd_present(int fwts_mtd_flags, char *mtd_devnode)
>>>>> +{
>>>>> + return !access(mtd_devnode, fwts_mtd_flags);
>>>>> +}
>>>>> +
>>>>> +int mtd_hdr_query(fwts_framework *fw, char *mtd_devnode) {
>>>>> +
>>>>> + /* snippet from skiboot libflash/ffs.h */
>>>>> + struct ffs_hdr {
>>>>> + char magic[8];
>>>>> + };
>>>>> + int fd = 0;
>>>>> + ssize_t bytes_read = 0;
>>>>> + struct ffs_hdr mtd_hdr;
>>>>> +
>>>>> + if ((fd = open(mtd_devnode, O_RDONLY)) < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL, "OPAL MTD Info",
>>>>> + "Cannot get data from MTD device '%s'.",
>>>>> + mtd_devnode);
>>>>> + return FWTS_ERROR;
>>>>> + }
>>>>> +
>>>>> + bytes_read = read(fd, &mtd_hdr, sizeof(mtd_hdr) );
>>>>> +
>>>>> + if (bytes_read >= 4) {
>>>>> + /* FFS_MAGIC from skiboot libflash/ffs.h */
>>>>> + if (strncmp(mtd_hdr.magic, "PART", 4) == 0) {
>>>>> + fwts_log_info(fw,
>>>>> + "MTD device '%s' header eye-catcher 'PART'"
>>>>> + " verified.\n", mtd_devnode);
>>>>> + } else {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "MTD device '%s' header eye-catcher 'PART'"
>>>>> + " not able to be"
>>>>> + " verified. Check the system setup.\n",
>>>>> + mtd_devnode);
>>>>> + close(fd);
>>>>> + return FWTS_ERROR;
>>>>> + }
>>>>> + } else {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL, "OPAL MTD Info",
>>>>> + "MTD device '%s' header was unable to be read."
>>>>> + " Cannot validate the integrity of the MTD."
>>>>> + " Check the system setup.\n",
>>>>> + mtd_devnode);
>>>>> + close(fd);
>>>>> + return FWTS_ERROR;
>>>>> + }
>>>>> +
>>>>> + close(fd);
>>>>> + return FWTS_OK;
>>>>> +}
>>>>> +
>>>>> +int mtd_dev_query(fwts_framework *fw, char *mtd_devnode)
>>>>> +{
>>>>> +
>>>>> +#ifdef HAVE_MTD_MTD_ABI_H
>>>>> + int fd = 0;
>>>>> + int fwts_mtd_flags = 0;
>>>>> + struct mtd_info_user mtd_info;
>>>>> +#endif
>>>>> +
>>>>> + if (!mtd_present(R_OK | W_OK, mtd_devnode)) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL, "OPAL MTD Info",
>>>>> + "Cannot read or write to MTD device '%s'"
>>>>> + " check your user privileges.", mtd_devnode);
>>>>> + return FWTS_ERROR;
>>>>> + } else {
>>>>> + fwts_log_info(fw, "MTD device '%s' is verified"
>>>>> + " and %s is read/write in the file system, the"
>>>>> + " MTD device itself will be checked later,"
>>>>> + " see MTD Flags.",
>>>>> + mtd_devnode, mtd_devnode);
>>>>> + }
>>>>> +
>>>>> +#ifdef HAVE_MTD_MTD_ABI_H
>>>>> + if (strstr(mtd_devnode, "ro")) {
>>>>> + fwts_mtd_flags = O_RDONLY;
>>>>> + } else {
>>>>> + fwts_mtd_flags = O_RDWR;
>>>>> + }
>>>>> +
>>>>> + if ((fd = open(mtd_devnode, fwts_mtd_flags)) < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL, "OPAL MTD Info",
>>>>> + "Cannot get data from '%s'"
>>>>> + " device interface.", mtd_devnode);
>>>>> + return FWTS_ERROR;
>>>>> + }
>>>>> +
>>>>> + if (ioctl(fd, MEMGETINFO, &mtd_info)) {
>>>>> + close(fd);
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL, "OPAL MTD Info",
>>>>> + "Cannot get data from '%s'"
>>>>> + " device interface.", mtd_devnode);
>>>>> + return FWTS_ERROR;
>>>>> + } else {
>>>>> + fwts_log_info(fw, "MTD device '%s' attributes follow:"
>>>>> + " MTD Type=%u (3=MTD_NORFLASH),"
>>>>> + " MTD Flags=%u (1024=MTD_WRITEABLE),"
>>>>> + " MTD total size=%u bytes,"
>>>>> + " MTD erase size=%u bytes,"
>>>>> + " MTD write size=%u,"
>>>>> + " MTD oob size=%u",
>>>>> + mtd_devnode,
>>>>> + mtd_info.type,
>>>>> + mtd_info.flags,
>>>>> + mtd_info.size,
>>>>> + mtd_info.erasesize,
>>>>> + mtd_info.writesize,
>>>>> + mtd_info.oobsize);
>>>>> + close(fd);
>>>>> + return FWTS_OK;
>>>>> + }
>>>>> +}
>>>>> +#else
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "MTD Info unable to be retrieved"
>>>>> + " for '%s' due to lack of "
>>>>> + "mtd-abi.h, check your "
>>>>> + "configuration and rebuild.",
>>>>> + mtd_devnode);
>>>>> + return FWTS_ERROR;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +static int mtd_info_test1(fwts_framework *fw)
>>>>> +{
>>>>> + char fdt_node_path[PATH_MAX];
>>>>> + int count, i, fd;
>>>>> + ssize_t bytes = 0, bytes_read = 0;
>>>>> + struct dirent **namelist;
>>>>> +
>>>>> + fd = open(FDT_FLASH_PATH, O_RDONLY);
>>>>> + if (fd < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "Failed to open the path %s."
>>>>> + " Check the installation"
>>>>> + " for the path %s.\n",
>>>>> + FDT_FLASH_PATH,
>>>>> + FDT_FLASH_PATH);
>>>>> + return FWTS_ERROR;
>>>>> + }
>>>>> + bytes_read = read(fd, fdt_node_path, sizeof(fdt_node_path));
>>>> do we expect bytes_read to be sizeof(fdt_node_path) if successful too?
>>> On success, bytes_read will be the number of bytes actually read
>> yup, and should that be sizeof(fdt_node_path)? if so, it may be worth
>> checking for that.
>>
>>>>> + if (bytes_read < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "Failed to get the FDT info."
>>>>> + " Check the installation "
>>>>> + "for the path %s.\n",
>>>>> + FDT_FLASH_PATH);
>>>>> + close(fd);
>>>>> + return FWTS_ERROR;
>>>>> + }
>>>>> + close(fd);
>>>>> + fwts_log_info(fw, "MTD Info validated FDT of '%s'.",
>>>>> + fdt_node_path);
>>>>> +
>>>>> + count = scandir(SYSFS_MTD_PATH, &namelist, NULL, alphasort);
>>>>> + if (count < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "Scan for MTD '%s' unable to find any "
>>>>> + "candidates. Check the installation "
>>>>> + "for the MTD device config.",
>>>>> + SYSFS_MTD_PATH);
>>>>> + }
>>>>> +
>>>>> + bytes = 0;
>>>>> +
>>>>> + fwts_log_nl(fw);
>>>>> + fwts_log_info(fw, "STARTING checks of MTD devices");
>>>>> + fwts_log_nl(fw);
>>>>> +
>>>>> + for (i = 0; i < count; i++) {
>>>>> + struct dirent *dirent;
>>>>> + char *sys_device_path; /* /sys/class/device/mtdx */
>>>>> + char *mtd_device_path; /* /dev/mtdx */
>>>>> + char *driver_path;
>>>>> + char fdt_node_path_tmp[PATH_MAX];
>>>>> + char mtd_driver_path[PATH_MAX];
>>>>> +
>>>>> + memset(fdt_node_path_tmp, 0, sizeof(fdt_node_path_tmp));
>>>>> + memset(mtd_driver_path, 0, sizeof(mtd_driver_path));
>>>>> +
>>>>> + dirent = namelist[i];
>>>>> +
>>>>> + if (dirent->d_name[0] == '.' || bytes ||
>>>>> + asprintf(&sys_device_path,
>>>>> + "%s/%s/device/of_node",
>>>>> + SYSFS_MTD_PATH, dirent->d_name) < 0) {
>>>>> + /* asprintf must be last condition so when it */
>>>>> + /* evaluates sys_device_path gets allocated */
>>>>> + free(namelist[i]);
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + bytes = readlink(sys_device_path, fdt_node_path_tmp,
>>>>> + sizeof(fdt_node_path_tmp) - 1);
>>>>> + free(sys_device_path);
>>>>> + if (bytes < 0) {
>>>>> + /* if mtd system flash does not have an FDT node */
>>>>> + /* just continue */
>>>> minor nitpick, can the two comments above be indented by 1 tab
>>> Done
>>>>> + free(namelist[i]);
>>>>> + /* reset the bytes to continue */
>>>>> + bytes = 0;
>>>>> + continue;
>>>>> + }
>>>>> + fdt_node_path_tmp[bytes] = '\0';
>>>>> +
>>>>> + if (strstr(fdt_node_path_tmp, fdt_node_path)) {
>>>>> + bytes = asprintf(&mtd_device_path, "/dev/%s",
>>>>> + dirent->d_name);
>>>>> + if (bytes < 0) {
>>>>> + free(namelist[i]);
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "Failed to get the device path."
>>>>> + " Check the installation for the"
>>>>> + " path '/dev/%s'.",
>>>>> + dirent->d_name);
>>>>> + continue;
>>>>> + }
>>>>> + mtd_device_path[bytes] = '\0';
>>>>> + bytes = 0;
>>>>> + if (asprintf(&driver_path,
>>>>> + "%s/%s/device/driver",
>>>>> + SYSFS_MTD_PATH, dirent->d_name) < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "Failed to get the MTD Path."
>>>>> + " Check the installation "
>>>>> + "for the path '%s/%s/device/driver'.",
>>>>> + SYSFS_MTD_PATH,
>>>>> + dirent->d_name);
>>>>> + free(mtd_device_path);
>>>>> + continue;
>>>>> + }
>>>>> + bytes = readlink(driver_path, mtd_driver_path,
>>>>> + sizeof(mtd_driver_path) -1);
>>>>> + if (bytes < 0) {
>>>>> + fwts_failed(fw, LOG_LEVEL_CRITICAL,
>>>>> + "OPAL MTD Info",
>>>>> + "Failed to get the MTD drive path."
>>>>> + " Check the installation for the "
>>>>> + "path %s.",
>>>>> + driver_path);
>>>>> + free(mtd_device_path);
>>>>> + free(driver_path);
>>>>> + continue;
>>>>> + } else {
>>>>> + mtd_driver_path[bytes] = '\0';
>>>>> + bytes = 0;
>>>>> + free(driver_path);
>>>>> + }
>>>>> +
>>>>> + if (strstr(mtd_driver_path, "powernv_flash")) {
>>>>> + if (!strstr(mtd_device_path, "ro")) {
>>>>> + if (mtd_dev_query(fw,
>>>>> + mtd_device_path)) {
>>>>> + /* failures logged in subroutine */
>>>>> + free(mtd_device_path);
>>>>> + continue;
>>>>> + }
>>>>> + if (mtd_hdr_query(fw,
>>>>> + mtd_device_path)) {
>>>>> + /* failures logged in subroutine */
>>>>> + free(mtd_device_path);
>>>>> + continue;
>>>>> + }
>>>>> + fwts_log_nl(fw);
>>>> minor nitpick - the above has the indentation from what I can tell.
>>> Can you clarify this comment, not sure what is requested ?
>>>>> + }
>>>>> + free(mtd_device_path);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + free(namelist[i]);
>>>>> + }
>>>>> + free(namelist);
>>>>> +
>>>>> + fwts_log_info(fw, "ENDING checks of MTD devices");
>>>>> + fwts_log_nl(fw);
>>>>> +
>>>>> + fwts_passed(fw, "OPAL MTD info passed.");
>>>>> +
>>>>> + return FWTS_OK;
>>>>> +}
>>>>> +
>>>>> +static int mtd_info_init(fwts_framework *fw)
>>>>> +{
>>>>> + if (fw->fdt) {
>>>>> +#ifdef HAVE_LIBFDT
>>>>> + int node;
>>>>> + /* perform some FDT validation */
>>>>> + node = fdt_path_offset(fw->fdt,
>>>>> + "/ibm,opal/nvram");
>>>>> + if (node >= 0) {
>>>>> + if (!fdt_node_check_compatible(fw->fdt, node,
>>>>> + "ibm,opal-nvram")) {
>>>>> + fwts_log_info(fw,
>>>>> + "MTD Info initialization validated"
>>>>> + " FDT for 'ibm,opal-nvram'.");
>>>>> + return FWTS_OK;
>>>>> + } else {
>>>>> + return FWTS_SKIP;
>>>>> + }
>>>>> + }
>>>>> +#endif
>>>>> + } else {
>>>>> + fwts_log_info(fw, "The OPAL MTD device tree node is not"
>>>>> + " able to be detected so skipping the mtd_info"
>>>>> + " test. There may be tools missing such as"
>>>>> + " libfdt-dev or dtc, check that the packages"
>>>>> + " are installed and re-build if needed."
>>>>> + " If this condition persists try running the"
>>>>> + " dt_base test to further diagnose. If dt_base"
>>>>> + " test is not available this is probably a"
>>>>> + " setup problem.");
>>>>> + return FWTS_SKIP;
>>>>> + }
>>>>> +
>>>>> + /* only run test when fdt node is confirmed */
>>>>> + return FWTS_SKIP;
>>>>> +}
>>>>> +
>>>>> +static fwts_framework_minor_test mtd_info_tests[] = {
>>>>> + { mtd_info_test1, "OPAL MTD Info" },
>>>>> + { NULL, NULL }
>>>>> +};
>>>>> +
>>>>> +static fwts_framework_ops mtd_info_ops = {
>>>>> + .description = "OPAL MTD Info",
>>>>> + .init = mtd_info_init,
>>>>> + .minor_tests = mtd_info_tests
>>>>> +};
>>>>> +
>>>>> +FWTS_REGISTER_FEATURES("mtd_info", &mtd_info_ops, FWTS_TEST_ANYTIME,
>>>>> + FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV,
>>>>> + FWTS_FW_FEATURE_DEVICETREE);
>>>>>
>
--
fwts-devel mailing list
fwts-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic