[patch] inteltool support for FreeBSD

[Idwer Vollering]

Add support for FreeBSD.

Signed-off-by: Idwer Vollering

---

I took the liberty to copy MSR code from flashrom's hwaccess.h.
Patch was written using FreeBSD 8.1, earlier versions might or might not
work.

[Warren Turkal]

On Sunday, October 17, 2010 09:37:31 am Idwer Vollering wrote:
> Add support for FreeBSD.
>
> Signed-off-by: Idwer Vollering

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?
* 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.

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?

The Makefile change looks ok.

Thanks,
wt

[Idwer Vollering]

2010/10/18 Warren Turkal
> On Sunday, October 17, 2010 09:37:31 am Idwer Vollering wrote:
> > Add support for FreeBSD.
> >
> > Signed-off-by: Idwer Vollering

Updated patch attached.
Signed-off-by: Idwer Vollering
> 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

[Stefan Reinauer]

On 18.10.2010, at 07:26, Idwer Vollering  wrote:
> 2010/10/18 Warren Turkal 
> On Sunday, October 17, 2010 09:37:31 am Idwer Vollering wrote:
> > Add support for FreeBSD.
> >
> > Signed-off-by: Idwer Vollering
>
> Updated patch attached.
> Signed-off-by: Idwer Vollering

Acked-by Stefan Reinauer
> 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: [e-mail]
> http://www.coreboot.org/mailman/listinfo/coreboot

[Uwe Hermann]

On Mon, Oct 18, 2010 at 07:09:03PM -0700, Stefan Reinauer wrote:
> > Updated patch attached.
> > Signed-off-by: Idwer Vollering
>
> Acked-by Stefan Reinauer

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.

[Warren Turkal]

On Mon, Oct 18, 2010 at 7:26 AM, Idwer Vollering  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

[Carl-Daniel Hailfinger]

On 20.10.2010 10:07, Warren Turkal wrote:
> On Mon, Oct 18, 2010 at 7:26 AM, Idwer Vollering  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