[RFC PATCH] device-core: sysfs open - close notify

[RFC PATCH] device-core: sysfs open - close notify

[Samu Onkalo]

Device drivers have no idea when somebody in userspace keeps some sysfs entry
open. The driver just received read or write calls. The driver may want to
control HW state based on activity so it either have to turn HW on and off
for each sysfs access or it needs separate enable disable entry which controls
the HW state. In cases where sysfs is used to pass some events under interrupt
control (like proximity events from the proximity sensors) it is not enough to
just keep sysfs entry open in userspace.

This patch adds a possibility for driver to know when some sysfs entry is
open.

Device driver structure is enhanced with optional open and close methods.
Device driver can provide those if it needs information about the sysfs
activity. Device core is enhanced to provide open / close methods for
sysfs file operations. Sysfs open / close operations call open / close
methods if they are provided.

Signed-off-by: Samu Onkalo <samu.p.onkalo@nokia.com>
---
 drivers/base/core.c    |   24 ++++++++++++++++++++++++
 fs/sysfs/file.c        |   16 +++++++++++++++-
 include/linux/device.h |    2 ++
 include/linux/sysfs.h  |    2 ++
 4 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6ed6454..f9c5810 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -112,9 +112,33 @@ static ssize_t dev_attr_store(struct kobject *kobj, struct attribute *attr,
 	return ret;
 }
 
+static int dev_attr_open(struct kobject *kobj, struct attribute *attr)
+{
+	struct device *dev = to_dev(kobj);
+	int ret = 0;
+	if (dev->driver && unlikely(dev->driver->open)) {
+		ret = dev->driver->open(dev, attr);
+		if (unlikely(ret > 0)) {
+			WARN(1, KERN_ERR "%s driver open call returned "
+				"positive value\n", dev->driver->name);
+			ret = -EPERM;
+		}
+	}
+	return ret;
+}
+
+static void dev_attr_close(struct kobject *kobj, struct attribute *attr)
+{
+	struct device *dev = to_dev(kobj);
+	if (dev->driver && dev->driver->close)
+		dev->driver->close(dev, attr);
+}
+
 static const struct sysfs_ops dev_sysfs_ops = {
 	.show	= dev_attr_show,
 	.store	= dev_attr_store,
+	.open	= dev_attr_open,
+	.close	= dev_attr_close,
 };
 
 
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index da3fefe..4feaec2 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -392,10 +392,18 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 	if (error)
 		goto err_free;
 
+	/* Notify about open dirent */
+	if (ops->open) {
+		error = ops->open(kobj, attr_sd->s_attr.attr);
+		if (error)
+			goto err_open;
+	}
+
 	/* open succeeded, put active references */
 	sysfs_put_active(attr_sd);
 	return 0;
-
+err_open:
+	sysfs_put_open_dirent(attr_sd, buffer);
  err_free:
 	kfree(buffer);
  err_out:
@@ -407,6 +415,12 @@ static int sysfs_release(struct inode *inode, struct file *filp)
 {
 	struct sysfs_dirent *sd = filp->f_path.dentry->d_fsdata;
 	struct sysfs_buffer *buffer = filp->private_data;
+	struct kobject *kobj = sd->s_parent->s_dir.kobj;
+	const struct sysfs_ops *ops = kobj->ktype->sysfs_ops;
+
+	/* Notify about dirent release */
+	if (ops->close)
+		ops->close(kobj, sd->s_attr.attr);
 
 	sysfs_put_open_dirent(sd, buffer);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index dd48953..788d982 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -138,6 +138,8 @@ struct device_driver {
 	void (*shutdown) (struct device *dev);
 	int (*suspend) (struct device *dev, pm_message_t state);
 	int (*resume) (struct device *dev);
+	int (*open) (struct device *dev, struct attribute *);
+	void (*close) (struct device *dev, struct attribute *);
 	const struct attribute_group **groups;
 
 	const struct dev_pm_ops *pm;
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 30b8815..9b92c29 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -112,6 +112,8 @@ struct bin_attribute {
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
+	int (*open)(struct kobject *, struct attribute *);
+	void (*close)(struct kobject *, struct attribute *);
 };
 
 struct sysfs_dirent;
-- 
1.6.0.4

Re: [RFC PATCH] device-core: sysfs open - close notify

[Ming Lei]

2010/10/31 Samu Onkalo <samu.p.onkalo@nokia.com>:
> Device drivers have no idea when somebody in userspace keeps some sysfs entry
> open. The driver just received read or write calls. The driver may want to
> control HW state based on activity so it either have to turn HW on and off
> for each sysfs access or it needs separate enable disable entry which controls
> the HW state. In cases where sysfs is used to pass some events under interrupt
> control (like proximity events from the proximity sensors) it is not enough to
> just keep sysfs entry open in userspace.

Since very few drivers have this kind of requirement, why not take notifier
call chain mechanism to do such thing? eg. register a notifier function
if the driver is interested in such kind of open/close events.

Your patch may cause many unnecessary memory waste because
most of drivers does not need attribute file .open/.close notifier.


thanks,
-- 
Lei Ming

Re: [RFC PATCH] device-core: sysfs open - close notify

[Alan Cox]

> Your patch may cause many unnecessary memory waste because
> most of drivers does not need attribute file .open/.close notifier.

Firstly there are not that many driver objects in a small system so it
wouldn't take that much to shift the balance the other way. Secondly
its becoming clear that every time a driver goes to runtime pm these
issues come up - even with things like configuration values for drivers
that need to wake the hardware and then silence it.

So the whole sysfs/open thing is going to keep haunting us with runtime
pm, the question is where to put the callbacks so we don't bloat stuff.
Clearly not per attribute or per sysfs node. One possibility would be
with the runtime pm stuff, but that would need a clean reliable way to
go sysfs->device->runtime_pm

There are also obvious hackish ways to handle it like passing a 0
length read to indicate close etc - they save memory but they are
asking for problems in future.

Re: [RFC PATCH] device-core: sysfs open - close notify

[Onkalo Samu]

On Mon, 2010-11-01 at 18:11 +0100, ext Alan Cox wrote:
> > Your patch may cause many unnecessary memory waste because
> > most of drivers does not need attribute file .open/.close notifier.
> 
> Firstly there are not that many driver objects in a small system so it
> wouldn't take that much to shift the balance the other way. Secondly
> its becoming clear that every time a driver goes to runtime pm these
> issues come up - even with things like configuration values for drivers
> that need to wake the hardware and then silence it.
> 
> So the whole sysfs/open thing is going to keep haunting us with runtime
> pm, the question is where to put the callbacks so we don't bloat stuff.
> Clearly not per attribute or per sysfs node. One possibility would be
> with the runtime pm stuff, but that would need a clean reliable way to
> go sysfs->device->runtime_pm
> 
> There are also obvious hackish ways to handle it like passing a 0
> length read to indicate close etc - they save memory but they are
> asking for problems in future.
> 

Memory footprint could be minimized by combining separate open close
functions to one like sysfs_open_close_notify and the actual operation
would be a call parameter. But is that hackish?

And perhaps some flags could be used to indicate which attributes trigs
the open / close notification.

-Samu

 

Re: [RFC PATCH] device-core: sysfs open - close notify

[Enrico Weigelt]

* Onkalo Samu <samu.p.onkalo@nokia.com> wrote:

<snip />

Perhaps it also could be an more generic event notification op,
where some event id or bitmask tells what actually happened ?
Something like:

    ...
    void (*notify)(struct kobject *, struct attribute *, int event, void * param);
    ...

That would at least save one ptr of space and can be extended for
other stuff later.


cu
-- 
----------------------------------------------------------------------
 Enrico Weigelt, metux IT service -- http://www.metux.de/

 phone:  +49 36207 519931  email: weigelt@metux.de
 mobile: +49 151 27565287  icq:   210169427         skype: nekrad666
----------------------------------------------------------------------
 Embedded-Linux / Portierung / Opensource-QM / Verteilte Systeme
----------------------------------------------------------------------