Date: Tue, 14 Jun 2005 02:18:10 +0200 From: Hans Petter Selasky <hselasky@c2i.net> To: freebsd-hackers@freebsd.org Cc: Joerg Sonnenberger <joerg@britannica.bec.de> Subject: Re: Obvious bug in /sys/i386/include/bus.h (was: bus_at386.h) Message-ID: <200506140218.11844.hselasky@c2i.net> In-Reply-To: <20050613162743.GA769@britannica.bec.de> References: <200506131412.38967.hselasky@c2i.net> <200506131758.25671.hselasky@c2i.net> <20050613162743.GA769@britannica.bec.de>
index | next in thread | previous in thread | raw e-mail
On Monday 13 June 2005 18:27, Joerg Sonnenberger wrote:
> On Mon, Jun 13, 2005 at 05:58:24PM +0200, Hans Petter Selasky wrote:
> > static void
> > filter(struct fifo *f)
> > {
> > do {
> > if(!f->len)
> > {
> > if(f->m) ...;
> >
> > f->m = get_mbuf();
> > if(!f->m) break;
> >
> > f->len = m->m_len;
> > f->ptr = m->m_data;
> > }
> >
> > /* f->Z_chip is the maximum transfer length */
> >
> > io_len = min(f->len, f->Z_chip);
>
> if (io_len == 0)
> continue;
>
> > bus_space_write_multi_1(t,h,xxx,f->ptr,io_len);
> >
> > f->len -= io_len;
> > f->Z_chip -= io_len;
> > f->ptr += io_len;
> >
> > } while(Z_chip);
> > }
>
> [...]
>
> > Adding that extra check for zero transfer length is not going to affect
> > performance at all. If one does that using "C", the compiler can optimize
> > away that "if(count) ..." when "count" is a constant. Besides the i386
> > machine instructions "ins" and "outs" already exhibit that behaviour.
>
> The compiler can only optimize it away if it is known to be a constant.
> But thinking again about it, it should be documented at least whether
> a count of 0 is allowed or not. I think it makes more sense to not allow
> it, but others (you included) might disagree.
Why?
If a count of zero is not allowed, then "bus_space_xxx()" should panic on
count equal to zero and not freeze the system, so that the user is able to
find out what is wrong:
#if defined(XXX)
if(count == 0) panic("not allowed");
#endif
But that is just stupid, why not just return:
#if defined(XXX)
if(count == 0) return;
#endif
And then I can put:
#define XXX
in my code before including "bus.h". Instead of creating wrappers, just to be
sure:
#define bus_space_read_multi_1(t,h,off,ptr,count) \
{ if(count) bus_space_read_multi_1(t,h,off,ptr,count); }
Because this is really specific to i386 and amd64. If you look at "alpha",
"sparc64" and "ia64" they all use "while(count--) ...". So I think the one
that wrote that code did a mistake.
My theory is that he or her copied:
static __inline void
insb(u_int port, void *addr, size_t cnt)
{
__asm __volatile("cld; rep; insb"
: "+D" (addr), "+c" (cnt)
: "d" (port)
: "memory");
}
And forgot to add that extra check for count equal to zero, when converting
"rep" into "loop"!
--HPS
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200506140218.11844.hselasky>