[Cbe-oss-dev] [patch 02/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Arnd Bergmann]

On Saturday 08 March 2008, Murali Iyer wrote:
> Subject: [patch 02/16] axon driver header files
> 
> Description:
> 	This patch provides include files for the purposes of
> 	kernel and userland interfaces into the axon driver.

It's better to format the changelog without the word 'Description'
or the indenting, so that it will end up the same way as all
other patches in the git changelog.

> Signed-off-by: H Brett Bolen (hbbolen at us.ibm.com)
> Signed-off-by: Murali Iyer   (mniyer at us.ibm.com)
> Signed-off-by: Tim Schimke   (tschimke at us.ibm.com)
> Signed-off-by: Jesse Arroyo  (arroyoj at us.ibm.com)

Email addresses are formatted as 'H Brett Bolen <hbbolen at us.ibm.com>'
according to the usual RFCs, please do that in order to not confuse
tools parsing the headers.

Note that patches from multiple authors are rather uncommon,
so the system does not deal with it well. Since the mail was
sent from Murali's address and there is no 'From:' line in the
description, it will be parsed as having originated from Murali,
sent to Brett, back to Murali, from there to Tim and finally
to Jesse, which is probably not what you want.

The Signed-off-by: from the person sending the mail should always
come last, and you don't need to have S-o-b from each of the authors,
but it is usually a nice guesture to list all co-authors in the
changelog.

> +/*
> + * Following message types are used by the notify functions.
> + * Values 0 to 7 will generate "wake-up" interrupts to the remote system.
> + * Values 8 to 15 will generate call-backs on the local system.
> + */
> +enum message_type	{
> +	D2D_MSG_TYPE	     = 0, /* base driver to driver notification */
> +	APNET_MSG_TYPE	     = 1, /* apnet ethernet notification */
> +	DACS_MSG_TYPE	     = 2, /* DACS interface notification */
> +	DMA_LOCAL_MSG_TYPE   = 3, /* notification to system that initiated */
> +				  /*  a DMA */
> +	DMA2_LOCAL_MSG_TYPE  = 4, /* notification to system that initiated */
> +				  /*  a DMA */
> +	DMA_REMOTE_MSG_TYPE  = 5, /* notification to system that didn't */
> +				  /*  initiate the DMA */
> +	DMA2_REMOTE_MSG_TYPE = 6  /* notification to system that didn't */
> +				  /* initiate the DMA */
> +	/* type 7 reserved for other remote notifications */
> +	/* types 8 - 15 reserved for local callbacks */
> +};

It may be good to mark the parts of the header that are defining the
interface between the two sides, so you will know if you break the
communication or can avoid doing it.

Since the enum is in the global namespace, please qualify it with an
appropriate prefix, e.g. axon_message_type.

> +/**
> + * axon_get_num_devices  - called by apnet
> + *
> + * Returns 0 if no device(s) found, positive number for number of devices found
> + * (typically 1-4) or -1 for error
> + */
> +int axon_get_num_devices( void );

The kerneldoc style comments typically go into the source file, not
the header.

Please run your patches through scripts/checkpatch.pl to tell you about
coding style. Here, your spaces are misplaced. I won't comment about
things that checkpatch.pl should find otherwise.

> +/**
> + * axon_get_remote_memory  - called by apnet
> + * @dev_num - Device number ( typically 0 - 3 )
> + *
> + * Returns  NULL if remote system is not ready or driver not loaded on remote
> + * or ioremapped memory address of the remote system
> + */
> +void __iomem *axon_get_remote_memory(int dev_num);

Enumerating by dev_num is a poor interface choice. In the kernel, we usually
pass around reference counted pointers instead. With the pointer, you can
always have some shared members (e.g. the struct device) that you pass
to other kernel interfaces.

> +/**
> + * axon_get_local_memory - called by apnet

The comment should better say what the interface is doing, not who calls
it. For finding out the caller, we have cscope etc., but it's not
obvious what the function does.

> +/**
> + * axon_register_notify_callback, axon_unregister_notify_callback,
> + * axon_enable_notify_callback, axon_disable_notify_callback and
> + * axon_send_remote_notification  - for apnet interface
> + *
> + * @message_type - Type of the unique message number, see enum message_type
> + * @dev_num - Device number ( typically 0 - 3 )
> + * @p_data  - Private data pointer, passed with call back
> + * @cb_func - call back function
> + *
> + * Returns  0 for success or -ve value for error
> + *
> + * Registered function will be called back with message_type and
> + * pointer to private data provided during registration.
> + */
> +
> +/* callback prototype */
> +typedef	  int  (*axon_callback)( int   type,
> +				 void *callback_data);
> +
> +int  axon_register_notify_callback(  enum message_type msg_type, int dev_num,
> +				     void *p_data,
> +				     axon_callback p_func);

The dev_num and p_data arguments should probably be the same pointer,
and they should be the first argument. Object oriented programming
in C requires some amount of extra rules, and one of those we tend to
use in the kernel is to always pass the object as the first argument
of a method.

If you need to pass an additional data argument other than the device
itself, I would put that last, like

int axon_register_callback(struct axon_device *dev,
	enum axon_message_type type, axon_callback *func, void *data);


Instead of 'notify_callback', you can probably just write 'callback',
which will carry the same information, just shorter.

> +void axon_unregister_notify_callback( enum message_type msg_type, int dev_num);
> +
> +void axon_enable_notify_callback( enum message_type msg_type, int dev_num);
> +void axon_disable_notify_callback( enum message_type msg_type, int dev_num);

same here.

> +void axon_send_remote_notification(   enum message_type msg_type, int dev_num);
> +
> +
> +enum {
> +	AXON_APNET_DMATYPE_PUT	= 0x00000001, /* dma local to remote */
> +	AXON_APNET_DMATYPE_GET	= 0x00000002, /* dma remote to local */
> +	AXON_APNET_DMA_CALLBACK = 0x04000000  /* request a callback
> +					       * when DMA is complete
> +					       */
> +};
> +
> +
> +/* helper mapping functions for apnet */
> +int axon_dma_map_single(int dev_num, void *addr,
> +			size_t size, int dir, dma_addr_t *handle);
> +
> +void axon_dma_unmap_single(int dev_num, dma_addr_t handle,
> +			   size_t size, int dir);

I think these should not be needed if you supply the right
dma-API implementation for device behind the PLB5.

> +struct axon_apnet_dma_cb {
> +	dma_addr_t local_buf;	/* DMA mapped address for the local memory */
> +	u64 remote_offset;	/* Physical address of the remote memory for data */
> +	int length;		/* Length in bytes of transfer */
> +	int flags;
> +	/* DMA mapped address for the local status value */
> +	dma_addr_t local_status;
> +	int remote_status_offset; /* location to write status value */
> +	axon_callback cb_func;	/* temporary plumbing for callback */
> +	void *cb_data;		/* pointer to return on callback */
> +};

The apnet stuff has no place in the common header, it should
probably just be moved into the implementation file. If the
common code needs to know about apnet, you're probably doing
something wrong with the layering of the drivers.

> +/**
> + * axon_apnet_dma - Perform a DMA within the apnet memory area
> + * @dev_num: Device number ( typically 0 - 3 )
> + * @cb: apnet dma request control block
> + */
> +int axon_apnet_dma(int dev_num, struct axon_apnet_dma_cb *cb);

same here.

> +#endif /*  __AXON_IF_H__ */
> Index: linux-2.6.23.1/include/linux/triblade/axon_ioctl.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.23.1/include/linux/triblade/axon_ioctl.h
> @@ -0,0 +1,149 @@

Please don't mix high-level driver stuff with the common plumbing
layer. The ioctl definitions need to go into the patch that
provides the user interface, and it would be good to keep that
patch separate from the rest of the series.

I remember that your earlier patch versions had the user interface
in the main driver, which we should avoid at any cost.
My feeling is that there will have to be incompatible changes
to the user interface, so it's better to have that driver as
a separate module, so you can either load the mainline version,
or the backwards-compatible version that customers start using
before the driver gets merged.

> +/*
> + * mmap offsets
> + */
> +#define LOCAL_SMA_OFFSET   0x0
> +#define REMOTE_SMA_OFFSET  0x0100000
> +#define DMA_COMMAND_BUFFER_OFFSET  0x0800000
> +

Fixed offsets seem a little scary, it might be better to have
separate file handles where each one has a different area
of memory to get mapped.

> +/*
> + * opaque handles
> + */
> +typedef	 __u64 AXON_memory_region_handle;

For an opaque handle, we usually use file descriptors, backed
with anon_inodes.

In your identifiers, don't use capital letters.

	Arnd <><