[PATCH]Fix to the new sysfs bin file support

[PATCH]Fix to the new sysfs bin file support

[Rusty Lynch]

I happen to notice the new binary file support in sysfs and had to take
for a spin.  If I understand this correct my write file will need to
allocate the buffer->data, but then I have no way of freeing that memory
since I don't get a release callback.

Here is a patch that:
* makes sysfs cleanup the buffer->data allocated by the attribute write
functions
* fixes a bug that causes the kernel to oops when somebody attempts to
write to the file.

BTW, would you be totally opposed to a patch that added open, release,
and ioctl to the list of functions supported by sysfs binary files?

Another question... How would a driver know that the various write and
read calls are coming from the same open, or would there be a way for a
driver to make it so that only one thing can open the sysfs file at a
time?
    --rustyl

--- fs/sysfs/bin.c.orig	2003-03-05 18:33:44.000000000 -0800
+++ fs/sysfs/bin.c	2003-03-05 18:34:01.000000000 -0800
@@ -50,6 +50,10 @@
 		ret = count;
 	}
  Done:
+	if (buffer && buffer->data) {
+		kfree(buffer->data);
+		buffer->data = NULL;
+	}
 	return ret;
 }
 
@@ -66,7 +70,7 @@
 static int fill_write(struct file * file, const char * userbuf, 
 		      struct sysfs_bin_buffer * buffer)
 {
-	return copy_from_user(buffer,userbuf,buffer->count) ?
+	return copy_from_user(buffer->data,userbuf,buffer->count) ?
 		-EFAULT : 0;
 }
 

Re: [PATCH]Fix to the new sysfs bin file support

[Patrick Mochel]

On 5 Mar 2003, Rusty Lynch wrote:

> I happen to notice the new binary file support in sysfs and had to take
> for a spin.  If I understand this correct my write file will need to
> allocate the buffer->data, but then I have no way of freeing that memory
> since I don't get a release callback.

Hm, good point. Perhaps a better way to do it would be:

open()
	allocate buffer
	fill buffer

read()
	copy_to_user()

write()
	copy_from_user()
	sync with driver

close()
	free buffer

Once the buffer is filled, then the user could read()/seek() on it to 
their hearts' content, without having to call the driver back. 

On a write, the user would end up modifying the desired bits of the
buffer, then sysfs would call the ->write() method, passing the entire 
buffer. 

All this assumes that the buffer will not change while the file is open, 
but for these purposes, I think that's ok to make.

> Here is a patch that:
> * makes sysfs cleanup the buffer->data allocated by the attribute write
> functions
> * fixes a bug that causes the kernel to oops when somebody attempts to
> write to the file.

Thanks.

> BTW, would you be totally opposed to a patch that added open, release,
> and ioctl to the list of functions supported by sysfs binary files?

->ioctl() - No. 

Why ->open() and ->release()? If we free the buffer in our release(), then 
why do you need a notification? 

> Another question... How would a driver know that the various write and
> read calls are coming from the same open, or would there be a way for a
> driver to make it so that only one thing can open the sysfs file at a
> time?

I don't think you could know that a ->read() came from the same process as 
the previous ->read(). Why would you need to know that?

Thanks,

	-pat

Re: [PATCH]Fix to the new sysfs bin file support

[Rusty Lynch]

On Thu, 2003-03-06 at 07:34, Patrick Mochel wrote:
<snip> 
> > BTW, would you be totally opposed to a patch that added open, release,
> > and ioctl to the list of functions supported by sysfs binary files?
> 
> ->ioctl() - No. 
> 
> Why ->open() and ->release()? If we free the buffer in our release(), then 
> why do you need a notification? 

I was looking to see if I could use a sysfs bin file as a replacement
for the char device file used by watchdog drivers which need to:
1. know when the device file is opened in order to start the timer
2. know when the device file is released in order to stop the timer if
the nowayout option is not on

This also why I was asking for ioctl's.

Now I suspected that this was not the intended usage for a sysfs bin
file, but I wasn't sure.

> 
> > Another question... How would a driver know that the various write and
> > read calls are coming from the same open, or would there be a way for a
> > driver to make it so that only one thing can open the sysfs file at a
> > time?
> 
> I don't think you could know that a ->read() came from the same process as 
> the previous ->read(). Why would you need to know that?

I was under the impression that some existing device drivers could use
something from the file pointer to implement a session for a specific
open/close.  All I have ever done was to force only one process to open
my device file at a time so I would not have to worry about concurrent
sessions.

If I am mistaken about the ability to distinguish between different
processes, then no big deal, but it would be nice to be able to force
only one open at a time.  (Maybe a flag in bin_attribute?)

    --rustyl
> 
> Thanks,
> 
> 	-pat