Re: [PATCH] commands: import memtester 4.3.0 from Debian GNU/Linux

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Peter,

On Wed, Aug 26, 2020 at 05:26:32PM +0300, Peter Mamonov wrote:
> diff --git a/commands/memtester/memtester.c b/commands/memtester/memtester.c
> new file mode 100644
> index 0000000000..7be6a9c693
> --- /dev/null
> +++ b/commands/memtester/memtester.c

Is this file original memtester code or have you written it for barebox?
It looks like originally memtester but not much is left from it. Could
you put the barebox part into a separate (new) file for easier future
updates?

> @@ -0,0 +1,316 @@
> +/*
> + * memtester version 4
> + *
> + * Very simple but very effective user-space memory tester.
> + * Originally by Simon Kirby <sim@stormix.com> <sim@neato.org>
> + * Version 2 by Charles Cazabon <charlesc-memtester@pyropus.ca>
> + * Version 3 not publicly released.
> + * Version 4 rewrite:
> + * Copyright (C) 2004-2012 Charles Cazabon <charlesc-memtester@pyropus.ca>
> + * Licensed under the terms of the GNU General Public License version 2 (only).
> + * See the file COPYING for details.
> + *
> + */
> +
> +#define __version__ "4.3.0"
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <getopt.h>
> +#include <common.h>
> +#include <command.h>
> +#include <environment.h>
> +#include <fs.h>
> +
> +#include "types.h"
> +#include "sizes.h"
> +#include "tests.h"
> +
> +#define EXIT_FAIL_NONSTARTER    0x01
> +#define EXIT_FAIL_ADDRESSLINES  0x02
> +#define EXIT_FAIL_OTHERTEST     0x04
> +
> +struct test tests[] = {
> +    { "Random Value", test_random_value },
> +    { "Compare XOR", test_xor_comparison },
> +    { "Compare SUB", test_sub_comparison },
> +    { "Compare MUL", test_mul_comparison },
> +    { "Compare DIV",test_div_comparison },
> +    { "Compare OR", test_or_comparison },
> +    { "Compare AND", test_and_comparison },
> +    { "Sequential Increment", test_seqinc_comparison },
> +    { "Solid Bits", test_solidbits_comparison },
> +    { "Block Sequential", test_blockseq_comparison },
> +    { "Checkerboard", test_checkerboard_comparison },
> +    { "Bit Spread", test_bitspread_comparison },
> +    { "Bit Flip", test_bitflip_comparison },
> +    { "Walking Ones", test_walkbits1_comparison },
> +    { "Walking Zeroes", test_walkbits0_comparison },
> +    { "8-bit Writes", test_8bit_wide_random },
> +    { "16-bit Writes", test_16bit_wide_random },
> +    { NULL, NULL }
> +};

Should be static

> +
> +/* Function declarations */
> +
> +/* Global vars - so tests have access to this information */
> +int use_phys = 0;
> +off_t physaddrbase = 0;
> +
> +static int do_memtester(int argc, char **argv) {
> +    ul loops, loop, i;
> +    size_t wantraw, wantmb, wantbytes, wantbytes_orig, bufsize,
> +         halflen, count;
> +    char *memsuffix, *addrsuffix, *loopsuffix;
> +    void volatile *buf, *aligned;
> +    ulv *bufa, *bufb;
> +    int exit_code = 0, ret;
> +    int memfd = 0, opt, memshift;
> +    size_t maxbytes = -1; /* addressable memory, in bytes */
> +    size_t maxmb = (maxbytes >> 20) + 1; /* addressable memory, in MB */
> +    /* Device to mmap memory from with -p, default is normal core */
> +    char *device_name = "/dev/mem";
> +    struct stat statbuf;
> +    int device_specified = 0;
> +    const char *env_testmask = 0;
> +    ul testmask = 0;
> +
> +    printf("memtester version " __version__ " (%d-bit)\n", UL_LEN);
> +    printf("Copyright (C) 2001-2012 Charles Cazabon.\n");
> +    printf("Licensed under the GNU General Public License version 2 (only).\n");
> +    printf("\n");
> +
> +    /* If MEMTESTER_TEST_MASK is set, we use its value as a mask of which
> +       tests we run.
> +     */
> +    if ((env_testmask = getenv("MEMTESTER_TEST_MASK"))) {i

Why is this an envrionment variable? I would expect this to be a
commandline option.

> +        errno = 0;
> +        testmask = simple_strtoul(env_testmask, 0, 0);
> +        if (errno) {
> +            printf("error parsing MEMTESTER_TEST_MASK %s: %s\n",
> +                    env_testmask, strerror(errno));
> +            return COMMAND_ERROR_USAGE;
> +        }
> +        printf("using testmask 0x%lx\n", testmask);
> +    }
> +
> +    while ((opt = getopt(argc, argv, "p:d:")) != -1) {
> +        switch (opt) {
> +            case 'p':
> +                errno = 0;
> +                physaddrbase = (off_t) simple_strtoull(optarg, &addrsuffix, 16);
> +                if (errno != 0) {
> +                    printf("failed to parse physaddrbase arg; should be hex "
> +                           "address (0x123...)\n");
> +                    return COMMAND_ERROR_USAGE;
> +                }
> +                if (*addrsuffix != '\0') {
> +                    /* got an invalid character in the address */
> +                    printf("failed to parse physaddrbase arg; should be hex "
> +                           "address (0x123...)\n");
> +                    return COMMAND_ERROR_USAGE;
> +                }
> +                /* okay, got address */
> +                use_phys = 1;
> +                break;
> +            case 'd':
> +                if (stat(optarg,&statbuf)) {
> +                    printf("can not use %s as device: %s\n", optarg,
> +                            strerror(errno));
> +                    return COMMAND_ERROR_USAGE;
> +                } else {
> +                    if (!S_ISCHR(statbuf.st_mode)) {
> +                        printf("can not mmap non-char device %s\n",
> +                                optarg);
> +                        return COMMAND_ERROR_USAGE;
> +                    } else {
> +                        device_name = optarg;
> +                        device_specified = 1;
> +                    }
> +                }
> +                break;
> +            default: /* '?' */
> +                return COMMAND_ERROR_USAGE;
> +        }
> +    }
> +    if (device_specified && !use_phys) {
> +        printf("for mem device, physaddrbase (-p) must be specified\n");
> +        return COMMAND_ERROR_USAGE;
> +    }
> +
> +    if (optind >= argc) {
> +        printf("need memory argument, in MB\n");
> +        return COMMAND_ERROR_USAGE;
> +    }
> +
> +    errno = 0;
> +    wantraw = (size_t) simple_strtoul(argv[optind], &memsuffix, 0);
> +    if (errno != 0) {
> +        printf("failed to parse memory argument");
> +        return COMMAND_ERROR_USAGE;
> +    }
> +    switch (*memsuffix) {
> +        case 'G':
> +        case 'g':
> +            memshift = 30; /* gigabytes */
> +            break;
> +        case 'M':
> +        case 'm':
> +            memshift = 20; /* megabytes */
> +            break;
> +        case 'K':
> +        case 'k':
> +            memshift = 10; /* kilobytes */
> +            break;
> +        case 'B':
> +        case 'b':
> +            memshift = 0; /* bytes*/
> +            break;
> +        case '\0':  /* no suffix */
> +            memshift = 20; /* megabytes */
> +            break;
> +        default:
> +            /* bad suffix */
> +            return COMMAND_ERROR_USAGE;
> +    }

We have strtoull_suffix() for this purpose. Also for this case have a
look at parse_area_spec(). With this you can specify a memory region
with <start>-<end> or <start>+<size> with start/end/size given in
decimal or hex with an optional [kMG] suffix.

> +    wantbytes_orig = wantbytes = ((size_t) wantraw << memshift);
> +    wantmb = (wantbytes_orig >> 20);
> +    optind++;
> +    if (wantmb > maxmb) {
> +        printf("This system can only address %llu MB.\n", (ull) maxmb);
> +        return EXIT_FAIL_NONSTARTER;
> +    }

Please check the error codes. EXIT_FAIL_NONSTARTER is 2, just like
COMMAND_ERROR_USAGE. This is probably not what you want.

I am not looking at the actual tests I assume these are taken from
memtester directly and are ok as such.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox