Re: [PATCH 4/6] ehea: header files

[Michael Ellerman <michael@ellerman.id.au>]

On Mon, 2006-08-14 at 14:53 +0200, Jan-Bernd Themann wrote:
> Michael Ellerman wrote:
> >> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea.h	1969-12-31 16:00:00.000000000 -0800
> >> +++ kernel/drivers/net/ehea/ehea.h	2006-08-08 23:59:39.927452928 -0700
> >> +
> >> +#define EHEA_PAGESHIFT  12
> >> +#define EHEA_PAGESIZE   4096UL
> >> +#define EHEA_CACHE_LINE 128
> > 
> > This looks like a very bad idea, what happens if you're running on a
> > machine with 64K pages?
> > 
> 
> The EHEA_PAGESIZE define is needed for queue management to hardware side.

You mean the eHEA has its own concept of page size? Separate from the
page size used by the MMU?

> >> +/*
> >> + *  h_galpa:
> >> + *  for pSeries this is a 64bit memory address where
> >> + *  I/O memory is mapped into CPU address space
> >> + */
> >> +
> >> +struct h_galpa {
> >> +	u64 fw_handle;
> >> +};
> > 
> > What is a h_galpa? And why does it need a struct if it's just a u64?
> > 
> 
> The eHEA chip is not PCI attached but directly connected to a proprietary
> bus. Currently, we can access it by a simple 64 bit address, but this is not
> true in all cases. Having a struct here allows us to encapsulate the chip
> register access and to respond to changes to system hardware.
> 
> We'll change the name to h_epa meaning "ehea physical address"

Hmm, I'm not convinced. Having the struct doesn't really encapsulate
much, because most of the places where you use the h_galpa struct just
pull out the fw_handle anyway. So if you change the layout of the struct
you're going to have to change most of the code anyway. And in the
meantime it makes the code a lot less readable, most people understand
what "u64 addr" is about, whereas "struct h_galpa" is much less
meaningful. </2c>

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

Attachment: signature.asc
Description: This is a digitally signed message part