Re: [net-2.6 PATCH 6/10] Neterion: New driver: Hardware init & configuration

[Andi Kleen <andi@firstfloor.org>]

Ramkrishna Vepa <ram.vepa@neterion.com> writes:

> +struct __hw_channel*
> +__hw_channel_allocate(
> +	struct __hw_device *hldev,
> +	struct __hw_vpath_handle *vph,
> +	enum __hw_channel_type type,
> +	u32 length,
> +	u32 alloc_work_array,
> +	u32 alloc_free_array,
> +	u32 alloc_reserve_array,
> +	u32 per_dtr_space,
> +	void *userdata)

Having so many arguments in a function is usually a clear sign that it needs
to be refactored into smaller functions.

> +	channel = (struct __hw_channel *) vmalloc(size);

You seem to use vmalloc nearly everywhere. First that has a lot of 
overhead (rounds up to pages, flushes TLBs) and also will cause more
TLB misses. It's better to avoid it when not absolutely needed.

> +	vxge_assert(channel != NULL);
> +
> +	hldev = (struct __hw_device  *)channel->devh;

That assert is pretyt pointless because you'll just get a NULL pointer
reference next. Same true all over. asserts (or rather BUG_ON) only
make sense when they check for something that's not nearly obvious
given a oops.

> +	while (next_ptr != 0) {
> +
> +		cap_id = VXGE_HW_PCI_CAP_ID((((u8 *)pci_config)  +  next_ptr));
> +
> +		switch (cap_id) {
> +
> +		case VXGE_HW_PCI_CAP_ID_PM:
> +			hldev->pci_caps.pm_cap_offset = next_ptr;

This all could be done much shorter with a table.

> +	if (VXGE_HW_PCI_CONFIG_SPACE_SIZE <= 0x100)
> +		goto exit;
> +
> +	next_ptr = 0x100;

Such magic numbers are frowned upon.

Also in general you should use the pci capabilities accessor functions
provided by the PCI layer.


> +			break;
> +		case VXGE_HW_PCI_EXT_CAP_ID_DSN:
> +			hldev->pci_e_ext_caps.dsn_cap_offset = next_ptr;
> +			break;
> +		case VXGE_HW_PCI_EXT_CAP_ID_PWR:
> +			hldev->pci_e_ext_caps.pwr_budget_cap_offset = next_ptr;
> +			break;
> +
> +		default:
> +
> +			break;
> +		}
> +
> +		next_ptr = (u16)VXGE_HW_PCI_EXT_CAP_NEXT(
> +				*(u32 *)(((u8 *)pci_config)  +  next_ptr));

Especially the patch orgies are scary. Does that really use readl() et.al.
correctly? I doubt it. Again this should use the pci layer code for this.

> +/**
> + * vxge_hw_device_private_set - Set driver context.
> + * @hldev: HW device handle.
> + * @data: pointer to driver context
> + *
> + * Use HW device to set driver context.
> + *
> + * See also: vxge_hw_device_private_get()
> + */
> +void vxge_hw_device_private_set(struct __hw_device *hldev, void *data)
> +{
> +	hldev->upper_layer_data = data;
> +}

Such wrappers are not encouraged in Linux code, which aims to do with
only necessary abstraction. Lots of occurrences.

> +
> +#ifdef VXGE_HW_INTERNAL_COMPILER_ERROR
> +
> +#pragma optimize("", off)
> +
> +#endif

That's not for gcc isn't it?

> +/*
> + * __hw_ring_block_memblock_idx - Return the memblock index
> + * @block: Virtual address of memory block
> + *
> + * This function returns the index of memory block
> + */
> +static inline u32
> +__hw_ring_block_memblock_idx(
> +	u8 (block)[VXGE_HW_BLOCK_SIZE])
> +{
> +	return (u32)*((u64 *)((u8 *)block  +
> +		VXGE_HW_RING_MEMBLOCK_IDX_OFFSET));

Is that readl/writel clean again?

In general your driver looks like it could use a pass with sparse's
__iomem checking (make V=1)

> +static enum vxge_hw_status
> +__hw_ring_mempool_item_alloc(
> +	struct vxge_hw_mempool *mempoolh,
> +	void *memblock,
> +	u32 memblock_index,
> +	struct vxge_hw_mempool_dma *dma_object,
> +	void *item,
> +	u32 index,
> +	u32 is_last,
> +	void *userdata)

Again far too many arguments.


Stopped reading for now, but that file needs a lot of work in general.

-Andi