devmem2

[Bernhard Fischer]

On Wed, Jul 11, 2007 at 03:11:26PM +0200, Marc Leeman wrote:
>A patch to include a small program that reads from/to physical memory.
>Useful for e.g. tuning the processor register map after bootup.
>
>I put in util-linux.

It isn't in the util-linux package, is it? Perhaps miscutils would be a
better place.

Generally it looks really whitespace damaged, run (from toplevel)
indent */devmem2.c, please.

>Index: busybox-1.6.1/util-linux/Config.in
>===================================================================
>--- busybox-1.6.1.orig/util-linux/Config.in
>+++ busybox-1.6.1/util-linux/Config.in
>@@ -5,6 +5,13 @@
> 
> menu "Linux System Utilities"
> 
>+config DEVMEM2
>+	bool "devmem2"
>+	default n
>+	help
>+	  devmem2 is a small program that reads and writes from physcial

physical

>+	  memory using /dev/mem.
>+
> config DMESG
> 	bool "dmesg"
> 	default n
>Index: busybox-1.6.1/util-linux/Kbuild
>===================================================================
>--- busybox-1.6.1.orig/util-linux/Kbuild
>+++ busybox-1.6.1/util-linux/Kbuild
>@@ -30,3 +30,4 @@
> lib-$(CONFIG_SWAPONOFF)		+=swaponoff.o
> lib-$(CONFIG_SWITCH_ROOT)	+=switch_root.o
> lib-$(CONFIG_UMOUNT)		+=umount.o
>+lib-$(CONFIG_DEVMEM2)		+=devmem2.o
>Index: busybox-1.6.1/util-linux/devmem2.c
>===================================================================
>--- /dev/null
>+++ busybox-1.6.1/util-linux/devmem2.c
>@@ -0,0 +1,106 @@
>+/*
>+ * devmem2.c: Simple program to read/write from/to any location in memory.
>+ *
>+ *  Copyright (C) 2000, Jan-Derk Bakker (jdb at lartmaker.nl)
>+ *  Include in busybox by Marc Leeman (marc.leeman at barco.com)
>+ *
>+ * This software has been developed for the LART computing board
>+ * (http://www.lart.tudelft.nl/). The development has been sponsored by
>+ * the Mobile MultiMedia Communications (http://www.mmc.tudelft.nl/)
>+ * and Ubiquitous Communications (http://www.ubicom.tudelft.nl/)
>+ * projects.
>+ *
>+ *
>+ * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

Please use the common short boilerplate.
>+ *
>+ */
>+
>+#include "libbb.h"
>+
>+#define FATAL do { bb_perror_msg_and_die("Error at line %d, file %s (%d) [%s]\n", \
>+  __LINE__, __FILE__, errno, strerror(errno)); exit(1); } while(0)

Is this really needed? This is really gcc centric (FILE and LINE)
Also i believe that perror already does strerror(errno). Please check
this..
>+
>+#define MAP_SIZE 4096UL
>+#define MAP_MASK (MAP_SIZE - 1)

possible name clash?
>+
>+int devmem2_main(int argc, char **argv);
>+int devmem2_main(int argc, char **argv) {
>+    int fd;
>+    void *map_base, *virt_addr;
>+	unsigned long read_result, writeval;
>+	off_t target;
>+	int access_type = 'w';
>+
>+	if(argc < 2) {
>+		 bb_show_usage();
>+	}
>+	target = strtoul(argv[1], 0, 0);
>+
>+	if(argc > 2)
>+		access_type = tolower(argv[2][0]);

If we should die on strtoul errors, there is already a wrapper for this,
please reuse.

>+
>+
>+    if((fd = xopen("/dev/mem", O_RDWR | O_SYNC)) == -1) FATAL;

xopen will die, the FATAL is thusly redundant.

>+    printf("/dev/mem opened.\n");

Smaller if you use const char bb_path_dev_mem[] = "/dev/mem"; ?
>+    fflush(stdout);

What's that good for? drop.
>+
>+    /* Map one page */
>+    map_base = mmap(0, MAP_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, target & ~MAP_MASK);
>+    if(map_base == (void *) -1) FATAL;

use MAP_FAILED not an arbitrary const.

>+    printf("Memory mapped at address %p.\n", map_base);
>+    fflush(stdout);

really need to flush here? drop.
>+
>+    virt_addr = map_base + (target & MAP_MASK);

smaller to just use map base and adjust the munmap offset later on?
>+    switch(access_type) {

Please flatten that manually or provide cycles to regtest
http://gcc.gnu.org/ml/gcc-patches/2007-04/msg00927.html

>+		case 'b':
>+			read_result = *((unsigned char *) virt_addr);
>+			break;
>+		case 'h':
>+			read_result = *((unsigned short *) virt_addr);
>+			break;
>+		case 'w':
>+			read_result = *((unsigned long *) virt_addr);
>+			break;
>+		default:
>+			bb_perror_msg_and_die("Illegal data type '%c'.\n", access_type);
>+	}
>+    printf("Value at address 0x%lx (%p): 0x%lx\n", target, virt_addr, read_result);

Sounds a bit verbose, but ok.

>+    fflush(stdout);

ditch fflush?

>+
>+	if(argc > 3) {
>+		writeval = strtoul(argv[3], 0, 0);

xatonum or the like? i.e. die on error

>+		switch(access_type) {

can't you interleave that switch statement with the above?
>+			case 'b':
>+				*((unsigned char *) virt_addr) = writeval;
>+				read_result = *((unsigned char *) virt_addr);
>+				break;
>+			case 'h':
>+				*((unsigned short *) virt_addr) = writeval;
>+				read_result = *((unsigned short *) virt_addr);
>+				break;
>+			case 'w':
>+				*((unsigned long *) virt_addr) = writeval;
>+				read_result = *((unsigned long *) virt_addr);
>+				break;
>+		}
>+		printf("Written 0x%lx; readback 0x%lx\n", writeval, read_result);
>+		fflush(stdout);

yet another flush. brrr

>+	}
>+
>+	if(munmap(map_base, MAP_SIZE) == -1) FATAL;
>+    close(fd);
>+    return 0;

Please use EXIT_SUCCESS
>+}