Re: [PATCH 01/13] [RFC] ipath basic headers

Christoph Hellwig
2005-12-17T13:15:00Z

> + * $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?