Subject: Re: Busspace sanity ...
To: Jason Thorpe <thorpej@nas.nasa.gov>
From: Stefan Grefen <grefen@hprc.tandem.com>
List: port-i386
Date: 06/09/1998 21:32:44
In message <199806091749.KAA08315@lestat.nas.nasa.gov>  Jason Thorpe wrote:
> On Tue, 09 Jun 1998 16:27:08 +0200 
>  Stefan Grefen <grefen@hprc.tandem.com> wrote:
> 
>  > My serial console showed me that there is a new check in 
>  > bus_space_write_XXX to catch unaligned busspace accesses.
>  > If this happens in the ethernet driver of a diskless machine, this 
>  > gets your attention :-))
> 
> That was the point :-)

I understand that :-))

> 
>  > I think it is a little to aggressive.
>  > Forcing alignment to access-size may help getting more drivers real MI it
>  > is annoying for people that check already for propper alignment for
>  > the actual architecture.
> 
> I don't think so, because those checks are enabled w/ DEBUG, which is
> "more expensive kernel consistency checks, and possibly noisy output".
> 
> The truth of the matter is that drivers that do unaligned access are
> broken, even on platforms which do not require strict alignment.  Those
> drivers should be fixed.

I consider a driver fixed if it checks for proper alignment for a given
architecture and acts on misalignment.

> 
>  > Eg. a check with ALIGNED_POINTER(p,unit) should be sufficient to avoid
>  > the reminder from bus_space_XXXX.
> 
> Exactly... the reminder from bus_space_*() is designed to point out
> drivers which DO NOT do alignment fixups!  Once the drivers are fixed,
> the messages go away.

My point is that even if ALIGNED_POINTER() is true (always on a i386),
__BUS_SPACE_ALIGNED_ADDRESS() maybe false. 
The code in question does a ALIGNED_POINTER() and a pullup if that fails.
I consider that not broken.

> 
>  > There could be an option like BUSSPACE_DEBUG_ALIGNMENT with 
>  > stricter rules for MI-testing, but for the normal kernel (with DEBUG)
>  > the __BUS_SPACE_ALIGNED_ADDRESS macro should be equal to ALIGNED_POINTER.
> 
> No...  that is not what the checks were designed to do.  The checks are
> specifically designed to enfore _MI_ alignment requirements, as documented
> in the bus_space(9) manual page.

I can't find anything there that mandates that data-pointers have to be 
aligned differently than in 'normal' code. 

I think the __BUS_SPACE_ADDRESS_SANITY checks for the data pointer should
use ALIGNED_POINTER (but than the CPU will find those anyway :-)) ).

An option for stricter checking would allow to test if a driver would
run on an architecture with different alignment restrictions (and I 
think that is not covered by DEBUG).

I would hate to have to copy the mbuf data on a slow i386SX running
diskless, because some fast RISC ships can't do unaligned access :-)))

> 
> Can you collect the messages and submit a bug report against the device
> driver in question?  Then it will get fixed, and you won't have to see
> the messages anymore :-)

It is a driver for the CS8900 lan driver, I ported from the SHARK tree to
i386. It's work in progress because it has to be tested again in the
SHARK tree (I'm waiting for hardware). 
I uploaded the current version sometime ago to ftp.netbsd.org incoming.

Stefan

PS.
Code sniplet:
                if(!ALIGNED_POINTER(p,u_int16_t)) {
                    /*
                     * THIS IS UGLY
                     */
                     m0=m_pullup(m,len);
                     continue;
                }
		...
		 bus_space_write_multi_2(sc->sc_iot,sc->sc_ioh,
		     PORT_RXTX_DATA,p,(len>>1) );

Console messages:
buffer 0xf025b495 not aligned to 2 bytes ../../../../dev/ic/cs8900.c:3067
....



> 
> Jason R. Thorpe                                       thorpej@nas.nasa.gov
> NASA Ames Research Center                            Home: +1 408 866 1912
> NAS: M/S 258-5                                       Work: +1 650 604 0935
> Moffett Field, CA 94035                             Pager: +1 650 428 6939

--
Stefan Grefen                                Tandem Computers Europe Inc.
grefen@hprc.tandem.com                       High Performance Research Center
 --- Hacking's just another word for nothing left to kludge. ---