Messages in this thread | | | Subject | Re: [PATCH 01/13] [RFC] ipath basic headers | From | Robert Walsh <> | Date | Sat, 17 Dec 2005 14:39:18 -0800 |
| |
> > +#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/
| |