Re: Obvious bug in /sys/i386/include/bus.h (was: bus_at386.h)

From: Hans Petter Selasky (hselasky_at_c2i.net)
Date: 06/14/05

  • Next message: Hans Petter Selasky: "Re: Obvious bug in /sys/i386/include/bus.h" 
    To: freebsd-hackers@freebsd.org
Date: Tue, 14 Jun 2005 02:18:10 +0200

    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
    _______________________________________________
    freebsd-hackers@freebsd.org mailing list
    http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
    To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"

  • Next message: Hans Petter Selasky: "Re: Obvious bug in /sys/i386/include/bus.h"

    Relevant Pages