lkml.org 
[lkml]   [2005]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 01/13] [RFC] ipath basic headers
    From
    Date
    > > +#define yield() sched_yield()
    >
    > Some might get upset about what I assume is userspace test harness code or
    > what _is_ this doing?) in a driver. But if the maintainers find it useful
    > we can live with it,

    That is cosimulator code. It's easy enough to remove. I'll look into
    it.

    > > +#ifndef _BITS_PER_BYTE
    > > +#define _BITS_PER_BYTE 8
    > > +#endif
    >
    > I'd be inclined to stick BITS_PER_BYTE into include/linux/types.h.

    Really? I was just going to suggest removing it, but if sticking it in
    types.h works for you, then fine.

    > > +static __inline__ void ipath_shortcopy(void *dest, void *src, uint32_t cnt)
    > > + __attribute__ ((always_inline));
    >
    > s/__inline__/inline/ throughout.

    OK.

    > > +#define round_up(v,sz) (((v) + (sz)-1) & ~((sz)-1))
    >
    > We have ALIGN()

    Yup.

    > > +struct ipath_int_vec {
    > > + int long long addr;
    >
    > long long

    OK.

    > > +#define IPATH_USERINIT _IOW('s', 16, struct ipath_user_info)
    > > +/* init; kernel/chip params to user */
    > > +#define IPATH_BASEINFO _IOR('s', 17, struct ipath_base_info)
    > > +/* send a packet */
    > > +#define IPATH_SENDPKT _IOW('s', 18, struct ipath_sendpkt)
    >
    > uh-oh. ioctls. Do we have compat conversions for them all, if needed?

    For those that are needed, I believe we covered them all. Some have
    suggested removing ioctls. I'm willing to look into alternatives, but
    if you think they're OK, I'd rather leave them.

    > > +/*
    > > + * A segment is a linear region of low physical memory.
    > > + * XXX Maybe we should use phys addr here and kmap()/kunmap()
    > > + * Used by the verbs layer.
    > > + */
    > > +struct ipath_seg {
    > > + void *vaddr;
    > > + u64 length;
    > > +};
    >
    > Suggest `long' for the length. We don't need 64 bits on 32-bit machines.

    OK.

    > > +struct ipath_mregion {
    > > + u64 user_base; /* User's address for this region */
    >
    > void *.
    >
    > > + u64 iova; /* IB start address of this region */
    >
    > Maybe here too.

    OK.

    > > +int ipath_mlock(unsigned long, size_t, struct page **);
    >
    > Sometimes it does `int foo()' and sometimes `extern int foo()'. I tend to
    > think the `extern' is a waste of space.

    Yup.

    > > +#define ipath_func_krecord(a)
    > > +#define ipath_func_urecord(a, b)
    > > +#define ipath_func_mrecord(a, b)
    > > +#define ipath_func_rkrecord(a)
    > > +#define ipath_func_rurecord(a, b)
    > > +#define ipath_func_rmrecord(a, b)
    > > +#define ipath_func_rsrecord(a)
    > > +#define ipath_func_rcrecord(a)
    >
    > What are all these doing? Might need do{}while(0) for safety.

    I'll look at cleaning them out. Probably left-overs from some earlier
    experiment.

    > > +#ifdef IPATH_COSIM
    > > +extern __u32 sim_readl(const volatile void __iomem * addr);
    > > +extern __u64 sim_readq(const volatile void __iomem * addr);
    >
    > The driver has a strange mixture of int32_t, s32 and __s32. s32 is
    > preferred.

    Yea - I'll clean that up.

    > > + */
    > > +static __inline__ uint32_t ipath_kget_ureg32(const ipath_type stype,
    > > + ipath_ureg regno, int port)
    > > +{
    > > + uint64_t *ubase;
    > > +
    > > + ubase = (uint64_t *) (devdata[stype].ipath_uregbase
    > > + + (char *)devdata[stype].ipath_kregbase
    > > + + devdata[stype].ipath_palign * port);
    > > + return ubase ? ipath_readl(ubase + regno) : 0;
    > > +}
    >
    > Are all these u64's needed on 32-bit?

    Don't know - I'll ask around. We don't support the hardware in 32-bit
    anyway, so...

    > > +static __inline__ uint64_t ipath_kget_kreg64(const ipath_type stype,
    > > + ipath_kreg regno)
    > > +{
    > > + if (!devdata[stype].ipath_kregbase)
    > > + return ~0ULL;
    >
    > We don't know that the architecture implements u64 as unsigned long long.
    > Some use unsigned long. Best way of implmenting the all-ones pattern is
    > just `-1'.

    OK.

    > Gee. Big driver.

    Tell me about it :-) Basically, we're doing infiniband in software: no
    offload.

    Regards,
    Robert.

    --
    Robert Walsh Email: rjwalsh@pathscale.com
    PathScale, Inc. Phone: +1 650 934 8117
    2071 Stierlin Court, Suite 200 Fax: +1 650 428 1969
    Mountain View, CA 94043.


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-12-17 23:42    [W:7.328 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site