[Cbe-oss-dev] [patch 11/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 11/16] axon driver, sysfs support
> 
> Description:
> 	This patch provides sysfs and debugfs support. Driver
> 	settings ( such as local and remote shared memory sizes)
> 	are communicated to userspace using the sysfs interfaces.
> 	Additionally, there is some debugging facilities using
> 	debugfs.
 
The sysfs attributes might make more sense to get moved to the
code that works on these variables, since there is not much
code behind them, and it's not so much "sysfs support" that
you are interested in but rather you use attributes as a way
of setting the variables you already have.

> +static struct class  *axon_class;

Using a class and class_device seems odd here. You already have device
nodes for the device itself, so the attributes can just go in there.
Moreover, the class_device concept has been removed in recent kernels.

> +#ifdef CONFIG_TB_AXON_PCI
> +/**
> + * axon_soft_reset - called by user by writing through sysfs
> + * @cd: class device structure of the device
> + * @buf: buffer containing string passed by the user
> + * @count: number of characters in the buffer
> + *
> + * sending 1 resets PCIe interface
> + */
> +static ssize_t axon_soft_reset(struct class_device *cd,
> +                                 const char *buf, size_t count)
> +{
> +	struct axon *priv = (struct axon*) class_get_devdata(cd);
> +	unsigned long reset;
> +	char *endp;
> +
> +	reset = simple_strtoul(buf, &endp, 0);
> +	if (reset == 1 && *endp == '\n')
> +		axon_send_soft_reset(priv);
> +
> +	return strnlen(buf, count);
> +}
> +static CLASS_DEVICE_ATTR(reset,    S_IWUSR, NULL, axon_soft_reset);
> +#endif

The presence of this file should not depend of CONFIG_TB_AXON_PCI,
but rather on the question of the specific device is a south bridge
or a PCI device. To simplify this, you might want to always create
the file and implement axon_send_soft_reset as a no-op for the
southbridge.

> +
> +/**
> + * axon_sysfs_add_device - and dir entry for each device
> + */
> +int axon_sysfs_add_device( struct axon *priv,
> +			   int         instance)
> +{
> +	struct class_device *class_dev;
> +	int  err;
> +
> +	/* add sysfs directory */
> +	if ( axon_class) {
> +		int  i;
> +		class_dev = class_device_create( axon_class,
> +						 NULL,
> +						 priv->dev_id,
> +						 &(priv->axon_device->dev),
> +						 "axon%d", instance);
> +		if (IS_ERR(class_dev)) {
> +			err = PTR_ERR(class_dev);
> +			printk(KERN_DEBUG
> +				"%s: cannot create device\n",
> +				__FUNCTION__);
> +
> +			goto error_out;
> +		}
> +
> +		/* update private instance struct */
> +		priv->class = axon_class;
> +		priv->class_dev = class_dev;
> +		class_set_devdata( class_dev, priv);
> +
> +		/* add device entries */
> +		for ( i = 0; i < ARRAY_SIZE( axon_attrs) ; ++i) {
> +			err = class_device_create_file( class_dev,
> +							axon_attrs[i]);
> +			if (err) {
> +				printk(KERN_DEBUG
> +					"%s: cannot create file\n",
> +					__FUNCTION__);
> +				goto error_out_files;
> +			}
> +		}
> +	}
> +	return 0;
> +
> +error_out_files:
> +	class_dev = NULL;
> +error_out:
> +	return err;
> +}

You can simply use an attribute_group to create multiple attributes
at once, including correct cleanup in case of an error.

> +struct dfs_entry_struct {
> +	char              name[32];
> +	struct dentry    *entry;
> +	struct list_head  list;
> +};

This abstraction should not be necessary, nobody else needs this.

> +
> +/**
> + * axon_debugfs_add_device - and dir entry & elements for each device
> + * instance.
> + */
> +int axon_debugfs_add_device( struct axon *priv,
> +			     int          instance)
> +{
> +
> +	/*  add debufs entries */
> +	if ( dfs_rootdir) {
> +		char            dir_name[15];
> +		struct dentry  *inst_dir;
> +		sprintf(dir_name, "axon%d", instance);
> +		inst_dir = dfs_add_dir( dir_name, dfs_rootdir);
> +
> +		/* buf sizes */
> +		dfs_add_u32_entry( "sma_size",
> +				   (u32 *) &(priv->local_sma_size),
> +				   inst_dir);
> +		dfs_add_u32_entry( "remote_sma_size",
> +				   (u32 *) &(priv->remote_sma_size),
> +				   inst_dir);
> +		dfs_add_u32_entry( "dma_buffer_size",
> +				   (u32 *) &(priv->dma_cmd_buf_length),
> +				   inst_dir);
> +#ifdef CONFIG_TB_AXON_PCI
> +
> +		/* pcie addresses */
> +		dfs_add_u64_entry( "pcie_mem0_base",
> +				   (u64 *) &(priv->pcie_mem0base),
> +				   inst_dir);
> +		dfs_add_u64_entry( "pcie_mem0_len",
> +				   (u64 *) &(priv->pcie_mem0len),
> +				   inst_dir);
> +		dfs_add_u64_entry( "pcie_mem1_base",
> +				   (u64 *) &(priv->pcie_mem1base),
> +				   inst_dir);
> +		dfs_add_u64_entry( "pcie_mem1_len",
> +				   (u64 *) &(priv->pcie_mem1len),
> +				   inst_dir);
> +		dfs_add_u64_entry( "pcie_mem2_base",
> +				   (u64 *) &(priv->pcie_mem2base),
> +				   inst_dir);
> +		dfs_add_u64_entry( "pcie_mem2_len",
> +				   (u64 *) &(priv->pcie_mem2len),
> +				   inst_dir);
> +
> +#endif
> +
> +		/* phys addresses */
> +		dfs_add_u64_entry( "remote_sma_paddr",
> +				   (u64 *) &(priv->remote_sma_paddr),
> +				   inst_dir);
> +		dfs_add_u64_entry( "remote_d2d_paddr",
> +				   (u64 *) &(priv->remote_d2d_paddr),
> +				   inst_dir);
> +
> +		/* misc */
> +		dfs_add_u32_entry( "piggyback_irqs",
> +				   (u32 *) &(priv->piggyback_irqs),
> +				   inst_dir);
> +
> +		dfs_add_blob_entry( "local_d2d",
> +				    (struct debugfs_blob_wrapper *)&priv->local_d2d,
> +				    inst_dir);
> +		dfs_add_blob_entry( "local_d2d_mr",
> +				    (struct debugfs_blob_wrapper *)&priv->local_d2d_mr_address,
> +				    inst_dir);
> +	}
> +	return 0;
> +}

Instead of lots of calls to this, I would make an array listing all the
files and then walk that for both creation and removal of a device.

	Arnd <><