new applet: devmem

[Bastian Blank]

On Sun, Oct 26, 2008 at 05:10:45AM -0400, Mike Frysinger wrote:
> +#define devmem_full_usage \
> +	"Read/Write from physical addresses" \
> +	"\n\nUsage:  devmem { address } [ type [ data ] ]" \
> +	"\n	address : memory address to act upon" \
> +	"\n	type    : access operation type : [b]yte, [h]alfword, [w]ord" \
> +	"\n	data    : data to be written"

Where is the definition of halfword and word? I assume 16 and 32 bit.
And what about 64 bit access?

> +config DEVMEM
> +	bool "devmem"
> +	default y

Why default y? This is no generic tool.

> +#define DEVMEM_MAP_SIZE 4096UL

I think this needs to match the pagesize, which is not fixed to 4096.

> +int devmem_main(int argc, char **argv) {
> +	void *map_base, *virt_addr;
> +	unsigned long read_result, writeval;
> +	off_t target;
> +	int fd, access_type = 'w';

Default value is not documented in the help.

> +	if (argc < 2)
> +		bb_show_usage();
> +
> +	target = bb_strtoul(argv[1], 0, 0);
> +
> +	if (argc > 2)
> +		access_type = tolower(argv[2][0]);
> +
> +	fd = xopen("/dev/mem", O_RDWR | O_SYNC);
> +
> +	if ((map_base = mmap(0, DEVMEM_MAP_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, target & ~DEVMEM_MAP_MASK)) == MAP_FAILED)

Where is overflow handled?

> +	printf("Memory mapped at address %p.\n", map_base);
> +
> +	virt_addr = map_base + (target & DEVMEM_MAP_MASK);

map_base is void *, aka no aritmetic allowed.

> +	if (access_type == 'b')
> +		read_result = *((unsigned char *) virt_addr);
> +	else if (access_type == 'h')
> +		read_result = *((unsigned short *) virt_addr);
> +	else if (access_type == 'w')
> +		read_result = *((unsigned long *) virt_addr);

This definitions are odd. long is 64bit on several arches.

> +	else {
> +		fprintf(stderr, "Illegal data type '%c'\n", access_type);
> +		exit(EXIT_FAILURE);

This belongs to the argument check.

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

Error checks?

> +		if (access_type == 'b') {
> +			*((unsigned char *) virt_addr) = writeval;
> +			read_result = *((unsigned char *) virt_addr);
> +		} else if (access_type == 'h') {
> +			*((unsigned short *) virt_addr) = writeval;
> +			read_result = *((unsigned short *) virt_addr);
> +		} else if (access_type == 'w') {
> +			*((unsigned long *) virt_addr) = writeval;
> +			read_result = *((unsigned long *) virt_addr);
> +		}
> +		printf("Written 0x%X; readback 0x%X\n", writeval, read_result);
> +	}
> +
> +	if (munmap(map_base, DEVMEM_MAP_SIZE) == -1)
> +		bb_perror_msg_and_die("munmap");
> +	close(fd);
> +	fflush_stdout_and_exit(EXIT_SUCCESS);

Why do you cleanup here but not on the error paths?

Bastian

-- 
	"That unit is a woman."
	"A mass of conflicting impulses."
		-- Spock and Nomad, "The Changeling", stardate 3541.9