On Sunday 26 October 2008 14:26, Bastian Blank wrote:
> 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?
svn version allows bhwl specifiers for byte/short/int/long sized accesses,
and allows bit width form too.
>
> > +config DEVMEM
> > + bool "devmem"
> > + default y
>
> Why default y? This is no generic tool.
Right, it was fixed.
> > +#define DEVMEM_MAP_SIZE 4096UL
>
> I think this needs to match the pagesize, which is not fixed to 4096.
Do you mean "use getpagesize() instead"?
I will fix it now, thanks!
> > +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.
Yes, it is not.
> > + 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?
Overflow of what?
> > + 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.
It was fixed.
> > + 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.
Was fixed too. 'w' is int now, 'l' is long.
> > + else {
> > + fprintf(stderr, "Illegal data type '%c'\n", access_type);
> > + exit(EXIT_FAILURE);
>
> This belongs to the argument check.
Was fixed
> > + if (argc > 3) {
> > + writeval = bb_strtoul(argv[3], 0, 0);
>
> Error checks?
Was fixed
> > + 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?
Was fixed to not clean up in any case.
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
