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
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
