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

[Arnd Bergmann]

On Saturday 08 March 2008, Murali Iyer wrote:
> +
> +#ifdef    CONFIG_TB_AXON_MTRR_SMA
> +#include <asm/mtrr.h>
> +#endif

You just discovered a bad interface provided by the kernel.
Instead of using that like the last few people did, you can
help everyone by fixing this. The correct solution is to
have an include/linux/mtrr.h file that you can include
in your driver and that either includes asm/mtrr.h or
provides no-op functions with the same names.

> +
> +/* externs */
> +
> +extern const char *axon_name;
> +extern int  axon_local_sma_pages;
> +extern int  axon_major;
> +extern int  num_axons_found;
> +extern int  axon_local_sma_pages;
> +extern int  axon_local_d2d_pages;
> +
> +extern const struct attribute_group axon_attr_group;

You should never have any extern declarations in an implementation
file, it makes it easy to add bugs whenever the code changes.

Instead, try to avoid globals first. If that doesn't work, put the
declaration into a shared header file that is included by both
the file defining the variable and the file using it.

> +int __init axon_arch_dmax_init(void)
> +{
> +	return 0;	/* nothing to do on host... yet */
> +}

I think this should be __devinit, not __init. You probably
get section mismatch warnings in some way if you do this wrong
though.

Note that with pci hotplugging, you can always add devices after
the __init section has been removed.

> +inline u32 axon_read_pim(struct axon *axon_priv, int reg)
> +{
> +	return readl(axon_priv->mmio_base + PCIE_CFG_OFFSET + reg);
> +}

'inline' probably does not do what you think it does with gcc.
Always use 'static inline'.

Try not do mix ioreadXX and readl in the same driver. You can always
use ioreadXX when you use pcim_iomap and similar functions.

> +	/* Indicate interface not ready */
> +	if ((rc = pci_write_config_byte(axon_priv->axon_device,
> +		PCIE_CFG_LINK_CAP_PORT, 0))) {
> +		printk(KERN_ERR "%s: pci config space 0x%02x write error."
> +			" rc=0x%08x\n",
> +			axon_priv->irq_name, PCIE_CFG_LINK_CAP_PORT, rc);
> +	}

style comment: use

	rc = func();
	if (rc)
		error_handling();

instead of

	if ((rc = func()))
		error_handling();


to make your code better readable. Also, don't use curly braces
around a one-line basic block.

Do you really have to change the PCI config space from the device
driver? Usually that should only be done only by the PCI subsystem
driver.

> +/**
> + *  config remote sma
> + */
> +void axon_pci_config_remote_sma(struct axon	*axon_priv)
> +{
> +
> +	if ( axon_priv == NULL) {
> +		pr_debug("axon_map_remote_sma: no priv\n");
> +		return;
> +	}

Style: Please write this as

	if (!axon_priv) {
		...
		return;
	}

Or better, don't check this at all, as it will only cause
kernel bugs to be silently ignored, instead of causing
a helpful message. In the next line, you dereference the
pointer anyway, so you get an Oops if it is NULL.

Then just don't call the function with an invalid argument.

> +/*
> + * axon_interrupt -
> + */
> +static irqreturn_t axon_interrupt( int	  irq_nr,
> +				   void	 *apriv)
> +{
> +	u32	mask;
> +	u8	ready;
> +	struct axon	    *priv  = (struct axon *) apriv;

If you don't need the cast, you should not write it explicitly,
here it is not needed because you get a void* argument anyway.

> +	/* todo -- add thread protection */
> +	current_axon = num_axons_found++;
> +	axon_priv = &axon_device_table[ current_axon];

Don't use a device table like this, it will cause you more problems
than you want, at least at the point where you have to deal with multiple
hotplugged axon cards in a single big machine.

> +
> +	/* initialize priv */
> +	axon_priv->index = current_axon;
> +	axon_init_instance( axon_priv);
> +	axon_priv->axon_device = dev;
> +
> +	/* initialize cdev */
> +	axon_setup_cdev(axon_priv);
> +
> +	/* add /sys/class/axon/axon0 instance */
> +	pr_debug("Adding /sys/class/axon/axon%d instance \n", current_axon);
> +	axon_sysfs_add_device( axon_priv, current_axon );
> +	axon_debugfs_add_device( axon_priv, current_axon );

You're calling lots of functions here that are not yet defined as of
patch 3/16. This is ok as long as you don't add the Kconfig and Makefile
changes at this point, but it would be better to sort the patches in
a way that you first define the APIs that you are using in the later
patches.

> +	/* decode the pci memory regions */
> +	axon_priv->pcie_mem0base = pci_resource_start(dev, 0);
> +	axon_priv->pcie_mem0len  = pci_resource_len(dev, 0);
> +	axon_priv->pcie_mem1base = pci_resource_start(dev, 2);
> +	axon_priv->pcie_mem1len  = pci_resource_len(dev, 2);
> +	axon_priv->pcie_mem2base = pci_resource_start(dev, 4);
> +	axon_priv->pcie_mem2len  = pci_resource_len(dev, 4);

I don't think you need the physical addresses here, why not just
pcim_iomap the resources directly?

> +	/* setup axon regs and scratchpad */
> +	if ( (axon_priv->pcie_mem1base != 0) &&
> +	     (axon_priv->pcie_mem1len != 0) ) {
> +		axon_priv->mmio_base = (void __iomem *)
> +			ioremap( axon_priv->pcie_mem1base,
> +				 axon_priv->pcie_mem1len);

As above, you can ignore the '!= 0' part and the typecast,
as these do not improve readability.

> +	rc = dmax_pci_probe(axon_priv);
> +	if (rc) {
> +		pr_debug("%s.%d: DMAx probe failed with %d.\n",
> +			MODULE_NAME, current_axon, rc);
> +		goto error;
> +	}

Instead of pr_debug, you can use dev_dbg() here, to print out
the driver and device name in a standard format.

> +	/* allocate the dma command buffer */
> +	axon_priv->raw_dma_cmd_buf = kzalloc((DMA_CMD_BUF_SIZE_DEFAULT +
> +					      PAGE_SIZE),
> +					     GFP_KERNEL);

When allocating multiple pages, it is better to use __get_free_pages(),
as that does not stress the slab allocator that much.

> +error:
> +	/* unmap  registers */
> +	if ( axon_priv->mmio_base) {
> +		iounmap( axon_priv->mmio_base);
> +	}
> +	return rc;
> +}

When you use pcim_iomap, you don't need an explicit unmap any more.

> +	mem = (void __iomem *) ioremap( axon_priv->pcie_mem2base,  len);

This cast should not be needed.

> Index: linux-2.6.23.1/drivers/misc/triblade/axon_pci.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.23.1/drivers/misc/triblade/axon_pci.h
> @@ -0,0 +1,136 @@
> +/**
> + * axon_pci - provides pci interface for the host side of the axon interface
> + *

Not sure why you need a header file for the pci driver, as nothing else
should rely on that one directly.

> +#define MODULE_NAME	"axon"

You don't need that macro, just put it directly into the driver definition
and use dev->driver->name in the other places.

> +#define MAX_NUM_AXONS	4

It's better to not rely on arbitrary limits. Usually the code even gets
simpler if you just pass the right pointers around.

> +/* macros to access scratchpad */
> +/* read and write with local processor byte order */
> +#define SPREG_RD(dev, reg) ioread32(&dev->reg)
> +#define SPREG_WRT(dev, value, reg) iowrite32(value, &dev->reg)
> +
> +/* read and write with remote processor byte order (implicit byte swap) */
> +#define SPREG_RD_REMOTE(dev, reg) ioread32be(&dev->reg)
> +#define SPREG_WRT_REMOTE(dev, value, reg) iowrite32be(value, &dev->reg)

These would better be expressed as static inline functions, which have
some type checking in them. They should also have an appropriate prefix
and not use capital letters.

> +/*
> + * axon_pci_ids
> + */
> +static struct pci_device_id axon_pci_ids[] = {
> +	{PCI_VENDOR_ID_IBM, 0x032D,  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +	{},
> +};

A header file is an interface and should therefore never have static
variable definitions in it that end up getting duplicated in every file
including it.
Similarly, any other definition that is only used in one file should
not go into a header but into the file using it.

> +static inline int axon_arch_register_driver(void)
> +{
> +	pr_debug(" axon_pci: register pci driver\n");
> +	return pci_register_driver(&axon_pci_driver);
> +}
> +
> +static inline void axon_arch_unregister_driver(void)
> +{
> +	pr_debug(" axon_pci: unregister pci driver\n");
> +	pci_unregister_driver( &axon_pci_driver);
> +}

This seems to break if you include both the headers for
PCI and OF implementations from the same place.
For the registration, the solution is very easy, just register
them from a module_init() function instead of introducing a
global symbol.

> +static inline void axon_arch_map_remote_mem(struct axon *axon_priv)
> +{
> +	axon_pci_config_remote_sma( axon_priv);
> +}

This will probably have to be a function pointer in the axon_priv
structure. If you have more of these, make a static structure with
all the function pointers and put a pointer to that structure into
the device.

	Arnd <><