Messages in this thread |  | | | Date | Sat, 17 Dec 2005 13:14:56 +0000 | | From | Christoph Hellwig <> | | Subject | Re: [PATCH 01/13] [RFC] ipath basic headers |
| |
> + * $Id: ipath_common.h 4491 2005-12-15 22:20:31Z rjwalsh $
please remove RCSIDs everywhere.
> +#ifdef __KERNEL__ > +#include <linux/ioctl.h> > +#include <linux/uio.h> > +#include <asm/atomic.h> > +#else /* !__KERNEL__; user mode */ > +#include <sys/ioctl.h> > +#include <sys/uio.h> > +#include <sys/types.h> > +#include <stdint.h> > + > +/* these aren't implemented for user mode, which is OK until we multi-thread */ > +typedef struct _atomic { > + uint32_t counter; > +} atomic_t; /* no atomic_t type in user-land */ > +#define atomic_set(a,v) ((a)->counter = (v)) > +#define atomic_inc_return(a) (++(a)->counter) > +#define likely(x) (x) > +#define unlikely(x) (x) > + > +#define yield() sched_yield()
Please push this out. It's fine if they reuse kernel-code in userspace this way, but please move the compat wrappers to a separate file that's not in the kernel tree.
> +typedef uint8_t ipath_type;
totally meaningless typedef
> +#ifndef _BITS_PER_BYTE > +#define _BITS_PER_BYTE 8 > +#endif
WTF?
> + > +static __inline__ void ipath_shortcopy(void *dest, void *src, uint32_t cnt) > + __attribute__ ((always_inline));
> +/* > + * this is used for very short copies, usually 1 - 8 bytes, > + * *NEVER* to the PIO buffers!!!!!!! use ipath_dwordcpy for longer > + * copies, or any copy to the PIO buffers. Works for 32 and 64 bit > + * gcc and pathcc > + */ > +static __inline__ void ipath_shortcopy(void *dest, void *src, uint32_t cnt)
in kernel land we __inline__ includes always_inline. Also no need for a separate prototype for a just following inline function.
> +{ > + void *ssv, *dsv; > + uint32_t csv; > + __asm__ __volatile__("cld\n\trep\n\tmovsb":"=&c"(csv), "=&D"(dsv), > + "=&S"(ssv) > + :"0"(cnt), "1"(dest), "2"(src) > + :"memory"); > +}
No way we're gonna put assembler code into such a driver.
> +struct ipath_int_vec { > + int long long addr; > + uint32_t info; > +};
please always used fixes-size types for user communication. also please avoid ioctls like the rest of the IB codebase.
> +/* Similarly, this is the kernel version going back to the user. It's slightly > + * different, in that we want to tell if the driver was built as part of a > + * PathScale release, or from the driver from the OpenIB, kernel.org, or a > + * standard distribution, for support reasons. The high bit is 0 for > + * non-PathScale, and 1 for PathScale-built/supplied. That bit is defined > + * in Makefiles, rather than this file. > + * > + * It's returned by the driver to the user code during initialization > + * in the spi_sw_version field of ipath_base_info, so the user code can > + * in turn check for compatibility with the kernel. > +*/ > +#define IPATH_KERN_SWVERSION ((IPATH_KERN_TYPE<<31) | IPATH_USER_SWVERSION)
NACK, there's no way we're gonna put in a way to identify an "official" version. The official version is the last one in mainline always.
> +#ifndef PCI_VENDOR_ID_PATHSCALE /* not in pci.ids yet */ > +#define PCI_VENDOR_ID_PATHSCALE 0x1fc1 > +#define PCI_DEVICE_ID_PATHSCALE_INFINIPATH1 0xa > +#define PCI_DEVICE_ID_PATHSCALE_INFINIPATH2 0xd > +#endif
so move it there?
> +typedef struct _ipath_portdata {
please avoid typedefs for struct types.
> +/* > + * these should be somewhat dynamic someday, although they are fixed > + * for all users of the device on any given load. > + * > + * NOTE: There is a VM bug in the 2.4 Kernels similar to the one Dave > + * fixed in the 2.6 Kernel. When using large or discontinuous memory, > + * we get random kernel oops. So, in 2.4, we are just going to stick > + * with 4k chunks instead of 64k chunks. > + */
No one cares about 2.4 kernels here.
> + * these function similarly to the mlock/munlock system calls. > + * ipath_mlock() is used to pin an address range (if not already pinned), > + * and optionally return the list of physical addresses > + * ipath_munlock() does the obvious, and ipath_mlock() cleans up all > + * private memory, used at driver unload. > + * ipath_mlock_nocopy() is similar to mlock, but only one page, and marks > + * the vm so the page isn't taken away on a fork. > + */ > +int ipath_mlock(unsigned long, size_t, struct page **); > +int ipath_mlock_nocopy(unsigned long, struct page **);
this kind of thing definitly doesn't belong into an LLDD. or maybe it's just stale prototypes?
> +#ifdef IPATH_COSIM > +extern __u32 sim_readl(const volatile void __iomem * addr); > +extern __u64 sim_readq(const volatile void __iomem * addr); > +extern void sim_writel(__u32 val, volatile void __iomem * addr); > +extern void sim_writeq(__u64 val, volatile void __iomem * addr); > +#define ipath_readl(addr) sim_readl(addr) > +#define ipath_readq(addr) sim_readq(addr) > +#define ipath_writel(val, addr) sim_writel(val, addr) > +#define ipath_writeq(val, addr) sim_writeq(val, addr) > +#else > +#define ipath_readl(addr) readl(addr) > +#define ipath_readq(addr) readq(addr) > +#define ipath_writel(val, addr) writel(val, addr) > +#define ipath_writeq(val, addr) writeq(val, addr) > +#endif
Please use the proper functions directly. Your simulator can override them if nessecary.
> +static __inline__ uint32_t ipath_kget_kreg32(const ipath_type stype, > + ipath_kreg regno) > +{ > + volatile uint32_t *kreg32; > + > + if (!devdata[stype].ipath_kregbase) > + return ~0; > + > + kreg32 = (volatile uint32_t *)&devdata[stype].ipath_kregbase[regno];
volatile use is probably always wrong. but this whole functions looks like a very odd wrapper anyway?
- 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/
|  |