| Submitter | Idwer Vollering |
|---|---|
| Date | 2010-10-18 14:26:54 |
| Message ID | <AANLkTi=L4ArznpT7kvyu8GVi7oa9iZb4AKvY-crpknMc@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/2133/ |
| State | Accepted |
| Headers | show |
Comments
On 18.10.2010, at 07:26, Idwer Vollering <vidwer@gmail.com> wrote: > 2010/10/18 Warren Turkal <wt@penguintechs.org> > On Sunday, October 17, 2010 09:37:31 am Idwer Vollering wrote: > > Add support for FreeBSD. > > > > Signed-off-by: Idwer Vollering <vidwer@gmail.com> > > Updated patch attached. > Signed-off-by: Idwer Vollering <vidwer@gmail.com> Acked-by Stefan Reinauer <stepan@coreboot.org> > > > In inteltool.h: > * Can you please briefly explain the need for the macros for {IN,OUT}{B,W,L} > when I don't seen them called from anywhere in the code? > > Dropped, thanks. > > * If you mean to use them, why are they implemented as macros instead of > functions. I think it'd be easier to read if they were function, and gcc could > possibly even inline such a simple function. > > See above. > > > In inteltool.c: > * Why not just include unistd.h on all platforms? > * I think the #ifdef __FREEBSD__ just makes the code difficult to read. I think > the platform specific code need to be factored out somehow. > > * The io_fd variable doesn't appear to be used anywhere after opening the > /dev/io file. Doesn't it need to be closed somewhere? If not, why even bother > creating a variable to hold the value of the open instead of just testing it > directly? > > Is this a possible memory leak ? If so, it needs to be fixed in flashrom too. > > > The Makefile change looks ok. > > Thanks, > wt > > <inteltool_freebsd.v2.patch> > -- > coreboot mailing list: coreboot@coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot
On Mon, Oct 18, 2010 at 7:26 AM, Idwer Vollering <vidwer@gmail.com> wrote: > Is this a possible memory leak ? If so, it needs to be fixed in flashrom > too. It would only be a memory leak if used in a long running process that doesn't use the file over and over. As it stands, the OS will reclaim the memory used to create the file descriptor when the process ends, and I am not sure there's much practical point in closing the file. However academically, the file probably should be closed. Thanks, wt
On 20.10.2010 10:07, Warren Turkal wrote: > On Mon, Oct 18, 2010 at 7:26 AM, Idwer Vollering <vidwer@gmail.com> wrote: > >> Is this a possible memory leak ? If so, it needs to be fixed in flashrom >> too. >> > > It would only be a memory leak if used in a long running process that > doesn't use the file over and over. As it stands, the OS will reclaim > the memory used to create the file descriptor when the process ends, > and I am not sure there's much practical point in closing the file. > However academically, the file probably should be closed. > AFAIK flashrom does close all file descriptors, even those used for device I/O on some BSDs. Regards, Carl-Daniel
On Mon, Oct 18, 2010 at 07:09:03PM -0700, Stefan Reinauer wrote: > > Updated patch attached. > > Signed-off-by: Idwer Vollering <vidwer@gmail.com> > > Acked-by Stefan Reinauer <stepan@coreboot.org> r5981. I dropped all those "copied from flashrom" comments all over the place, it's not really useful info and just clutters the code. Uwe.
Patch
Index: inteltool.h =================================================================== --- inteltool.h (revision 5965) +++ inteltool.h (working copy) @@ -3,6 +3,9 @@ * * Copyright (C) 2008-2010 by coresystems GmbH * + * MSR code for FreeBSD is copied from flashrom's hwaccess: + * Copyright (C) 2009 Carl-Daniel Hailfinger + * * 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; version 2 of the License. @@ -29,6 +32,12 @@ #endif #include <pci/pci.h> +/* This #include is needed for freebsd_{rd,wr}msr */ +/* Copied from flashrom. */ +#if defined(__FreeBSD__) +#include <machine/cpufunc.h> +#endif + #define INTELTOOL_VERSION "1.0" /* Tested chipsets: */ @@ -88,9 +97,21 @@ #define ARRAY_SIZE(a) ((int)(sizeof(a) / sizeof((a)[0]))) -#ifndef __DARWIN__ +/* Copied from flashrom. */ +#if !defined(__DARWIN__) && !defined(__FreeBSD__) typedef struct { uint32_t hi, lo; } msr_t; #endif +/* Copied from flashrom. */ +#if defined (__FreeBSD__) +/* FreeBSD already has conflicting definitions for wrmsr/rdmsr. */ +#undef rdmsr +#undef wrmsr +#define rdmsr freebsd_rdmsr +#define wrmsr freebsd_wrmsr +typedef struct { uint32_t hi, lo; } msr_t; +msr_t freebsd_rdmsr(int addr); +int freebsd_wrmsr(int addr, msr_t msr); +#endif /* End of copied code. */ typedef struct { uint16_t addr; int size; char *name; } io_register_t; void *map_physical(unsigned long phys_addr, size_t len); @@ -105,4 +126,3 @@ int print_epbar(struct pci_dev *nb); int print_dmibar(struct pci_dev *nb); int print_pciexbar(struct pci_dev *nb); - Index: inteltool.c =================================================================== --- inteltool.c (revision 5965) +++ inteltool.c (working copy) @@ -3,6 +3,7 @@ * * Copyright (C) 2008-2010 by coresystems GmbH * written by Stefan Reinauer <stepan@coresystems.de> + * Copyright (C) 2009 Carl-Daniel Hailfinger * * 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 @@ -24,6 +25,10 @@ #include <fcntl.h> #include <sys/mman.h> #include "inteltool.h" +/* Copied from flashrom. */ +#if defined(__FreeBSD__) +#include <unistd.h> +#endif static const struct { uint16_t vendor_id, device_id; @@ -213,7 +218,19 @@ } } +/* Copied from flashrom. */ +#if defined(__FreeBSD__) + int io_fd; +#endif + +#if defined(__FreeBSD__) + if ((io_fd = open("/dev/io", O_RDWR)) < 0) { + perror("/dev/io"); +#else if (iopl(3)) { +/* End of copied code. */ + perror("iopl"); +#endif /* End of the if/else control flow copied from flashrom. */ printf("You need to be root.\n"); exit(1); } Index: Makefile =================================================================== --- Makefile (revision 5965) +++ Makefile (working copy) @@ -33,6 +33,11 @@ ifeq ($(OS_ARCH), Darwin) LDFLAGS = -framework DirectIO -lpci -lz endif +ifeq ($(OS_ARCH), FreeBSD) +CFLAGS += -I/usr/local/include +LDFLAGS += -L/usr/local/lib +LIBS = -lz +endif all: pciutils dep $(PROGRAM)