Hi Joel,

On Jun 25, 2014, at 3:52 PM, Joel Becker wrote:

On Sun, Jun 22, 2014 at 12:37:42PM +0300, Pantelis Antoniou wrote:

quoted

ConfigFS lacked binary attributes up until now. This patch
introduces support for binary attributes in a somewhat similar
manner of sysfs binary attributes albeit with changes that
fit the configfs usage model.

Problems that configfs binary attributes fix are everything that
requires a binary blob as part of the configuration of a resource,
such as bitstream loading for FPGAs, DTBs for dynamically created
devices etc.

I wanted to object on principle, but I actually think I like the way you
did this.  More comments inline.

Hehe. It sounded weird to me at first, but actually it's not so bad.
It does solve a problem.

quoted

Look at Documentation/filesystems/configfs/configfs.txt for internals
and howto use them.

This patch generates a bunch of checkpatch warnings, but this stems from
following the formatting of the configfs code, so please ignore.

I thought someone had looked into cleaning up that cut-n-paste legacy.
I'm fine with new code following sanity..

OK.

quoted

@@ -431,7 +438,9 @@ static int configfs_attach_attr(struct configfs_dirent * sd, struct dentry * den

	spin_unlock(&configfs_dirent_lock);

	error = configfs_create(dentry, (attr->ca_mode & S_IALLUGO) | S_IFREG,
-				configfs_init_file);
+				(sd->s_type & CONFIGFS_ITEM_ATTR) ?
+					configfs_init_file :
+					configfs_init_bin_file);
	if (error) {
		configfs_put(sd);
		return error;

@@ -591,6 +600,7 @@ static int populate_attrs(struct config_item *item)

{
	struct config_item_type *t = item->ci_type;
	struct configfs_attribute *attr;
+	struct configfs_bin_attribute *bin_attr;
	int error = 0;
	int i;

@@ -603,6 +613,13 @@ static int populate_attrs(struct config_item *item)

		}
	}

No need for this blank line.

OK

quoted

+	if (t->ct_bin_attrs) {
+		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
+			if ((error = configfs_create_bin_file(item, bin_attr)))
+				break;
+		}
+	}
+
	if (error)
		detach_attrs(item);

diff --git a/fs/configfs/file.c b/fs/configfs/file.c
index 1d1c41f..aec051b 100644
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c

@@ -48,6 +48,10 @@ struct configfs_buffer {

	struct configfs_item_operations	* ops;
	struct mutex		mutex;
	int			needs_read_fill;
+	int			read_in_progress;
+	int			write_in_progress;
+	char			*bin_buffer;
+	int			bin_buffer_size;
};

@@ -107,8 +111,15 @@ static ssize_t

configfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
	struct configfs_buffer * buffer = file->private_data;
+	struct configfs_dirent * sd = file->f_path.dentry->d_fsdata;
	ssize_t retval = 0;

+	if (WARN_ON(sd == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(!(sd->s_type & CONFIGFS_ITEM_ATTR)))
+		return -EINVAL;
+
	mutex_lock(&buffer->mutex);
	if (buffer->needs_read_fill) {
		if ((retval = fill_read_buffer(file->f_path.dentry,buffer)))

@@ -123,6 +134,93 @@ out:

	return retval;
}

+/**
+ *	configfs_read_bin_file - read a binary attribute.
+ *	@file:	file pointer.
+ *	@buf:	buffer to fill.
+ *	@count:	number of bytes to read.
+ *	@ppos:	starting offset in file.
+ *
+ *	Userspace wants to read a binary attribute file. The attribute descriptor
+ *	is in the file's ->d_fsdata. The target item is in the directory's
+ *	->d_fsdata.
+ *
+ *	We check whether we need to refill the buffer. If so we will
+ *	call the attributes' ops->read_bin_attribute() twice. The first time we
+ *	will pass a NULL as a buffer pointer, which the attributes' method
+ *	will use to return the size of the buffer required. If no error
+ *	occurs we will allocate the buffer using kmalloc and call 
+ *	ops->read_bin_atribute() again passing that buffer as an argument.
+ *	Then we just copy to user-space using simple_read_from_buffer.
+ */
+
+static ssize_t
+configfs_read_bin_file(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct configfs_buffer * buffer = file->private_data;
+	struct dentry *dentry = file->f_path.dentry;
+	struct configfs_dirent * sd = dentry->d_fsdata;
+	struct config_item * item = to_item(dentry->d_parent);
+	struct configfs_item_operations * ops = buffer->ops;
+	struct configfs_attribute * attr = to_attr(dentry);
+	struct configfs_bin_attribute *bin_attr =
+		container_of(attr, struct configfs_bin_attribute, attr);

You defined to_bin_attr().  Use it here and in write_bin_file() rather
than open-coding the container_of() calls.

OK

quoted

+	ssize_t retval = 0;
+	ssize_t len = min_t(size_t, count, PAGE_SIZE);
+
+	if (WARN_ON(sd == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(!(sd->s_type & CONFIGFS_ITEM_BIN_ATTR)))
+		return -EINVAL;
+
+	mutex_lock(&buffer->mutex);
+
+	/* we don't support switching read/write modes */
+	if (buffer->write_in_progress) {
+		retval = -EINVAL;
+		goto out;
+	}
+	if (!buffer->read_in_progress)
+		buffer->read_in_progress = 1;

You can always set read_in_progress, even if the read has already
started.

OK

quoted

+
+	if (buffer->needs_read_fill) {
+
+		/* perform first read with buf == NULL to get extent */
+		len = ops->read_bin_attribute(item, bin_attr, NULL, 0);
+		if (len < 0) {
+			retval = len;
+			goto out;
+		}
+
+		buffer->bin_buffer = kmalloc(len, GFP_KERNEL);
+		if (buffer->bin_buffer == NULL) {
+			retval = -ENOMEM;
+			goto out;
+		}
+		buffer->bin_buffer_size = len;
+
+		/* perform second read to fill buffer */
+		len = ops->read_bin_attribute(item, bin_attr,
+				buffer->bin_buffer, len);
+		if (len < 0) {
+			retval = len;
+			kfree(buffer->bin_buffer);
+			buffer->bin_buffer_size = 0;
+			buffer->bin_buffer = NULL;
+			goto out;
+		}
+
+		buffer->needs_read_fill = 0;
+	}
+
+	retval = simple_read_from_buffer(buf, count, ppos, buffer->bin_buffer,
+					buffer->bin_buffer_size);
+out:
+	mutex_unlock(&buffer->mutex);
+	return retval;
+}
+

/**
 *	fill_write_buffer - copy buffer from userspace.

@@ -198,8 +296,15 @@ static ssize_t

configfs_write_file(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
	struct configfs_buffer * buffer = file->private_data;
+	struct configfs_dirent * sd = file->f_path.dentry->d_fsdata;
	ssize_t len;

+	if (WARN_ON(sd == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(!(sd->s_type & CONFIGFS_ITEM_ATTR)))
+		return -EINVAL;
+
	mutex_lock(&buffer->mutex);
	len = fill_write_buffer(buffer, buf, count);
	if (len > 0)

@@ -210,10 +315,80 @@ configfs_write_file(struct file *file, const char __user *buf, size_t count, lof

	return len;
}

-static int check_perm(struct inode * inode, struct file * file)
+/**
+ *	configfs_write_bin_file - write a binary attribute.
+ *	@file:	file pointer
+ *	@buf:	data to write
+ *	@count:	number of bytes
+ *	@ppos:	starting offset
+ *
+ *	Writting to a binary attribute file is similar to a normal read.
+ *	We buffer the consecutive writes (binary attribute files do not
+ *	support lseek) in a continuously growing buffer, but we don't 
+ *	commit until the close of the file.
+ */
+
+static ssize_t
+configfs_write_bin_file(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	struct configfs_buffer * buffer = file->private_data;
+	struct dentry *dentry = file->f_path.dentry;
+	struct configfs_dirent * sd = dentry->d_fsdata;
+	void *tbuf = NULL;
+	ssize_t len;
+
+	if (WARN_ON(sd == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(!(sd->s_type & CONFIGFS_ITEM_BIN_ATTR)))
+		return -EINVAL;
+
+	mutex_lock(&buffer->mutex);
+
+	/* we don't support switching read/write modes */
+	if (buffer->read_in_progress) {
+		len = -EINVAL;
+		goto out;
+	}
+	if (!buffer->write_in_progress)
+		buffer->write_in_progress = 1;
+
+	/* buffer grows? */
+	if (*ppos + count > buffer->bin_buffer_size) {
+
+		tbuf = kmalloc(*ppos + count, GFP_KERNEL);
+		if (tbuf == NULL) {
+			len = -ENOMEM;
+			goto out;
+		}
+
+		/* copy old contents */
+		if (buffer->bin_buffer) {
+			memcpy(tbuf, buffer->bin_buffer,
+					buffer->bin_buffer_size);

Fix the argument alignment.  Do this elsewhere in your patch, too.

Hmm, I see.

quoted

+			kfree(buffer->bin_buffer);
+		}
+
+		/* clear the new area */
+		memset(tbuf + buffer->bin_buffer_size, 0,
+				*ppos + count - buffer->bin_buffer_size);
+		buffer->bin_buffer = tbuf;
+		buffer->bin_buffer_size = *ppos + count;
+	}
+
+	len = simple_write_to_buffer(buffer->bin_buffer, buffer->bin_buffer_size,
+			ppos, buf, count);
+	if (len > 0)
+		*ppos += len;
+out:
+	mutex_unlock(&buffer->mutex);
+	return len;
+}
+

<snip>

quoted

diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index 5946ad9..dd9c24b 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c

@@ -231,7 +231,7 @@ const unsigned char * configfs_get_name(struct configfs_dirent *sd)

	if (sd->s_type & (CONFIGFS_DIR | CONFIGFS_ITEM_LINK))
		return sd->s_dentry->d_name.name;

-	if (sd->s_type & CONFIGFS_ITEM_ATTR) {
+	if (sd->s_type & (CONFIGFS_ITEM_ATTR | CONFIGFS_ITEM_BIN_ATTR)) {
		attr = sd->s_element;
		return attr->ca_name;
	}

diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 34025df..15f1079 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h

<snip>

quoted

@@ -207,6 +209,89 @@ static ssize_t _item##_attr_store(struct config_item *item,		\

	return ret;							\
}

+struct file;
+struct vm_area_struct;
+
+struct configfs_bin_attribute {
+	struct configfs_attribute	attr;
+	void				*private;
+};

cb_attr and cb_private or similar prefixes, please.

OK, will roll everything up in the next submission.

Joel

Regards

-- Pantelis